#11679 LoopingCall vs Clock tests #11680
Conversation
clock.advance(0) will run not only any calls scheduled for clock.seconds() or earlier, but it will also run any calls that _those_ calls schedule with `clock.callLater(0, ...)`. This diverges from the real IReactorTime implementations which force all scheduled events to wait at least one reactor iteration before they can run. This impacts the LoopingCall test cases for the case of interval == 0 because those tests do a lot of scheduling with `clock.callLater(0, ...)`. In particular, the tests often `advance(0)` and expect a `LoopingCall` to execute multiple iterations. These changes make the tests advance the clock in enough discrete steps that a real reactor would have time to run all of the necessary calls. After twisted#5962 the old version of these tests will fail. Also as part of twisted#5962 one clearly-marked fudge left in these tests will need to be removed because I don't see how to make the test work (and remain precise) with both the broken Clock behavior and the real reactor behavior.
There was a problem hiding this comment.
Changes looks good.
But I am not sure what is the point of this?
I guess that once Clock is fixed, we can have the tests refactored in the same PR.
My understanding is that these tests are wrong.
They work with Clock, but would never work with a real reactor,
Also, once Clock is fixed, we might want to rewrite the tests to explictly define the loop behaviour for interval=0
Anyway. If this makes future refactoring easy, I don't see any harm in merging this.
Thanks again!
Looking forward to the Clock refactoring.
I wasn't aware of this detail, and I think I was bitten / smashehd by this beahviour ... wondering why the unit test code is different to what I observe with a functional / end to end test.
| @@ -506,10 +506,10 @@ def foo(cnt): | |||
| loop.clock = clock | |||
| deferred = loop.start(0, now=False) | |||
|
|
|||
| clock.advance(0) | |||
| clock.pump([0] * 5) | |||
There was a problem hiding this comment.
I guess we can also have the test like this
And my understanding is that this is is how the tests will be refactored after the Clock is fixed.
| clock.pump([0] * 5) | |
| # Iterating the reactor will only cause the loop to loop once. | |
| # even if the loop is configured to loop without a delay. | |
| clock.advance(0) | |
| self.assertEqual([1], accumulator) |
There was a problem hiding this comment.
Because of the assertion on line 512 (accumulator == [1] * 5) the clock/reactor will still need to iterate 5 times. It is the old, broken behavior that makes clock.advance(0) run the LoopingCall 5 times.
| clock.advance(0) | ||
| self.assertEqual([1, 1, 1, 1, 1], accumulator) | ||
| clock.pump([0] * 4) | ||
| self.assertEqual([1] * 5, accumulator) |
There was a problem hiding this comment.
Just asking.
Is there any advantage in this way of writing the test?
I prefer to have the assertions as "dumb" as possible, to reduce the risk of errors...to the extend of having more repetition.
There was a problem hiding this comment.
I think you're asking about [1, 1, 1, 1, 1] vs [1] * 5? I think I find [1] * 5 to be the less risky version. My eyes slide over [1, 1, 1, 1, 1] very easily but the exact number of 1s there is absolutely critical to the correctness of the test.
Probably there is some other pattern altogether that is better than either of these, keeping things simpler than list multiplication but also making it easy to read without making a mistake. I didn't think very hard about finding such a pattern while working on this.
Thanks for looking. Sorry I wasn't more clear about the purpose. This is a somewhat weird PR.
I believe that except for https://github.com/twisted/twisted/pull/11680/files#diff-b3f880003e83657375b9dd7fedfc131fd6208ce3de9b339337853b457e55c305R592 the tests are now correct with respect to both Clock and real reactors. In an upcoming PR where I fix Clock, I'll just delete that line. The changes would probably have made more sense as part of the PR that fixes Clock but I didn't expect that to be a small PR so I wanted to try to get as much of it landed separately as possible. In retrospect, I probably could have done all the hard work of fixing things but left Clock broken, landed that PR, then done the last little bit to apply the fix to Clock and land that change along with all of these test fixes. Oh well...
This is correct - that is, in current trunk@HEAD, they are wrong and don't work with a real reactor. In this branch I think they are now mostly right.
Pooossibly. The tests definitely already define something about that behavior but they're not very systematic about it. It would probably be good to rewrite them so they cover all of the cases in some logical, coherent fashion. I'm not sure if I'll commit to taking on that work though. :)
It does, thanks!
Coming up soon :)
Indeed! I had forgotten about it too until these tests hit me in the face... Thanks again. |
Co-authored-by: Adi Roiban <adiroiban@gmail.com>
Scope and purpose
Fixes #11679
From the commit message: