Skip to content
This repository was archived by the owner on Mar 9, 2026. It is now read-only.

test: presubmit against Lite samples#425

Merged
anguillanneuf merged 2 commits intomasterfrom
lite-samples-testing
Jun 11, 2021
Merged

test: presubmit against Lite samples#425
anguillanneuf merged 2 commits intomasterfrom
lite-samples-testing

Conversation

@anguillanneuf
Copy link
Contributor

@anguillanneuf anguillanneuf commented Jun 10, 2021

Add a presubmit test to run against samples in https://github.com/googleapis/python-pubsublite.

This is to prevent future development here from breaking python-pubsublite unknowingly, which has happened before in version 2.4.0: https://pypi.org/project/google-cloud-pubsub/#history

Hard-coded pytest code instead of using nox.

@snippet-bot
Copy link

snippet-bot bot commented Jun 10, 2021

No region tags are edited in this PR.

This comment is generated by snippet-bot.
If you find problems with this result, please file an issue at:
https://github.com/googleapis/repo-automation-bots/issues.
To update this comment, add snippet-bot:force-run label or use the checkbox below:

  • Refresh this comment

@product-auto-label product-auto-label bot added api: pubsub Issues related to the googleapis/python-pubsub API. samples Issues that are directly related to samples. labels Jun 10, 2021
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Jun 10, 2021
@anguillanneuf anguillanneuf force-pushed the lite-samples-testing branch from c3edc6e to 2a00008 Compare June 10, 2021 22:35
@anguillanneuf anguillanneuf force-pushed the lite-samples-testing branch from f3b3508 to 0000c3d Compare June 10, 2021 23:56
@anguillanneuf anguillanneuf force-pushed the lite-samples-testing branch from 0000c3d to 844d329 Compare June 11, 2021 00:14
@anguillanneuf anguillanneuf marked this pull request as ready for review June 11, 2021 00:29
@anguillanneuf anguillanneuf requested a review from a team June 11, 2021 00:29
@anguillanneuf
Copy link
Contributor Author

anguillanneuf commented Jun 11, 2021

The test was run and passed. See fusion:prod:cloud-devrel/client-libraries/python/googleapis/python-pubsub/presubmit/presubmit-against-pubsublite-samples

Highlighting a few areas for reviewers to comment:

  • For some reason, the added presubmit test isn't showing up in the build details by default, even though it ran. Also, a passing cloud-devrel/client-libraries/python/googleapis/python-pubsub/presubmit/presubmit seems to overwrite a failing cloud-devrel/client-libraries/python/googleapis/python-pubsub/presubmit/presubmit-against-pubsublite-samples. What can I do there?
  • I've hard-coded the test to use Python 3.6. Perhaps I shall add 3.7 and 3.8 too?

@busunkim96
Copy link
Contributor

busunkim96 commented Jun 11, 2021

For some reason, the added presubmit test isn't showing up in the build details by default, even though it ran. Also, a passing cloud-devrel/client-libraries/python/googleapis/python-pubsub/presubmit/presubmit seems to overwrite a failing cloud-devrel/client-libraries/python/googleapis/python-pubsub/presubmit/presubmit-against-pubsublite-samples. What can I do there?

I think if you rename the check on the internal config side it will show up as a separate check and not overwrite the existing presubmit.

I've hard-coded the test to use Python 3.6. Perhaps I shall add 3.7 and 3.8 too?

Do you think it's possible for a breaking change to only be noticeable in one Python version but not the others? If so, you can add the others. If you think it's unlikely I think it's OK to just have 3.6. :)

busunkim96
busunkim96 approved these changes Jun 11, 2021
View reviewed changes
Copy link
Contributor

@busunkim96 busunkim96 left a comment

Choose a reason for hiding this comment

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

Once the Kokoro status check has been renamed please add it to the repo settings config so it is marked required. https://github.com/googleapis/python-pubsub/blob/master/.github/sync-repo-settings.yaml#L7

@anguillanneuf
Copy link
Contributor Author

anguillanneuf commented Jun 11, 2021 •
edited
Loading

Thank you! Internal change (cl/378896781) submitted.

Do you think it's possible for a breaking change to only be noticeable in one Python version but not the others?

It's unlikely AFAIK. Having just 3.6 should be enough.

@anguillanneuf anguillanneuf added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 11, 2021
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 11, 2021
@anguillanneuf anguillanneuf merged commit c757a5e into master Jun 11, 2021
@anguillanneuf anguillanneuf deleted the lite-samples-testing branch June 11, 2021 18:12
plamut
plamut reviewed Jun 14, 2021
View reviewed changes
Copy link
Contributor

@plamut plamut left a comment

Choose a reason for hiding this comment

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

This check is very welcome, thanks for adding it!

I see that it has already been merged, but please double check if we gather the status code at the right place?

.kokoro/presubmit-against-pubsublite-samples.sh
deactivate py-3.6
rm -rf py-3.6/

EXIT=$?
Copy link
Contributor

@plamut plamut Jun 14, 2021

Choose a reason for hiding this comment

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

Maybe I'm missing something, but wouldn't this only check the status code of the last command, i.e. rm -rf py-3.6/ ? Shouldn't this be placed right after the python -m pytest quickstart_test.py line instead?

Copy link
Contributor

@plamut plamut Jun 15, 2021

Choose a reason for hiding this comment

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

cc: @anguillanneuf @busunkim96

Copy link
Contributor

@busunkim96 busunkim96 Jun 15, 2021

Choose a reason for hiding this comment

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

Ah that sounds right to me. @anguillanneuf Could you move up the EXIT=$? line to after python -m pytest ...?

Copy link
Contributor Author

@anguillanneuf anguillanneuf Jun 15, 2021

Choose a reason for hiding this comment

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

Thank you both for catching that. #427

@anguillanneuf anguillanneuf mentioned this pull request Jun 15, 2021
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@plamut plamut plamut left review comments

@busunkim96 busunkim96 busunkim96 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

api: pubsub Issues related to the googleapis/python-pubsub API. cla: yes This human has signed the Contributor License Agreement. samples Issues that are directly related to samples.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@anguillanneuf @busunkim96 @plamut @yoshi-kokoro