Skip to content

Conversation

@tmatsuo
Copy link
Contributor

@tmatsuo tmatsuo commented Apr 3, 2020

fixes #2945
fixes #2818

There were 8 tests which has time.sleep(10) for waiting data
propagation or whatever. I chose to use @eventually_consistent.call
instead of sleep in those tests.

Bonus point is now the tests are significantly faster because it doesn't
always wait for 10 seconds.

The downside is it will take a long time on failure. We may want to
specify the retry count on the eventualy consistent decorator once the
following issue is fixed:

GoogleCloudPlatform/python-repo-tools#25

@tmatsuo tmatsuo requested review from gguuss and leahecole April 3, 2020 20:21
@tmatsuo tmatsuo requested a review from a team as a code owner April 3, 2020 20:21
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Apr 3, 2020
Takashi Matsuo added 2 commits April 3, 2020 13:26
fixes GoogleCloudPlatform#2945
fixes GoogleCloudPlatform#2818

There were 8 tests which has `time.sleep(10)` for waiting data
propagation or whatever. I chose to use `@eventually_consistent.call`
instead of sleep in those tests.

Bonus point is now the tests are significantly faster because it doesn't
always wait for 10 seconds.

The downside is it will take a long time on failure. We may want to
specify the retry count on the eventualy consistent decorator once the
following issue is fixed:

GoogleCloudPlatform/python-repo-tools#25
@@ -1,3 +1,4 @@
pytest==5.3.2
gcp-devrel-py-tools==0.0.15
google-cloud-core
Copy link
Collaborator

Choose a reason for hiding this comment

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

How is google-cloud-core being used? I'm not seeing it anywhere. I could easily be missing something though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is used by gcp-devrel-py-tools, which has a broken dependency.
See: GoogleCloudPlatform/python-repo-tools#24

I'll fix them with upcoming PR.

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.

Only thing that jumped out at me was the use of gcp_devrel tools, I believe the python-samples-owners team is working to remove this library from our tests.

def test_commute_search_sample(capsys):
import commute_search_sample
import re
from gcp_devrel.testing import eventually_consistent
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe we're discouraging use of gcp_devrel and instead preferring flaky or retry. Is there an advantage to eventually_consistent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

eventually_consistent is a thin wrapper around retrying, specialized to eventually consistent stuff. I'm not sticking to use gcp_devrel, but the code is more readable and descriptive. I'm more than happy to maintain that library, so there's no worry of it being not maintained.

If you strongly feel we should use retrying, I can do that though.

@tmatsuo
Copy link
Contributor Author

tmatsuo commented Apr 7, 2020

Thanks! When we decide to use retrying, I'll make a change, but for now I'm just submitting this.

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

5 participants