core: if the socket disconnects, reconnect with browser-login event with the new socket

[step2yeung]

core: if the socket disconnects, reconnect with browser-login event with the new socket

Scenario:
Sometimes, socketio disconnects. This emits a disconnect event, causing testem to call onDisconnect.
onDisconnect sets up a timeout with the value from browser_disconnect_timeout, which will always expire and report an Browser timeout exceeded: <x>s error. The disconnect timeout doesn't seem useful, since nothing is done in between.

Change:
Reconnect the browser socket. This improves the resilience from socket disconnecting.
eg. Help with issues like #1021

[step2yeung]

@stefanpenner @johanneswuerbach would like to get your thoughts.

[stefanpenner]

@step2yeung seems too good to be true, whats the catch?

[step2yeung]

@stefanpenner Great question.

Let me add a bit more context in what I believe this code change does:
When the socket disconnects, socketio by default auto reconnect. So from socketio side, it makes sure it does it's part. Testem currently ignores this reconnect.
By emitting the browser-login event during the reconnect, the browserTestRunner that represents the browser will setup the socket communication using the new socket.

One catch could be: disconnect has historically meant the browser is gone and cannot be communicated with anymore.
Now, disconnected is only a transient state, and connected state will return once reconnection is successful. This opens up the door for reconnection loop if the socket connection continues to drop.
To battle this, added browser_reconnect_limit that only allow x number of reconnections.

Sidenote: Additional anecdotical evidence for this fix. For several ember test suite that failed for Browser timeout exceeded where the socket disconnected, this change allowed the full test suite to run to completion.

[stefanpenner]

@step2yeung thank you for the explanation, that makes sense. Thank you also for adding the reconnect limit, that we we don't just get stuck reconnecting forever if something is seriously wrong.

This basically sounds to me more like testem bugfix then anything else, is that the right way for me to think about it?

Are there next steps for this?

[step2yeung]

Yes, i believe that is a good way to look at this change. Where the bug is testem ignoring reconnects.
I do not see any next steps following this change.

[stefanpenner]

@step2yeung should we release?

The appveyor test does need to be fixed, but it seems unrelated to this PR..

[step2yeung]

@stefanpenner yes please! This should be release worthy.

Got #1335 started for the appveyor tests fixes

[raido]

I wonder if it helps with this: #1021 (comment) ?

[step2yeung]

@raido I don't think this change helps with your issue. This helps fixes with socket disconnecting during the test execution.

If I understand your issue correctly, it looks like startup of tests does not even happen. So you should be getting Browser failed to connect within 30s, which means the testem server did not receive an event that tells it the browser is ready to run tests.

[raido]

You are correct. I get browser initial connection timeout. I have it set at 120s instead of default 30s but no help.

[stefanpenner]

released as v2.16.0 🎉

[raido]

How will it get into ember apps? Through internal dependency bump?

[step2yeung]

@raido ember-cli depends on testem, so this will come with ember-cli
Its also possible to use the package.json resolution to point to the latest version

[raido]

I simply updated version in lock file.

[stefanpenner]

re-rolling the lockfile should be sufficient.