Fail when the timing assertions get something they cannot measure - #3032
Merged
Conversation
Should-BeFasterThan and Should-BeSlowerThan handled [scriptblock] and [timespan], and both branches return. Anything else fell out of the bottom of the function, so the assertion returned having asserted nothing and the test passed. A string, a number, $null and an array all passed silently. That also made a CI flake unreadable. On Windows PS7 the test asserting that a 10ms sleep is slower than 1ms failed with "no assertion failure error was thrown", and the whole test took 3ms. The scriptblock was never run, which is why there was no sleep and no failure, but nothing said so, and it looked like a scriptblock that ran impossibly fast. The next time it happens the message names the type and value we were actually handed. 🤖
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Should-BeFasterThanandShould-BeSlowerThanhandle[scriptblock]and[timespan], and both branchesreturn. Anything else fell out of the bottom of the function, so the assertion returned having asserted nothing and the test passed:Now:
Why now
This is what has been making
Should-BeFasterThan.Throws when scriptblock is slower than expectedflake on Windows PS7. From the run on #3023:The test's scriptblock is
{ Start-Sleep -Milliseconds 10 }and the whole test took 3ms. That is wall clock on theIt, not aStopwatchreading, so no clock or timer-resolution problem can explain it. The sleep never ran.& $Actualwas never reached, which means$assert.Actual()returned something that is neither a scriptblock nor a timespan, and the function fell through the bottom and returned silently.So it was never a timing flake. It looked like one because a scriptblock that is never run is indistinguishable from a scriptblock that ran impossibly fast, when nothing reports the difference.
This does not fix the underlying cause, we still do not know why
Actual()returns the wrong thing there, and only on Windows PS7. It makes the next occurrence say what it actually got instead of looking like a stopwatch wobble, and it closes the silent-pass hole either way, which is worth doing on its own.For the record, ruled out locally on macOS:
Start-Sleep -Milliseconds 10never under-ran in 3000 measurements (min 10.07ms), and the assertion did not flake once in 2000 iterations.Verification
Should-BeFasterThan.Tests.ps120/20 andShould-BeSlowerThan.Tests.ps19/9, with a data-driven test on each covering a string, a number and$null. The scriptblock and timespan paths are unchanged.🤖