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: restartOnFileChange option not restarting the test run #3727

Merged
merged 2 commits into from
Nov 16, 2021

Conversation

devoto13
Copy link
Collaborator

The problem was caused by a race condition in the client JS implementation. When the restartOnFileChange option is enabled, the server would send two commands to the browser: stop and execute. Both of these commands navigate the context. stop navigates to the blank page, while execute navigate to the context.html thus triggering a test run. The execute command would trigger the navigation synchronously, but the stop command would trigger it on the next event loop tick. As a result, if both commands arrive almost at the same time, their order is effectively reversed: first schedule new execution and then immediately stop it. The bug is resolved by handling both commands synchronously thus ensuring correct order.

The setTimeout() call in question was introduced in 15d80f4 to solve #27 and was retained over time. It's not completely clear why the timeout was added, but it does not seem to be relevant.

With clearContext: true, karma will reset the context and thus cancel any scheduled navigations as soon as the test framework has reported the complete event. As navigations are canceled they don't affect karma and thus karma does not care about them. It's true that the user may have a test with race conditions (e.g. missing done callback), but it is up to them to investigate and fix it.

With clearContext: false, the same logic applies, that karma does not care about navigations after complete event as long as they don't break karma itself. Here it gets a bit tricky:

  • Navigation within the same origin is undesired, but it doesn't break karma and the next execution can proceed normally.
  • Navigation to a different origin however is a problem as after such navigation the iframe becomes cross-origin and karma client will not be able to navigate it to the context.html or interact with it at all for that matter until the page reload (see this SO answer). This will break the watch mode, but IMO we can let it be given that the case is quite uncommon, the user will see the browser window displaying something unexpected and the error in the CLI output. The changes in this commit don't make this case any worse and it does not seem to be possible to prevent such problematic navigations with the current state of browsers per https://developer.mozilla.org/en-US/docs/Web/API/WindowEventHandlers/onbeforeunload:

To combat unwanted pop-ups, some browsers don't display prompts created in beforeunload event handlers unless the page has been interacted with. Moreover, some don't display them at all.

Fixes #3724

The problem was caused by a race condition in the client JS implementation. When the `restartOnFileChange` option is enabled, the server would send [two commands](https://github.com/karma-runner/karma/blob/b153355de7e05559d877a625c9b0c5d23a3548bd/lib/server.js#L377) to the browser: `stop` and `execute`. Both of these commands navigate the context. `stop` navigates to the blank page, while `execute` navigate to the `context.html` thus triggering a test run. The `execute` command would trigger the navigation synchronously, but the `stop` command would trigger it on the next event loop tick. As a result, if both commands arrive almost at the same time, their order is effectively reversed: first schedule new execution and then immediately stop it. The bug is resolved by handling both commands synchronously thus ensuring correct order.

The setTimeout() call in question was introduced in 15d80f4 to solve karma-runner#27 and was retained over time. It's not completely clear why the timeout was added, but it does not seem to be relevant.

With `clearContext: true`, karma will reset the context and thus cancel any scheduled navigations as soon as the test framework has reported the `complete` event. As navigations are canceled they don't affect karma and thus karma does not care about them. It's true that the user may have a test with race conditions (e.g. missing `done` callback), but it is up to them to investigate and fix it.

With `clearContext: false`, the same logic applies, that karma does not care about navigations after `complete` event as long as they don't break karma itself. Here it gets a bit tricky:

- Navigation within the same origin is undesired, but it doesn't break karma and the next execution can proceed normally.
- Navigation to a different origin however is a problem as after such navigation the iframe becomes cross-origin and karma client will not be able to navigate it to the `context.html` or interact with it at all for that matter until the page reload (see [this SO answer](https://stackoverflow.com/q/25098021/1377864)). This will break the watch mode, but IMO we can let it be given that the case is quite uncommon, the user will see the browser window displaying something unexpected and the error in the CLI output. The changes in this commit don't make this case any worse and it does not seem to be possible to prevent such problematic navigations with the current state of browsers per https://developer.mozilla.org/en-US/docs/Web/API/WindowEventHandlers/onbeforeunload:

> To combat unwanted pop-ups, some browsers don't display prompts created in beforeunload event handlers unless the page has been interacted with. Moreover, some don't display them at all.

Fixes karma-runner#3724
@devoto13 devoto13 requested a review from jginsburgn November 13, 2021 22:47
@jginsburgn
Copy link
Member

I have started qualifying this :)

@jginsburgn jginsburgn self-assigned this Nov 16, 2021
@jginsburgn jginsburgn merged commit cf318e5 into karma-runner:master Nov 16, 2021
@karmarunnerbot
Copy link
Member

🎉 This PR is included in version 6.3.9 🎉

The release is available on:

Your semantic-release bot 📦🚀

@devoto13 devoto13 deleted the fix-restart-on-file-change branch November 23, 2021 20:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

restartOnFileChange option is broken (again)
3 participants