From e913150049012b8816abff8315611fcbd1221c9b Mon Sep 17 00:00:00 2001 From: Yaroslav Admin Date: Sat, 13 Nov 2021 19:41:26 +0100 Subject: [PATCH 1/2] fix: restartOnFileChange option not restarting the test run 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 15d80f47a227839e9b0d54aeddf49b9aa9afe8aa 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 --- client/karma.js | 23 +++++++++-------------- static/karma.js | 23 +++++++++-------------- 2 files changed, 18 insertions(+), 28 deletions(-) diff --git a/client/karma.js b/client/karma.js index 0cc7556e0..64264d1d0 100644 --- a/client/karma.js +++ b/client/karma.js @@ -232,20 +232,15 @@ function Karma (updater, socket, iframe, opener, navigator, location, document) resultsBuffer = [] } - // A test could have incorrectly issued a navigate. Wait one turn - // to ensure the error from an incorrect navigate is processed. - var config = this.config - setTimeout(function () { - socket.emit('complete', result || {}) - if (config.clearContext) { - navigateContextTo('about:blank') - } else { - self.updater.updateTestStatus('complete') - } - if (returnUrl) { - location.href = returnUrl - } - }) + socket.emit('complete', result || {}) + if (this.config.clearContext) { + navigateContextTo('about:blank') + } else { + self.updater.updateTestStatus('complete') + } + if (returnUrl) { + location.href = returnUrl + } } this.info = function (info) { diff --git a/static/karma.js b/static/karma.js index 50be7e472..b88001881 100644 --- a/static/karma.js +++ b/static/karma.js @@ -242,20 +242,15 @@ function Karma (updater, socket, iframe, opener, navigator, location, document) resultsBuffer = [] } - // A test could have incorrectly issued a navigate. Wait one turn - // to ensure the error from an incorrect navigate is processed. - var config = this.config - setTimeout(function () { - socket.emit('complete', result || {}) - if (config.clearContext) { - navigateContextTo('about:blank') - } else { - self.updater.updateTestStatus('complete') - } - if (returnUrl) { - location.href = returnUrl - } - }) + socket.emit('complete', result || {}) + if (this.config.clearContext) { + navigateContextTo('about:blank') + } else { + self.updater.updateTestStatus('complete') + } + if (returnUrl) { + location.href = returnUrl + } } this.info = function (info) { From fd3f94e47c28c09873170cfd572523c786683428 Mon Sep 17 00:00:00 2001 From: Yaroslav Admin Date: Sat, 13 Nov 2021 23:41:34 +0100 Subject: [PATCH 2/2] test: add test case for restarting test run on file change --- test/e2e/restart-on-change.feature | 28 ++++++++++++++++++++ test/e2e/step_definitions/core_steps.js | 35 ++++++++++++++++++++++++- 2 files changed, 62 insertions(+), 1 deletion(-) create mode 100644 test/e2e/restart-on-change.feature diff --git a/test/e2e/restart-on-change.feature b/test/e2e/restart-on-change.feature new file mode 100644 index 000000000..7ccaa8fb8 --- /dev/null +++ b/test/e2e/restart-on-change.feature @@ -0,0 +1,28 @@ +Feature: Restart on file change + In order to use Karma + As a person who wants to write great tests + I want Karma to re-run tests whenever file changes. + + Scenario: Re-run tests when file changes + Given a configuration with: + """ + files = ['basic/plus.js', 'basic/test.js']; + browsers = ['ChromeHeadlessNoSandbox']; + plugins = [ + 'karma-jasmine', + 'karma-chrome-launcher' + ]; + restartOnFileChange = true; + singleRun = false; + """ + When I start a server in background + And I wait until server output contains: + """ + .. + Chrome Headless + """ + When I touch file: "basic/test.js" + Then the background stdout matches RegExp: + """ + Executed 2 of 2 SUCCESS[\s\S]+Executed 2 of 2 SUCCESS + """ diff --git a/test/e2e/step_definitions/core_steps.js b/test/e2e/step_definitions/core_steps.js index c9ed84a74..6b2cb31cd 100644 --- a/test/e2e/step_definitions/core_steps.js +++ b/test/e2e/step_definitions/core_steps.js @@ -27,7 +27,11 @@ When('I stop a server programmatically', function (callback) { }) When('I start a server in background', async function () { - await this.runBackgroundProcess(['start', '--log-level', 'debug', this.configFile]) + await this.runBackgroundProcess(['start', this.configFile]) +}) + +When('I start a server in background with additional arguments: {string}', async function (args) { + await this.runBackgroundProcess(['start', ...args.split(' '), this.configFile]) }) When('I wait until server output contains:', async function (expectedOutput) { @@ -50,6 +54,11 @@ When('I {command} Karma', async function (command) { await this.runForegroundProcess(`${command} ${this.configFile}`) }) +When('I touch file: {string}', async function (file) { + const now = new Date() + await fs.promises.utimes(path.join(this.workDir, file), now, now) +}) + When('I {command} Karma with additional arguments: {string}', async function (command, args) { await this.runForegroundProcess(`${command} ${this.configFile} ${args}`) }) @@ -149,3 +158,27 @@ Then(/^the file at ([a-zA-Z0-9/\\_.]+) contains:$/, function (filePath, expected throw new Error('Expected output to match the following:\n ' + expectedOutput + '\nGot:\n ' + data) } }) + +Then(/^the background (stdout|stderr) (is exactly|contains|matches RegExp):$/, async function (outputType, comparison, expectedOutput) { + const message = comparison === 'is exactly' ? 'Expected output to be exactly as above, but got:' + : comparison === 'contains' ? 'Expected output to contain the above text, but got:' + : 'Expected output to match the above RegExp, but got:' + + await waitForCondition( + () => { + const actualOutput = this.backgroundProcess[outputType].trim() + expectedOutput = expectedOutput.trim() + + switch (comparison) { + case 'is exactly': + return actualOutput === expectedOutput + case 'contains': + return actualOutput.includes(expectedOutput) + case 'matches RegExp': + return new RegExp(expectedOutput).test(actualOutput) + } + }, + 5000, + () => new Error(`${message}\n\n${this.backgroundProcess[outputType]}`) + ) +})