fix: restarted browsers not running tests #3233
Conversation
Currently whenever a browser disconnects completely (no socket.io connection loss), the launcher is instructed to "restart" the browser. Whenever the restarted browser now tries to "register" again, Karma considers the browser instance to be still executing and doesn't do anything about it (except setting the state to `EXECUTING` again). This means that the browser is in the state of executing, but practically it does nothing just waits. Resulting another disconnect (repeat here).
|
Note: The NodeJS v6 failures are unrelated to this pull request. |
devversion
force-pushed
the
fix/karma-browser-disconnect-restart
branch
from
afc010e
to
3f73b6d
Compare
devversion
force-pushed
the
fix/karma-browser-disconnect-restart
branch
from
3f73b6d
to
83f2493
Compare
| newBrowser.reconnect(socket) | ||
|
|
||
| // We are restarting a previously disconnected browser. | ||
| if (newBrowser.state === Browser.STATE_DISCONNECTED && config.singleRun) { |
There was a problem hiding this comment.
What it a bug that this was using DISCONNECTED before, or is this a change in behavior?
There was a problem hiding this comment.
Yeah that was a bug. The reconnect function always sets the state to CONNECTED if the state was STATE_DISCONNECTED. So it would never pass this check.
| // execution still continues because just the socket connection has been terminated. Now | ||
| // since we know whether this is just a socket reconnect or full client reconnect, we | ||
| // need to update the browser state accordingly. This is necessary because in case a | ||
| // browser crashed and has been restarted, we need to start with a fresh execution. |
There was a problem hiding this comment.
These comments are important but seem long. How about placing the comment inside the if and something like // Browser crashed and restarted.
(I really dislike the state-setting from outside design).
There was a problem hiding this comment.
I honestly don't think that it's a concern that this comment is long. Since this is basically a side-effect and we want to make clear why it happens, I think it's good to have this explained as detailed as possible. Especially since this seems to be a long-term and critical issue that can cause flaky test runs on CI's.
// Browser crashed and restarted is basically a comment like below where it says // We are restarting a previously disconnected browser. but basically misses crucial information.
Also I agree with the state setting from outside being a bit weird, but I'd rather set it here than passing stuff back into the Browser. As a matter of the event flow and the data we need from the client socket, it kind of makes sense to me to handle it here. I'm up other ideas on how we can do it.
Improve comments & log messages
|
@johnjbarton Addressed mostly all of your comments. Please have another look. |
|
Maybe v2.x of karma-sauce-launcher does not work on nodejs 6. I tried three re-runs but all fail. |
|
@johnjbarton Yeah, |
|
I think we should pin karma-runner 3.x to sauce-launcher 1.x first, then release last 3.x and move on to 4.x without node 6 This PR can do in after the first step if it passes with sauce-launcher 1.x |
|
@johnjbarton Thanks for restarting. And the plan sounds good! |
Currently whenever a browser disconnects completely (no socket.io connection loss), the launcher is instructed to
"restart"the browser.Whenever the restarted browser now tries to
"register"again, Karma considers the browser instance to be still executing and doesn't do anything about it (except setting the state fromEXECUTING_DISCONNECTEDtoEXECUTINGagain).This means that the browser is in the state of executing, but practically it does nothing and just waits.
Resulting in another disconnect (story repeats here)