-
Notifications
You must be signed in to change notification settings - Fork 10.6k
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
Add paused method to PauseFailControl #6724
Conversation
(Python is green.) |
self._condition.notify_all() | ||
|
||
def paused(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Can this be named wait_for_pause?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about block_until_paused
?
Maybe just add a small comment that Control's can't be shared because it seems like that is a change in behavior. |
|
||
@contextlib.contextmanager | ||
def pause(self): | ||
"""Pauses code under control while controlling code is in context.""" | ||
with self._condition: | ||
self._paused = True | ||
self._pause = True | ||
yield | ||
with self._condition: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to make paused
be private, and call it after the yield
? Or are there possible cases where the control never gets paused but we want to unpause it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what you're asking here - read up on what @contextlib.contextmanager
does to this function?
(One of the reasons I don't like decorators in Python is that they often change code in radical ways.)
I think this object makes sense with callable public methods and non-callable private fields and I'm not at all seeing what a private method would do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After reading @contextlib.contextmanager
, what I said doesn't make sense...
What I intended is something along these lines:
@contextlib.contextmanager
with self._condition:
self._pause = True
self._block_until_paused()
yield
self._pause = False
self._condition.notify_all()
Then anytime we have
with control.pause()
#Do some stuff
We know that control is actually paused without needing to call block_until_paused
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be really bad because the way this object is used is
with control.pause():
<set in motion the behavior under test that would lead to the system under test being paused>
<either call control.block_until_paused or don't>
<maybe do some other stuff>
. With the semantics you suggest, that code would have to be changed to
<set in motion the behavior under test that would lead to the system under test being paused>
with control.pause():
<either call control.block_until_paused or don't>
<maybe do some other stuff>
and that would create a race condition in which the system under test could pass the control point (where it calls control.control()
) before the test system enters the control.pause()
context.
Concretely, look at the way this is used in expiration tests: we say that the service-side application should pause, then we invoke the RPC, then we chill out until we see the RPC time out, and then we leave the pause context and allow the service-side application to return from its RPC method handler. With the behavior you propose, the test would have to invoke the RPC before entering the pause context and the service-side application could finish servicing the RPC method before the test enters the pause context.
Right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. Thanks for the context.
This method will be used in tests that want to ensure before proceeding that the system under test has progressed to the control point.
91c88fc
to
6f61a13
Compare
Comment addressing what I think you meant about sharing added. Back over to you for more review. |
LGTM |
(Python is green.) |
This will be used in tests that want to ensure before proceeding that
the system under test has progressed to the control point.