Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: restarted browsers not running tests #3233

Conversation

devversion
Copy link
Contributor

@devversion devversion commented Dec 14, 2018

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 from EXECUTING_DISCONNECTED to EXECUTING again).

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)

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).
@devversion
Copy link
Contributor Author

Note: The NodeJS v6 failures are unrelated to this pull request.

@devversion devversion force-pushed the fix/karma-browser-disconnect-restart branch from afc010e to 3f73b6d Compare December 14, 2018 19:00
@devversion devversion force-pushed the fix/karma-browser-disconnect-restart branch from 3f73b6d to 83f2493 Compare December 14, 2018 19:27
client/karma.js Outdated Show resolved Hide resolved
test/client/karma.spec.js Outdated Show resolved Hide resolved
newBrowser.reconnect(socket)

// We are restarting a previously disconnected browser.
if (newBrowser.state === Browser.STATE_DISCONNECTED && config.singleRun) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What it a bug that this was using DISCONNECTED before, or is this a change in behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

See the reconnect function source

Copy link

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

lib/server.js Outdated Show resolved Hide resolved
Copy link
Contributor

@johnjbarton johnjbarton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some suggestions for the comments.

// 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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

@devversion devversion Dec 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

lib/browser.js Outdated Show resolved Hide resolved
lib/browser.js Outdated Show resolved Hide resolved
lib/browser.js Outdated Show resolved Hide resolved
@devversion
Copy link
Contributor Author

@johnjbarton Addressed mostly all of your comments. Please have another look.

@johnjbarton
Copy link
Contributor

Maybe v2.x of karma-sauce-launcher does not work on nodejs 6. I tried three re-runs but all fail.

@devversion
Copy link
Contributor Author

@johnjbarton Yeah, selenium-webdriver which is now a dependency of the Saucelabs launcher, does not support NodeJS v6. See: https://unpkg.com/[email protected]/package.json

@johnjbarton
Copy link
Contributor

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

@devversion
Copy link
Contributor Author

@johnjbarton Thanks for restarting. And the plan sounds good!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants