Skip to content
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

#11679 LoopingCall vs Clock tests #11680

Merged
merged 4 commits into from Sep 22, 2022
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Empty file.
78 changes: 57 additions & 21 deletions src/twisted/test/test_task.py
Expand Up @@ -491,8 +491,8 @@ def test_withCountFloatingPointBoundary(self):

def test_withCountIntervalZero(self):
"""
L{task.LoopingCall.withCount} with interval set to 0
will call the countCallable 1.
L{task.LoopingCall.withCount} with interval set to 0 calls the
countCallable with a count of 1.
"""
clock = task.Clock()
accumulator = []
Expand All @@ -506,10 +506,10 @@ def foo(cnt):
loop.clock = clock
deferred = loop.start(0, now=False)

clock.advance(0)
clock.pump([0] * 5)
exarkun marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

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.

Suggested change
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)

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

self.successResultOf(deferred)

self.assertEqual([1, 1, 1, 1, 1], accumulator)
self.assertEqual([1] * 5, accumulator)

def test_withCountIntervalZeroDelay(self):
"""
Expand All @@ -534,20 +534,20 @@ def foo(cnt):
loop.clock = clock
loop.start(0, now=False)

clock.advance(0)
clock.pump([0] * 2)
# Loop will block at the third call.
self.assertEqual([1, 1], accumulator)
self.assertEqual([1] * 2, accumulator)

# Even if more time pass, the loops is not
# advanced.
clock.advance(2)
self.assertEqual([1, 1], accumulator)
clock.pump([1] * 2)
self.assertEqual([1] * 2, accumulator)

# Once the waiting call got a result the loop continues without
# observing any delay in countCallable.
deferred.callback(None)
clock.advance(0)
self.assertEqual([1, 1, 1, 1, 1], accumulator)
clock.pump([0] * 4)
self.assertEqual([1] * 5, accumulator)
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.


def test_withCountIntervalZeroDelayThenNonZeroInterval(self):
"""
Expand All @@ -558,33 +558,69 @@ def test_withCountIntervalZeroDelayThenNonZeroInterval(self):
deferred = defer.Deferred()
accumulator = []

# The amount of time to let pass (the number of 1 second steps to
# take) before the looping function returns an unfired Deferred.
stepsBeforeDelay = 2

# The amount of time to let pass (the number of 1 second steps to
# take) after the looping function returns an unfired Deferred before
# fiddling with the loop interval.
extraTimeAfterDelay = 5

# The new value to set for the loop interval when fiddling with it.
mutatedLoopInterval = 2

# The amount of time to let pass (in one jump) after fiddling with the
# loop interval.
durationOfDelay = 9

# This is the amount of time that passed between the
# Deferred-returning call of the looping function and the next time it
# gets a chance to run.
skippedTime = extraTimeAfterDelay + durationOfDelay

# This is the number of calls that would have been made to the
# function in that amount of time if the unfired Deferred hadn't been
# preventing calls and if the clock hadn't made a large jump after the
# Deferred fired.
expectedSkipCount = skippedTime // mutatedLoopInterval

# Because of #5962 LoopingCall sees an unrealistic time for the second
# call (it seems 1.0 but on a real reactor it will see 2.0) which
# causes it to calculate the skip count incorrectly. Fudge our
# expectation here until #5962 is fixed.
expectedSkipCount += 1

def foo(cnt):
accumulator.append(cnt)
if len(accumulator) == 2:
if len(accumulator) == stepsBeforeDelay:
return deferred

loop = task.LoopingCall.withCount(foo)
loop.clock = clock
loop.start(0, now=False)

# Even if a lot of time pass, loop will block at the third call.
clock.advance(10)
self.assertEqual([1, 1], accumulator)
# Even if a lot of time passes the loop will stop iterating once the
# Deferred is returned. 1 * stepsBeforeDelay is enough time to get to
# the Deferred result. The extraTimeAfterDelay shows us it isn't
# still iterating afterwards.
clock.pump([1] * (stepsBeforeDelay + extraTimeAfterDelay))
self.assertEqual([1] * stepsBeforeDelay, accumulator)

# When a new interval is set, once the waiting call got a result the
# loop continues with the new interval.
loop.interval = 2
loop.interval = mutatedLoopInterval
deferred.callback(None)

clock.advance(durationOfDelay)
# It will count skipped steps since the last loop call.
clock.advance(7)
self.assertEqual([1, 1, 3], accumulator)
self.assertEqual([1, 1, expectedSkipCount], accumulator)

clock.advance(2)
self.assertEqual([1, 1, 3, 1], accumulator)
clock.advance(1 * mutatedLoopInterval)
self.assertEqual([1, 1, expectedSkipCount, 1], accumulator)

clock.advance(4)
self.assertEqual([1, 1, 3, 1, 2], accumulator)
clock.advance(2 * mutatedLoopInterval)
self.assertEqual([1, 1, expectedSkipCount, 1, 2], accumulator)

def testBasicFunction(self):
# Arrange to have time advanced enough so that our function is
Expand Down