Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix iot test by using new method name #1528

Closed
wants to merge 3 commits into from
Closed

Fix iot test by using new method name #1528

wants to merge 3 commits into from

Conversation

mesmacosta
Copy link
Contributor

I noticed that this test started breaking in another PR that I opened.

Doing a quick analysis:

The iot/manager test failing seem to be related to this change:
googleapis/nodejs-iot@68a1d6e

Method name changed, but test from sample code was not updated.
client.registryPath -> client.deviceRegistryPath

So I can't say for sure, because I don't know the full extend of that commit, but this small change might fix it, I'd like to run it in the integrated environment to see if it will solve.
@fhinkel

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Oct 25, 2019
This was referenced Oct 25, 2019
Copy link
Contributor

@gguuss gguuss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you for the fix! I was trying to decide whether the right thing to do was to revise the client library or fix the sample.

@gguuss
Copy link
Contributor

gguuss commented Oct 25, 2019

Tests are passing for me locally, merging when green.

@gguuss gguuss added kokoro:force-run Add this label to force Kokoro to re-run the tests. kokoro:run Add this label to force Kokoro to re-run the tests. labels Oct 25, 2019
@kokoro-team kokoro-team removed kokoro:run Add this label to force Kokoro to re-run the tests. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Oct 25, 2019
@gguuss
Copy link
Contributor

gguuss commented Oct 25, 2019

I will see about getting the breaking change rolled back.

@fhinkel fhinkel added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 25, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 25, 2019
@gguuss gguuss added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Oct 26, 2019
@gguuss
Copy link
Contributor

gguuss commented Oct 26, 2019

I'll see if we can revert the breaking change

@fhinkel fhinkel changed the title Try to fix integrated test after method change Fix iot test by using new method name Oct 27, 2019
@mesmacosta
Copy link
Contributor Author

does it make sense to resolve the conflicting file? @gguuss

@gguuss
Copy link
Contributor

gguuss commented Nov 2, 2019

Yes, let's just go ahead and allow the breaking change in this case, I'll try and track down the other code samples that are out there requiring changes.

Copy link
Contributor

@gguuss gguuss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@gguuss gguuss added kokoro:force-run Add this label to force Kokoro to re-run the tests. kokoro:run Add this label to force Kokoro to re-run the tests. and removed do not merge Indicates a pull request not ready for merge, due to either quality or timing. labels Nov 5, 2019
@kokoro-team kokoro-team removed kokoro:run Add this label to force Kokoro to re-run the tests. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Nov 5, 2019
@gguuss
Copy link
Contributor

gguuss commented Nov 13, 2019

@mesmacosta Thank you so much for making the PR; we were able to get this resolved in the update to the IoT Client library version 1.3.1 so I'm going to go ahead and close this, favoring skipping the additional record of the breaking change as done in this PR.

@gguuss gguuss closed this Nov 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants