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.

Sign up for GitHub

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

Jump to bottom

restartOnFileChange option is broken (again) #3724

Closed
scottohara opened this issue Nov 3, 2021 · 2 comments · Fixed by #3727
Closed

restartOnFileChange option is broken (again) #3724

scottohara opened this issue Nov 3, 2021 · 2 comments · Fixed by #3727
Labels
released

Comments

@scottohara
Copy link

(See similar issue from 2015: #1640)

Expected behaviour

✅ When restartOnFileChange: false, after a test run completes, saving a test file re-runs the test(s)
✅ When restartOnFileChange: true, after a test run completes, saving a test file re-runs the test(s)

Here I saved test.js three times, and it ran four times (initial run, plus 3 x saves):

expected

Actual behaviour

✅ When restartOnFileChange: false, after a test run completes, saving a test file re-runs the test(s)
🚫 When restartOnFileChange: true, after a test run completes, saving a test file does not re-run the test(s)

Here I saved test.js four times, and it ran once (initial run only):

actual

Steps to reproduce

I have created a trivial reproduction of the issue here:
https://github.com/scottohara/karma-restartonfilechange-bug
@devoto13
Copy link
Collaborator

devoto13 commented Nov 3, 2021

Thanks for the report. I can reproduce it, but not every time. It looks like it is caused by the race condition between the stop and execute signals sent to the browser. I'll look into it more.

@scottohara scottohara mentioned this issue Nov 7, 2021
Closed
devoto13 added a commit to devoto13/karma that referenced this issue Nov 13, 2021
@devoto13
fix: restartOnFileChange option not restarting the test run
e913150 
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 mentioned this issue Nov 13, 2021
Merged
jginsburgn pushed a commit that referenced this issue Nov 16, 2021
@devoto13
fix: restartOnFileChange option not restarting the test run
92ffe60 
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 #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 #3724
karmarunnerbot pushed a commit that referenced this issue Nov 16, 2021
@semantic-release-bot
chore(release): 6.3.9 [skip ci]
b8eafe9 
## [6.3.9](v6.3.8...v6.3.9) (2021-11-16)

### Bug Fixes

* restartOnFileChange option not restarting the test run ([92ffe60](92ffe60)), closes [#27](#27) [#3724](#3724)
@karmarunnerbot
Copy link
Member

🎉 This issue has been resolved in version 6.3.9 🎉

The release is available on:

Your semantic-release bot 📦🚀

@sync-by-unito sync-by-unito bot mentioned this issue Dec 1, 2021
Closed
@sync-by-unito sync-by-unito bot mentioned this issue Feb 1, 2022
Closed
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this issue Jun 17, 2022
Update dependency karma to v6.4.0 (#1412)
59fd409 
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [karma](https://karma-runner.github.io/) ([source](https://github.com/karma-runner/karma)) | devDependencies | minor | [`6.3.20` -> `6.4.0`](https://renovatebot.com/diffs/npm/karma/6.3.20/6.4.0) |

---

### Release Notes

<details>
<summary>karma-runner/karma</summary>

### [`v6.4.0`](https://github.com/karma-runner/karma/blob/HEAD/CHANGELOG.md#&#8203;640-httpsgithubcomkarma-runnerkarmacomparev6320v640-2022-06-14)

[Compare Source](karma-runner/karma@v6.3.20...v6.4.0)

##### Features

-   support SRI verification of link tags ([dc51a2e](karma-runner/karma@dc51a2e))
-   support SRI verification of script tags ([6a54b1c](karma-runner/karma@6a54b1c))

#### [6.3.20](karma-runner/karma@v6.3.19...v6.3.20) (2022-05-13)

##### Bug Fixes

-   prefer IPv4 addresses when resolving domains ([e17698f](karma-runner/karma@e17698f)), closes [#&#8203;3730](karma-runner/karma#3730)

#### [6.3.19](karma-runner/karma@v6.3.18...v6.3.19) (2022-04-19)

##### Bug Fixes

-   **client:** error out when opening a new tab fails ([099b85e](karma-runner/karma@099b85e))

#### [6.3.18](karma-runner/karma@v6.3.17...v6.3.18) (2022-04-13)

##### Bug Fixes

-   **deps:** upgrade socket.io to v4.4.1 ([52a30bb](karma-runner/karma@52a30bb))

#### [6.3.17](karma-runner/karma@v6.3.16...v6.3.17) (2022-02-28)

##### Bug Fixes

-   **deps:** update colors to maintained version ([#&#8203;3763](karma-runner/karma#3763)) ([fca1884](karma-runner/karma@fca1884))

#### [6.3.16](karma-runner/karma@v6.3.15...v6.3.16) (2022-02-10)

##### Bug Fixes

-   **security:** mitigate the "Open Redirect Vulnerability" ([ff7edbb](karma-runner/karma@ff7edbb))

#### [6.3.15](karma-runner/karma@v6.3.14...v6.3.15) (2022-02-05)

##### Bug Fixes

-   **helper:** make mkdirIfNotExists helper resilient to concurrent calls ([d9dade2](karma-runner/karma@d9dade2)), closes [/github.com/karma-runner/karma-coverage/issues/434#issuecomment-1017939333](https://github.com//github.com/karma-runner/karma-coverage/issues/434/issues/issuecomment-1017939333)

#### [6.3.14](karma-runner/karma@v6.3.13...v6.3.14) (2022-02-05)

##### Bug Fixes

-   remove string template from client code ([91d5acd](karma-runner/karma@91d5acd))
-   warn when `singleRun` and `autoWatch` are `false` ([69cfc76](karma-runner/karma@69cfc76))
-   **security:** remove XSS vulnerability in `returnUrl` query param ([839578c](karma-runner/karma@839578c))

#### [6.3.13](karma-runner/karma@v6.3.12...v6.3.13) (2022-01-31)

##### Bug Fixes

-   **deps:** bump log4js to resolve security issue ([5bf2df3](karma-runner/karma@5bf2df3)), closes [#&#8203;3751](karma-runner/karma#3751)

#### [6.3.12](karma-runner/karma@v6.3.11...v6.3.12) (2022-01-24)

##### Bug Fixes

-   remove depreciation warning from log4js ([41bed33](karma-runner/karma@41bed33))

#### [6.3.11](karma-runner/karma@v6.3.10...v6.3.11) (2022-01-13)

##### Bug Fixes

-   **deps:** pin colors package to 1.4.0 due to security vulnerability ([a5219c5](karma-runner/karma@a5219c5))

#### [6.3.10](karma-runner/karma@v6.3.9...v6.3.10) (2022-01-08)

##### Bug Fixes

-   **logger:** create parent folders if they are missing ([0d24bd9](karma-runner/karma@0d24bd9)), closes [#&#8203;3734](karma-runner/karma#3734)

#### [6.3.9](karma-runner/karma@v6.3.8...v6.3.9) (2021-11-16)

##### Bug Fixes

-   restartOnFileChange option not restarting the test run ([92ffe60](karma-runner/karma@92ffe60)), closes [#&#8203;27](karma-runner/karma#27) [#&#8203;3724](karma-runner/karma#3724)

#### [6.3.8](karma-runner/karma@v6.3.7...v6.3.8) (2021-11-07)

##### Bug Fixes

-   **reporter:** warning if stack trace contains generated code invocation ([4f23b14](karma-runner/karma@4f23b14))

#### [6.3.7](karma-runner/karma@v6.3.6...v6.3.7) (2021-11-01)

##### Bug Fixes

-   **middleware:** replace %X_UA_COMPATIBLE% marker anywhere in the file ([f1aeaec](karma-runner/karma@f1aeaec)), closes [#&#8203;3711](karma-runner/karma#3711)

#### [6.3.6](karma-runner/karma@v6.3.5...v6.3.6) (2021-10-25)

##### Bug Fixes

-   bump vulnerable ua-parser-js version ([6f2b2ec](karma-runner/karma@6f2b2ec)), closes [#&#8203;3713](karma-runner/karma#3713)

#### [6.3.5](karma-runner/karma@v6.3.4...v6.3.5) (2021-10-20)

##### Bug Fixes

-   **client:** prevent socket.io from hanging due to mocked clocks ([#&#8203;3695](karma-runner/karma#3695)) ([105da90](karma-runner/karma@105da90))

#### [6.3.4](karma-runner/karma@v6.3.3...v6.3.4) (2021-06-14)

##### Bug Fixes

-   bump production dependencies within SemVer ranges ([#&#8203;3682](karma-runner/karma#3682)) ([36467a8](karma-runner/karma@36467a8)), closes [#&#8203;3680](karma-runner/karma#3680)

#### [6.3.3](karma-runner/karma@v6.3.2...v6.3.3) (2021-06-01)

##### Bug Fixes

-   **server:** clean up vestigial code from proxy ([#&#8203;3640](karma-runner/karma#3640)) ([f4aeac3](karma-runner/karma@f4aeac3)), closes [/tools.ietf.org/html/std66#section-3](https://github.com//tools.ietf.org/html/std66/issues/section-3)

#### [6.3.2](karma-runner/karma@v6.3.1...v6.3.2) (2021-03-29)

##### Bug Fixes

-   fix running tests in IE9 ([#&#8203;3668](karma-runner/karma#3668)) ([0055bc5](karma-runner/karma@0055bc5)), closes [/github.com/karma-runner/karma/blob/026fff870913fb6cd2858dd962935dc74c92b725/client/main.js#L14](https://github.com//github.com/karma-runner/karma/blob/026fff870913fb6cd2858dd962935dc74c92b725/client/main.js/issues/L14) [#&#8203;3665](karma-runner/karma#3665)

#### [6.3.1](karma-runner/karma@v6.3.0...v6.3.1) (2021-03-24)

##### Bug Fixes

-   **client:** clearContext after complete sent ([#&#8203;3657](karma-runner/karma#3657)) ([c0962e3](karma-runner/karma@c0962e3))

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox.

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).

Co-authored-by: cabr2-bot <cabr2.help@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1412
Reviewed-by: Epsilon_02 <epsilon_02@noreply.codeberg.org>
Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
@karronoli karronoli mentioned this issue Aug 16, 2022
Merged
anthony-redFox pushed a commit to anthony-redFox/karma that referenced this issue May 16, 2023
@devoto13 @anthony-redFox
fix: restartOnFileChange option not restarting the test run
1ea73e7 
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
anthony-redFox pushed a commit to anthony-redFox/karma that referenced this issue May 16, 2023
@semantic-release-bot @anthony-redFox
chore(release): 6.3.9 [skip ci]
43948eb 
## [6.3.9](karma-runner/karma@v6.3.8...v6.3.9) (2021-11-16)

### Bug Fixes

* restartOnFileChange option not restarting the test run ([92ffe60](karma-runner@92ffe60)), closes [karma-runner#27](karma-runner#27) [karma-runner#3724](karma-runner#3724)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
released
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: restartOnFileChange option not restarting the test run devoto13/karma
3 participants
@scottohara @devoto13 @karmarunnerbot