Make unit tests less flaky on Travis CI #1329
Conversation
| // Don't allow time to advance in this test. Otherwise it's possible for | ||
| // the animation to progress between setup and the assertion, causing flaky | ||
| // failures. | ||
| this.sandbox.useFakeTimers(); |
There was a problem hiding this comment.
On a separate branch I added a bunch of logging in this test, and I found that when it fails, the animation's progress value has advanced slightly from 0.9 between calling this.animation.enqueue(segment) and capturing the return value of this.animation.calculateProgress(this.animation.playLoop.calledAt). For example, this run reports progress of 0.902.
I eventually realized that this test assumes no significant time will pass in that space - that's why we're manually starting the progress at 0.9 - but on very slow machines that might not be true. The solution is to use sinon's fake clocks to effectively stop time during this test. The fake timers are implicitly cleaned up by the sandbox when the test is done.
| @@ -260,7 +263,8 @@ exports["Animation -- Servo"] = { | |||
| var indices = this.animation.findIndices(progress); | |||
| var val = this.animation.tweenedValue(indices, progress); | |||
|
|
|||
| test.ok(Math.abs(val - 74.843 < 0.01)); | |||
| test.ok(Math.abs(val - 74.843) < 0.01, | |||
| "Expected " + val + " to be within 0.01 of 74.843"); | |||
There was a problem hiding this comment.
I'm surprised that the original assertion here ever worked, due to the misplaced paren; turns out
Math.abs(true) === 1;
Math.abs(false) === 0;I figured it wouldn't hurt to leave a nicer assertion behind. 😁
| if (!calledTestDone) { | ||
| calledTestDone = true; | ||
| test.done(); | ||
| } |
There was a problem hiding this comment.
This was a little harder to track down, but my first clue was this issue on the nodeunit repo, followed by this issue that is still open and summarizes the problem well; nodeunit raises this really opaque error if you happen to call test.done() twice in a single test.
I scoured rgb.collection.js for places where this might happen but eventually noticed that it always failed right at the top of that file... So I checked the test file before it, and sure enough, there's some tricky and system-dependent callback stuff going on here. I did some more extra logging on a separate branch and discovered that the "reallyExit" stub was getting called twice in failure cases (examples here and here). I'm not sure how - that doesn't seem possible, since we restore the stub when it's called - but I figured a second guard against calling test.done() twice might be in order.
If anyone has a better idea of how the reallyExit stub is getting called a second time, I'm all ears. Maybe something in the previous test?
islemaster
changed the title
islemaster
force-pushed
the
stabilize-tests
branch
from
e06763c to
d7a8efc
Compare
|
👏 Nice! |
|
Thank you so much @islemaster !!! There are not enough emojis and smileys in the world to convey how happy (and relieved) this makes me. |
I believe I've identified and fixed the two top causes of flaky failures in the Travis CI builds. I've provided inline comments for each fix.
Existing failure rate
For a baseline, I looked at the last five builds on
master: 2574, 2573, 2569, 2568, and 2567.Animation -- Servo - keyframeEasingCannot read property 'setUp' of undefinedI've seen both of these on my own pull requests recently too - they seem to be fairly common.
New failure rate
Considering builds: 2578 (PR), 2579 (PR), 2580 (PR), 41, 42, and 43.