test: refactor concurrency test using orchestrate by chrisrossi · Pull Request #709 · googleapis/python-ndb

chrisrossi · 2021-08-16T18:56:18Z

Towards googleapis/google-cloud-python#15862

Towards #691

tseaver · 2021-08-16T19:38:55Z

 log = logging.getLogger(__name__)


+def _syncpoint_692():


That name is super puzzling -- can you provide an explanation, or a link to an issue, in the comment? Even, better, give it a name that indicates where it is used, e.g., _syncpoint_update_key, or something similar.

I see the link in the testcase below to #692: seems like it would be good to repeat here. Also, I wonder about wrapping it in something like if __debug__, to avoid the overhead of even an empty python function call. Of course, that would require geting __debug__ set during the test runs (IIRC it is set by default, and only switched off with -O on the command line, which would also disable assert statments).

5ec4bda

Poking around, it looks like you're correct about when __debug__ is True. I'm not sure how common it is in practice to pass -O in production. I know I've never made a habit of it.

Another solution I thought about was getting rid of the no-op function and using something like # pragma: SYNCPOINT syncname to declare syncpoints, then do some coverage style postprocessing to insert the calls during testing. If you think the current implementation is mind-bending, though...

It's almost certainly not even something worth considering, unless orchestrate gets some kind of life outside of just NDB.

Why not build this on sys.settrace? Then you wouldn't have to modify the code under tests or cause extra calls and associated overhead.

You could pass in breakpoints for each functions as file-line locations to yield.

Well, I didn't know about it, for starters. I can take a peek. On first blush, it seems like using line numbers is pretty fragile. I'd want a more robust way to define the syncpoint/breakpoint.

Looks like I could probably use this to implement the # pragma: SYNCPOINT idea.

I actually have something like this working now. Will try and make it presentable tomorrow.

An interesting side-effect of using sys.settrace is it breaks coverage, since it also uses sys.settrace. ;-)

tseaver · 2021-08-16T19:58:55Z

+    tests can help with this.
+
+    As soon as any error or failure is detected, no more scenarios are run
+    and that error is propagated to the main thread.


I really appreciate the extensive writeup here of the rationale. Even with its help, following the implementation below is more than a bit brain-bending.

Sorry. I was trying to be sensitive to the fact that it would be difficult to follow without a lot of explanation.

I wonder if using the same test function twice contributes to the bendy-ness: maybe using a simple test (no syncpoints) along with the one you have already would clarify any? Maybe as an additional example?

tseaver · 2021-08-16T20:13:31Z

Is the test_insert_entity_in_transaction_without_preallocating_id known to be flaky? I don't see an issue for it.

chrisrossi · 2021-08-17T15:43:59Z

Is the test_insert_entity_in_transaction_without_preallocating_id known to be flaky? I don't see an issue for it.

I missed the Kokoro error, so I can't comment specifically on whatever you saw earlier. There is a little flakiness to lots of the system tests due to eventual consistency. tests.system.eventually is an attempt to paper over most of that, but sometimes something takes a really long time to become consistent and we get a failure.

tseaver · 2021-08-17T20:04:16Z

-    def advance(self):
-        """Allow a call to the wrapper to proceed.
+    if os.environ.get("REDIS_CACHE_URL"):
+        yield redis_cache


Coverage issue here.

Added "pragma: NO COVER". Don't want coverage to depend on which caches are available.

tseaver · 2021-08-17T20:05:23Z

-        """
-        self._futures.popleft().set_result(None)
+    def memcache_cache():
+        return global_cache_module.MemcacheCache.from_environment()


chrisrossi · 2021-08-23T22:54:22Z

+                if syncpoints:
+                    syncpoints.pop()
+                else:
+                    do_nothing()  # pragma: SYNCPOINT


This shows a limitation of the current settrace approach, in that the comment must be on a reachable line, so the call to do_nothing is just here to have something to hang the pragma off of. It would be more satisfying to be able to write something like:

if syncpoints: syncpoints.pop() else: # pragma: SYNCPOINT test_calls.append(name)

I spent some time trying to figure out how to make this possible and concluded it was complicated enough to leave out for the first pass. Unfortunately, neither tokenize nor ast, on their own, provide enough information to figure out which side of the else the pragma is on and assign to a line of code. It should be possible to use both to figure it out, but it's not trivial.

I think it's fine for the pragma to mark the syncpoint.

chrisrossi · 2021-08-23T22:57:08Z

+"""
+
+
+def _get_syncpoints(filename):


This is a very simplistic implementation that requires every pragma be located on a reachable line of code. This is probably sufficient. A more sophisticated algorithm that allowed pragmas to appear on their own lines is possible, but not trivial. See this comment.

tseaver

ISTM that orchestrate might find its home in python-testutils -- other code with tricksy concurrency (e.g., pubsub flow control, firestore watch, etc.) might find it useful.

plamut · 2021-08-25T08:21:52Z

ISTM that orchestrate might find its home in python-testutils -- other code with tricksy concurrency (e.g., pubsub flow control, firestore watch, etc.) might find it useful.

That's indeed a possibility.

For example, see one of the unit tests for publisher flow controller that tests a specific sequence of add()/release() operations on a shared thread-safe FlowController instance:
https://github.com/googleapis/python-pubsub/blob/4726a8f8f9aad8527c58371bbcb741398cda9b53/tests/unit/pubsub_v1/publisher/test_flow_controller.py#L204-L280

There are quite a few Event instances and Event.wait() calls to make sure those add()/release() operations are executed in a specific order that we want to check. With orchestrate it might be possible to simplify that test (and the related ones) quite a bit.

test: refactor concurrency test using orchestrate

9df9196

Towards #691

chrisrossi requested a review from jimfulton August 16, 2021 18:56

chrisrossi requested a review from andrewsg as a code owner August 16, 2021 18:56

chrisrossi requested review from a team August 16, 2021 18:56

google-cla Bot added the cla: yes This human has signed the Contributor License Agreement. label Aug 16, 2021

product-auto-label Bot added the api: datastore Issues related to the googleapis/python-ndb API. label Aug 16, 2021

tseaver reviewed Aug 16, 2021

View reviewed changes

Comment thread tests/conftest.py Outdated

Better name/locality for no-op function.

e9e33fe

chrisrossi force-pushed the test-orchestrate branch from 28dee77 to e9e33fe Compare August 17, 2021 14:11

Chris Rossi added 4 commits August 17, 2021 10:14

Only call syncpoint if __debug__ is True

5ec4bda

Python 2.7

5cd8e70

Assert something.

37c33f8

Test the 'real' caches, too, if present.

3525e04

tseaver reviewed Aug 17, 2021

View reviewed changes

Use settrace

19e207d

chrisrossi commented Aug 23, 2021

View reviewed changes

Merge branch 'master' into test-orchestrate

d500999

chrisrossi requested a review from tseaver August 24, 2021 17:01

tseaver approved these changes Aug 24, 2021

View reviewed changes

chrisrossi merged commit b27725e into googleapis:master Aug 24, 2021

chrisrossi deleted the test-orchestrate branch August 24, 2021 17:30

chrisrossi mentioned this pull request Aug 26, 2021

Port orchestrate from NDB googleapis/python-test-utils#53

Closed

Conversation

chrisrossi commented Aug 16, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tseaver Aug 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisrossi Aug 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tseaver commented Aug 16, 2021

Uh oh!

chrisrossi commented Aug 17, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tseaver left a comment

Choose a reason for hiding this comment

Uh oh!

plamut commented Aug 25, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tseaver Aug 16, 2021 •

edited

Loading

chrisrossi Aug 17, 2021 •

edited

Loading