From dc5dc255dbee44dfef99e541873e31a04203ce2a Mon Sep 17 00:00:00 2001 From: Dmitry Sharabin Date: Tue, 11 Feb 2025 22:30:27 +0100 Subject: [PATCH 01/12] First stab at adding support for tests timeout --- src/classes/TestResult.js | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/src/classes/TestResult.js b/src/classes/TestResult.js index 76a11f2..a9a17d3 100644 --- a/src/classes/TestResult.js +++ b/src/classes/TestResult.js @@ -83,6 +83,27 @@ export default class TestResult extends BubblingEventTarget { * Run the test(s) */ async run () { + // If not specified, give the test 10s to run + let timeout = this.test.maxTime ?? this.test.maxTimeAsync ?? this.options.timeout ?? 10000; + + // Add a buffer to account for the time it takes to evaluate the test + timeout += 500; + + let timeoutId = setTimeout(() => { + this.error = new Error("Timed out"); + this.timeTaken = timeout; + + if (this.test.throws !== undefined) { + // We might expect this kind of error, so we evaluate the test to get a result + Object.assign(this, this.evaluateThrown()); + } + else { + this.pass = false; + } + + this.dispatchEvent(new Event("done", {bubbles: true})); + }, timeout); + this.messages = await interceptConsole(async () => { if (!this.parent) { // We are running the test in isolation, so we need to run beforeAll (if it exists) @@ -115,6 +136,10 @@ export default class TestResult extends BubblingEventTarget { } }); + // If we are here, the test didn't timeout + // Clean up + clearTimeout(timeoutId); + this.evaluate(); } From 7cfa3a2019cbe6ed75e182d329a83faec9954f7d Mon Sep 17 00:00:00 2001 From: Dmitry Sharabin Date: Thu, 13 Feb 2025 11:58:27 +0100 Subject: [PATCH 02/12] Fix the bug that *actually* should be fixed --- src/classes/TestResult.js | 55 +++++++++++++++++++++------------------ 1 file changed, 29 insertions(+), 26 deletions(-) diff --git a/src/classes/TestResult.js b/src/classes/TestResult.js index a9a17d3..5a0973c 100644 --- a/src/classes/TestResult.js +++ b/src/classes/TestResult.js @@ -83,39 +83,33 @@ export default class TestResult extends BubblingEventTarget { * Run the test(s) */ async run () { - // If not specified, give the test 10s to run - let timeout = this.test.maxTime ?? this.test.maxTimeAsync ?? this.options.timeout ?? 10000; - - // Add a buffer to account for the time it takes to evaluate the test - timeout += 500; + let test = this.test; - let timeoutId = setTimeout(() => { - this.error = new Error("Timed out"); - this.timeTaken = timeout; + let timeoutId; + if (test.maxTime || test.maxTimeAsync) { + // Add a buffer to account for the time it takes to evaluate the test + let timeout = Math.max(test.maxTime ?? 0, test.maxTimeAsync ?? 0) + 50; - if (this.test.throws !== undefined) { - // We might expect this kind of error, so we evaluate the test to get a result - Object.assign(this, this.evaluateThrown()); - } - else { - this.pass = false; - } + timeoutId = setTimeout(() => { + this.error = new Error("Timed out"); + this.timeTaken = timeout; - this.dispatchEvent(new Event("done", {bubbles: true})); - }, timeout); + this.evaluate(); + }, timeout); + } this.messages = await interceptConsole(async () => { if (!this.parent) { // We are running the test in isolation, so we need to run beforeAll (if it exists) - await this.test.beforeAll?.(); + await test.beforeAll?.(); } - await this.test.beforeEach?.(); + await test.beforeEach?.(); let start = performance.now(); try { - this.actual = this.test.run ? this.test.run.apply(this.test, this.test.args) : this.test.args[0]; + this.actual = test.run ? test.run.apply(test, test.args) : test.args[0]; this.timeTaken = performance.now() - start; if (this.actual instanceof Promise) { @@ -127,18 +121,20 @@ export default class TestResult extends BubblingEventTarget { this.error = e; } finally { - await this.test.afterEach?.(); + await test.afterEach?.(); if (!this.parent) { // We are running the test in isolation, so we need to run afterAll - await this.test.afterAll?.(); + await test.afterAll?.(); } } }); - // If we are here, the test didn't timeout - // Clean up - clearTimeout(timeoutId); + if (timeoutId) { + // If we are here, the test didn't timeout + // Clean up + clearTimeout(timeoutId); + } this.evaluate(); } @@ -211,7 +207,7 @@ export default class TestResult extends BubblingEventTarget { if (test.throws !== undefined) { Object.assign(this, this.evaluateThrown()); } - else if (test.maxTime || test.maxTimeAsync) { + else if (test.expect === undefined && (test.maxTime || test.maxTimeAsync)) { Object.assign(this, this.evaluateTimeTaken()); } else { @@ -386,6 +382,13 @@ ${ this.error.stack }`); let test = this.test; let ret = {pass: true, details: []}; + if (this.error && this.error.message.includes("Timed out")) { + ret.pass = false; + ret.details.push(`Test timed out after ${ this.timeTaken }ms`); + + return ret; + } + if (test.maxTime) { ret.pass &&= this.timeTaken <= test.maxTime; From e23ca262b7b590936dbc0d1c32c44b1029eb7b6e Mon Sep 17 00:00:00 2001 From: Dmitry Sharabin Date: Mon, 17 Feb 2025 14:00:58 +0100 Subject: [PATCH 03/12] Address feedback: Remove redundant check --- src/classes/TestResult.js | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/classes/TestResult.js b/src/classes/TestResult.js index 5a0973c..46fa722 100644 --- a/src/classes/TestResult.js +++ b/src/classes/TestResult.js @@ -130,11 +130,9 @@ export default class TestResult extends BubblingEventTarget { } }); - if (timeoutId) { - // If we are here, the test didn't timeout - // Clean up - clearTimeout(timeoutId); - } + // If we are here, the test didn't timeout + // Clean up + clearTimeout(timeoutId); this.evaluate(); } From 3671a5bf92f0b8d63a72ae6a14734bad3fe6750a Mon Sep 17 00:00:00 2001 From: Dmitry Sharabin Date: Mon, 17 Feb 2025 14:16:58 +0100 Subject: [PATCH 04/12] Address feedback --- src/classes/TestResult.js | 28 +++++++++++----------------- 1 file changed, 11 insertions(+), 17 deletions(-) diff --git a/src/classes/TestResult.js b/src/classes/TestResult.js index 46fa722..b44ca38 100644 --- a/src/classes/TestResult.js +++ b/src/classes/TestResult.js @@ -85,18 +85,19 @@ export default class TestResult extends BubblingEventTarget { async run () { let test = this.test; - let timeoutId; - if (test.maxTime || test.maxTimeAsync) { - // Add a buffer to account for the time it takes to evaluate the test - let timeout = Math.max(test.maxTime ?? 0, test.maxTimeAsync ?? 0) + 50; + // By default, give the test 10 seconds to run + let timeout = 10000; + if (test.maxTime && (test.expect !== undefined || test.throws !== undefined)) { + // For result-based and error-based tests, maxTime is the timeout + timeout = test.maxTime; + } - timeoutId = setTimeout(() => { - this.error = new Error("Timed out"); - this.timeTaken = timeout; + let timeoutId = setTimeout(() => { + this.error = new Error("Timeout"); + this.timeTaken = timeout; - this.evaluate(); - }, timeout); - } + this.evaluate(); + }, timeout); this.messages = await interceptConsole(async () => { if (!this.parent) { @@ -380,13 +381,6 @@ ${ this.error.stack }`); let test = this.test; let ret = {pass: true, details: []}; - if (this.error && this.error.message.includes("Timed out")) { - ret.pass = false; - ret.details.push(`Test timed out after ${ this.timeTaken }ms`); - - return ret; - } - if (test.maxTime) { ret.pass &&= this.timeTaken <= test.maxTime; From 36dccc4995d2ab1838d605b657ff88677cfc261d Mon Sep 17 00:00:00 2001 From: Dmitry Sharabin Date: Mon, 17 Feb 2025 14:29:22 +0100 Subject: [PATCH 05/12] Better condition --- src/classes/TestResult.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/classes/TestResult.js b/src/classes/TestResult.js index b44ca38..d74ac10 100644 --- a/src/classes/TestResult.js +++ b/src/classes/TestResult.js @@ -87,7 +87,7 @@ export default class TestResult extends BubblingEventTarget { // By default, give the test 10 seconds to run let timeout = 10000; - if (test.maxTime && (test.expect !== undefined || test.throws !== undefined)) { + if (test.maxTime && (!("expect" in test) || test.throws !== undefined)) { // For result-based and error-based tests, maxTime is the timeout timeout = test.maxTime; } @@ -206,7 +206,7 @@ export default class TestResult extends BubblingEventTarget { if (test.throws !== undefined) { Object.assign(this, this.evaluateThrown()); } - else if (test.expect === undefined && (test.maxTime || test.maxTimeAsync)) { + else if (!("expect" in test) && (test.maxTime || test.maxTimeAsync)) { Object.assign(this, this.evaluateTimeTaken()); } else { From fa9e0e90c3080ec020c25945d0f25d03cd02d1a5 Mon Sep 17 00:00:00 2001 From: Dmitry Sharabin Date: Tue, 18 Feb 2025 15:08:11 +0100 Subject: [PATCH 06/12] =?UTF-8?q?=F0=9F=A4=A6=E2=80=8D=E2=99=82=EF=B8=8F?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/classes/TestResult.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/classes/TestResult.js b/src/classes/TestResult.js index d74ac10..1d304bf 100644 --- a/src/classes/TestResult.js +++ b/src/classes/TestResult.js @@ -87,7 +87,7 @@ export default class TestResult extends BubblingEventTarget { // By default, give the test 10 seconds to run let timeout = 10000; - if (test.maxTime && (!("expect" in test) || test.throws !== undefined)) { + if (test.maxTime && ("expect" in test || test.throws !== undefined)) { // For result-based and error-based tests, maxTime is the timeout timeout = test.maxTime; } From 059d65218318c944ecd0239a335bdeeecfa9e5f1 Mon Sep 17 00:00:00 2001 From: Dmitry Sharabin Date: Tue, 18 Feb 2025 17:38:35 +0100 Subject: [PATCH 07/12] Evaluate time taken for all types of tests --- src/classes/TestResult.js | 61 +++++++++++++++++++++------------------ 1 file changed, 33 insertions(+), 28 deletions(-) diff --git a/src/classes/TestResult.js b/src/classes/TestResult.js index 1d304bf..d5d6f93 100644 --- a/src/classes/TestResult.js +++ b/src/classes/TestResult.js @@ -203,13 +203,14 @@ export default class TestResult extends BubblingEventTarget { evaluate () { let test = this.test; + if (test.maxTime || test.maxTimeAsync) { + Object.assign(this, this.evaluateTimeTaken()); + } + if (test.throws !== undefined) { Object.assign(this, this.evaluateThrown()); } - else if (!("expect" in test) && (test.maxTime || test.maxTimeAsync)) { - Object.assign(this, this.evaluateTimeTaken()); - } - else { + else if ("expect" in test) { Object.assign(this, this.evaluateResult()); } @@ -229,7 +230,7 @@ export default class TestResult extends BubblingEventTarget { */ evaluateThrown () { let test = this.test; - let ret = {pass: !!this.error, details: []}; + let ret = {pass: (this.pass ?? true) && !!this.error, details: this.details ?? []}; // We may have more picky criteria for the error if (ret.pass) { @@ -271,38 +272,42 @@ export default class TestResult extends BubblingEventTarget { */ evaluateResult () { let test = this.test; - let ret = {pass: true, details: []}; - if (test.map) { - try { - this.mapped = { - actual: Array.isArray(this.actual) ? this.actual.map(test.map) : test.map(this.actual), - expect: Array.isArray(test.expect) ? test.expect.map(test.map) : test.map(test.expect), - }; + // If we are here and there is an error (e.g., the test timed out), we consider the test failed + let ret = {pass: (this.pass ?? true) && !this.error, details: this.details ?? []}; + if (ret.pass) { + if (test.map) { try { - ret.pass = test.check(this.mapped.actual, this.mapped.expect); + this.mapped = { + actual: Array.isArray(this.actual) ? this.actual.map(test.map) : test.map(this.actual), + expect: Array.isArray(test.expect) ? test.expect.map(test.map) : test.map(test.expect), + }; + + try { + ret.pass = test.check(this.mapped.actual, this.mapped.expect); + } + catch (e) { + this.error = new Error(`check() failed (working with mapped values). ${ e.message }`); + } } catch (e) { - this.error = new Error(`check() failed (working with mapped values). ${ e.message }`); + this.error = new Error(`map() failed. ${ e.message }`); } } - catch (e) { - this.error = new Error(`map() failed. ${ e.message }`); - } - } - else { - try { - ret.pass = test.check(this.actual, test.expect); - } - catch (e) { - this.error = new Error(`check() failed. ${ e.message }`); + else { + try { + ret.pass = test.check(this.actual, test.expect); + } + catch (e) { + this.error = new Error(`check() failed. ${ e.message }`); + } } - } - // If `map()` or `check()` errors, consider the test failed - if (this.error) { - ret.pass = false; + // If `map()` or `check()` errors, consider the test failed + if (this.error) { + ret.pass = false; + } } if (!ret.pass) { From b730fc84b334d77a004b905037bc75e676059562 Mon Sep 17 00:00:00 2001 From: Dmitry Sharabin Date: Tue, 18 Feb 2025 17:44:29 +0100 Subject: [PATCH 08/12] Add tests --- tests/timeout.js | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) create mode 100644 tests/timeout.js diff --git a/tests/timeout.js b/tests/timeout.js new file mode 100644 index 0000000..8f2e0a3 --- /dev/null +++ b/tests/timeout.js @@ -0,0 +1,20 @@ +export default { + name: "Tests for timeout", + description: "These tests are designed to fail.", + run: () => new Promise(resolve => setTimeout(resolve, 200, "foo")), + maxTime: 100, + tests: [ + { + name: "Result-based test", + expect: "bar", + }, + { + name: "Error-based test", + throws: error => error.cause.name !== "TimeoutError", + }, + { + name: "Time-based test", + maxTimeAsync: 100, + }, + ], +}; From 828694f8d6ed9f740e21ff0deb77902731d37fad Mon Sep 17 00:00:00 2001 From: Dmitry Sharabin Date: Tue, 18 Feb 2025 17:45:17 +0100 Subject: [PATCH 09/12] Fix tests All tests should be result-based --- tests/failing-tests.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/failing-tests.js b/tests/failing-tests.js index 9e5828d..3098499 100644 --- a/tests/failing-tests.js +++ b/tests/failing-tests.js @@ -1,6 +1,7 @@ export default { name: "Failing tests", description: "These tests are designed to fail and should not break the test runner", + expect: 42, tests: [ { name: "map() fails", @@ -15,7 +16,6 @@ export default { map: arg => undefined, check: (actual, expected) => actual.length < expected.length, arg: 42, - expect: 42, }, ], }; From 32675766be80da3d7323606c35853352f0047554 Mon Sep 17 00:00:00 2001 From: Dmitry Sharabin Date: Tue, 18 Feb 2025 18:48:19 +0100 Subject: [PATCH 10/12] Add test for default timeout --- tests/timeout.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tests/timeout.js b/tests/timeout.js index 8f2e0a3..6ff4e91 100644 --- a/tests/timeout.js +++ b/tests/timeout.js @@ -16,5 +16,10 @@ export default { name: "Time-based test", maxTimeAsync: 100, }, + { + name: "Default timeout", + run: () => new Promise(resolve => setTimeout(resolve, 10200)), + skip: true, // Comment this line out to see the test fail after 10 seconds + }, ], }; From b7cc1e302362444e712525b856f2ca1b4ced407f Mon Sep 17 00:00:00 2001 From: Dmitry Sharabin Date: Thu, 20 Feb 2025 15:12:39 +0100 Subject: [PATCH 11/12] Use `AbortSignal` instead of `setTimeout` We need to be able to abort (for real) the test execution on time out. --- src/classes/TestResult.js | 78 +++++++++++++++++++++------------------ tests/timeout.js | 2 +- 2 files changed, 43 insertions(+), 37 deletions(-) diff --git a/src/classes/TestResult.js b/src/classes/TestResult.js index d5d6f93..429b9f2 100644 --- a/src/classes/TestResult.js +++ b/src/classes/TestResult.js @@ -85,56 +85,62 @@ export default class TestResult extends BubblingEventTarget { async run () { let test = this.test; - // By default, give the test 10 seconds to run - let timeout = 10000; + let timer = { + timeout: 10000, // by default, give the test 10 seconds to run + }; + if (test.maxTime && ("expect" in test || test.throws !== undefined)) { // For result-based and error-based tests, maxTime is the timeout - timeout = test.maxTime; + timer.timeout = test.maxTime; } - let timeoutId = setTimeout(() => { - this.error = new Error("Timeout"); - this.timeTaken = timeout; - - this.evaluate(); - }, timeout); - - this.messages = await interceptConsole(async () => { - if (!this.parent) { - // We are running the test in isolation, so we need to run beforeAll (if it exists) - await test.beforeAll?.(); - } + this.messages = await Promise.race([ + new Promise(resolve => { + timer.handler = () => { + this.error = new Error(`Test timed out after ${ timer.timeout }ms`); + this.timeTaken = timer.timeout; + resolve([]); + }; + + timer.signal = AbortSignal.timeout(timer.timeout); + timer.signal.addEventListener("abort", timer.handler); + }), + interceptConsole(async () => { + if (!this.parent) { + // We are running the test in isolation, so we need to run beforeAll (if it exists) + await test.beforeAll?.(); + } - await test.beforeEach?.(); + await test.beforeEach?.(); - let start = performance.now(); + let start = performance.now(); - try { - this.actual = test.run ? test.run.apply(test, test.args) : test.args[0]; - this.timeTaken = performance.now() - start; + try { + this.actual = test.run ? test.run.apply(test, test.args) : test.args[0]; + this.timeTaken = performance.now() - start; - if (this.actual instanceof Promise) { - this.actual = await this.actual; - this.timeTakenAsync = performance.now() - start; + if (this.actual instanceof Promise) { + this.actual = await this.actual; + this.timeTakenAsync = performance.now() - start; + } } - } - catch (e) { - this.error = e; - } - finally { - await test.afterEach?.(); + catch (e) { + this.error = e; + } + finally { + await test.afterEach?.(); - if (!this.parent) { + if (!this.parent) { // We are running the test in isolation, so we need to run afterAll - await test.afterAll?.(); + await test.afterAll?.(); + } } - } + }), + ]).finally(() => { + // Don't fire the abort event if the test finished before the timeout + timer.signal.removeEventListener("abort", timer.handler); }); - // If we are here, the test didn't timeout - // Clean up - clearTimeout(timeoutId); - this.evaluate(); } diff --git a/tests/timeout.js b/tests/timeout.js index 6ff4e91..5bcce24 100644 --- a/tests/timeout.js +++ b/tests/timeout.js @@ -10,7 +10,7 @@ export default { }, { name: "Error-based test", - throws: error => error.cause.name !== "TimeoutError", + throws: error => !error.message.startsWith("Test timed out"), }, { name: "Time-based test", From bd835281428dea9b9b4a7fb2613c93f8aaa55b4f Mon Sep 17 00:00:00 2001 From: Dmitry Sharabin Date: Thu, 20 Feb 2025 15:23:37 +0100 Subject: [PATCH 12/12] Actually, we can use `setTimeout` We just need to ensure that only one promise is resolved by the required time and the properties are set. --- src/classes/TestResult.js | 28 ++++++++++++---------------- 1 file changed, 12 insertions(+), 16 deletions(-) diff --git a/src/classes/TestResult.js b/src/classes/TestResult.js index 429b9f2..78ade2a 100644 --- a/src/classes/TestResult.js +++ b/src/classes/TestResult.js @@ -85,26 +85,23 @@ export default class TestResult extends BubblingEventTarget { async run () { let test = this.test; - let timer = { - timeout: 10000, // by default, give the test 10 seconds to run - }; - + // By default, give the test 10 seconds to run + let timeout = 10000; if (test.maxTime && ("expect" in test || test.throws !== undefined)) { // For result-based and error-based tests, maxTime is the timeout - timer.timeout = test.maxTime; + timeout = test.maxTime; } + let timeoutId; this.messages = await Promise.race([ new Promise(resolve => { - timer.handler = () => { - this.error = new Error(`Test timed out after ${ timer.timeout }ms`); - this.timeTaken = timer.timeout; + timeoutId = setTimeout(() => { + this.error = new Error(`Test timed out after ${ timeout }ms`); + this.timeTaken = timeout; resolve([]); - }; - - timer.signal = AbortSignal.timeout(timer.timeout); - timer.signal.addEventListener("abort", timer.handler); + }, timeout); }), + interceptConsole(async () => { if (!this.parent) { // We are running the test in isolation, so we need to run beforeAll (if it exists) @@ -136,10 +133,9 @@ export default class TestResult extends BubblingEventTarget { } } }), - ]).finally(() => { - // Don't fire the abort event if the test finished before the timeout - timer.signal.removeEventListener("abort", timer.handler); - }); + ]); + + clearTimeout(timeoutId); this.evaluate(); }