Error in CI with HttpStream/FakeReportServer #1617
Conversation
aurelien-reeves
changed the title
|
@aurelien-reeves is this still a live issue? |
|
Yes, the issue is still live We may close that one until it raises often again |
|
Ha! Here's one right now: https://github.com/cucumber/cucumber-js/runs/5131120062?check_suite_focus=true |
This hopefully resolves the intermittend failures we've had in CI. Ref #1617
It looks like the problem occurs when the FakeReportServer starts up - sometimes although it says it's ready and listening, the server isn't really available, however long you wait for it. We'll need to clean up after this commit, go back and make a more minimal change, but I wanted to check it in for the record. Here's what I tried: - added HttpTerminator for managing the server shutdown (in case it wasn't shutting down cleanly) - added waitOn to poll for the server being up after calling `start()` - keep app in an instance variable - use the same port for each server instance In the end, it looks like the problem was caused by letting the http server choose its own port (by passing in 0 for the port). Sometimes this just doesn't seem to give us a working server. If we hard-code a fixed port, at least while we're also using `waitOn`, it seems to work consistently. I'm going to revert this commit next and try to pare down these changes to isolate the minimal change to fix the problem.
This reverts commit e056300.
This hopefully resolves the intermittend failures we've had in CI. Ref #1617
No need for all that socket management stuff
This reverts commit 03a5d89. Turns out it was being used after all!
mattwynne
force-pushed
the
http-stream-tests-race-condition
branch
from
70665af to
9467a1f
Compare
|
OK I think we have identified the culprit and have a fix. Pairing with @aurelien-reeves we isolated the problem down to starting the On a hunch, I tried using a fixed port instead of this trick of passing a In order to avoid hard-coding a port, I've used a library called Once I had it working I dismantled some complexity in the |
|
One thing that occurs to me is that we could move all of the cucumber-reports integration stuff out into some kind of plugin. It might be a useful exercise to create the extension points, and it would also mean we could leave these tests (and code) out of our main codebase / CI run. |
This reverts commit 5c9599a. I'd like to see this run once on the pull request before we remove it for good
|
Nice!!!! |
This reverts commit 59782d4. We don't want these tests in the main branch, and I don't think we need them anymore now we've fixed the race condition.
|
Well, I guess I spoke too soon! https://github.com/cucumber/cucumber-js/runs/5310937749?check_suite_focus=true Maybe we need a new test to isolate this issue. That previous one definitely seems to be passing OK now. |
|
Actually that "ERR_PREMATURE_CLOSE" error is the one we originally had and tried to reproduce and fix. |
Yeah, it's the same one I witnessed here, too. Interesting that it happened on Windows both times. I wonder if that's relevant. |
|
I would like us to still merge the changes in this PR as they're an improvement, even if they don't fix all the flickers in this area of the test suite. |
Tests are not stable enough on the CI
It seems to be due to a race condition around http_stream and/or the fakeReportServer used for the tests.