consolidate saucelabs tests to address flake #3019
Conversation
|
I don't know how to actually test for the existence of flake except to merge this and see if it works consistently. |
|
tentatively stuffing into v4, but reserve the right to pull out if it becomes a morass. I'll keep rebasing this. |
boneskull
force-pushed
the
issue/2890
branch
4 times, most recently
from
367b190
to
74418cb
Compare
|
Changes Unknown when pulling 367b190 on issue/2890 into ** on master**. |
boneskull
force-pushed
the
issue/2890
branch
from
74418cb
to
918d78a
Compare
|
Changes Unknown when pulling 918d78a on issue/2890 into ** on master**. |
That's pretty much where I'm at too. No idea what causes it, just that it always fits a certain pattern (specific browsers and hanging after one of two particular tests) when it does happen. Some of my investigation suggested a similar-looking issue could be caused by something going wrong in Mocha leading to a hanging test run after a suppressed internal error having to do with stringification and circular references, but that doesn't necessarily mean that that issue I discovered is the cause of the CI flake -- they could be unrelated problems that would just happen to look similar in their results. Keeping the Sauce tunnel open seems like a good idea anyway, so it's definitely worth a shot. Probably the only caveat I have about fiddling with how we run browser tests is that right now it's easy to see in the Travis jobs list which browser failed, but if we merged it into one Travis job we'd have to dig through pages of Karma output to find any errors. (This goes not just for flakes like this but for any test failure in a browser run.) If for some reason we need to do that, it isn't unmanageable; it's just the only potential con I can think of to any of the options we have. On the other hand, besides potentially fixing the flake, this looks like it makes the CI run much, much faster. Tradeoffs... |
test/node-unit/http.spec.js
Outdated
| it('should provide an example', function (done) { | ||
| http.get({ path: '/', port: 8888 }, function (res) { | ||
| http.get({ path: '/', port: port }, function (res) { |
There was a problem hiding this comment.
Just my two cents' worth, but I don't think we should be adding dependencies to tweak this test. It doesn't seem to be a real unit test of Mocha to begin with -- probably more like an example use case. If it's causing trouble we should stop running it and (if we want to keep it as an example) move it into documentation somewhere instead.
There was a problem hiding this comment.
Yeah, I didn't really stop to wonder why this test was running at all... it's just always been there.
|
Changes Unknown when pulling ffd5e80 on issue/2890 into ** on master**. |
|
@ScottFreeCode yeah, it does give better visibility into which browser failed. that being said, I'm fully behind trying to get Jenkins running, since it'll give us more control (and information). I have a droplet; just need to configure the thing. |
|
so far the only failures I've seen have not been of the "couldn't start a browser" variety, and I've ran quite a few builds today. |
- remove some "unit" tests of questionable value - linting of `karma.conf.js`
boneskull
force-pushed
the
issue/2890
branch
from
5fc9c86
to
8aa0e34
Compare
|
Changes Unknown when pulling a67bb6c on issue/2890 into ** on master**. |
|
@ScottFreeCode anyhow, this LGTM. I'll merge it tomorrow w/ your blessing |
- run browser tests concurrently to avoid SauceLabs flake - remove some "unit" tests of questionable value - linting of `karma.conf.js`
may or may not fix #2890.