From a0a5f808fb648802d1f5cf845ec0363cae78b59b Mon Sep 17 00:00:00 2001 From: Andrew Herron Date: Tue, 11 Aug 2026 16:43:00 +1000 Subject: [PATCH 1/2] TINYMCE-14759: Handle hook failures and stray promise rejections --- CHANGELOG.md | 7 + .../runner/src/main/ts/errors/ErrorCatcher.ts | 18 +- .../runner/src/main/ts/reporter/Reporter.ts | 69 ++++-- modules/runner/src/main/ts/runner/Run.ts | 21 +- modules/runner/src/main/ts/runner/Runner.ts | 6 +- modules/runner/src/main/ts/runner/TestRun.ts | 54 ++++- .../src/test/ts/errors/ErrorCatcherTest.ts | 26 +++ .../src/test/ts/reporter/ReporterTest.ts | 37 +++ .../src/test/ts/runner/RunnerTestUtils.ts | 12 +- .../runner/src/test/ts/runner/TestRunTest.ts | 215 +++++++++++++++++- .../test/ts/client/fail/BddHookFailTest.ts | 21 ++ .../test/ts/client/fail/StrayErrorFailTest.ts | 22 ++ 12 files changed, 464 insertions(+), 44 deletions(-) create mode 100644 modules/sample/src/test/ts/client/fail/BddHookFailTest.ts create mode 100644 modules/sample/src/test/ts/client/fail/StrayErrorFailTest.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 4a94e895..ceffb97b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## Unreleased +### Fixed +- A rejected promise or exception in a `before` or `after` hook aborted the entire run instead of being reported as a failure. #TINYMCE-14759 +- An error arriving after the test that caused it had finished was silently discarded, it is now reported against the most recent test. #TINYMCE-14759 +- A test that timed out no longer hides the error from work that was still in flight, it is reported as ` (error after test)`. #TINYMCE-14759 +- A promise rejected without a reason was treated as a pass. #TINYMCE-14759 +- An `after` or `afterEach` hook failure replaced the error of the test that had already failed. #TINYMCE-14759 + ## 16.2.0 - 2026-03-25 ### Added diff --git a/modules/runner/src/main/ts/errors/ErrorCatcher.ts b/modules/runner/src/main/ts/errors/ErrorCatcher.ts index 618625f5..d473b2e3 100644 --- a/modules/runner/src/main/ts/errors/ErrorCatcher.ts +++ b/modules/runner/src/main/ts/errors/ErrorCatcher.ts @@ -6,6 +6,7 @@ type ErrorHandler = (error: Error) => void export interface ErrorCatcher { readonly bind: (onError: ErrorHandler) => { unbind: () => void }; + readonly bindFallback: (onError: ErrorHandler) => { unbind: () => void }; readonly destroy: () => void; } @@ -20,12 +21,16 @@ const isError = (e: unknown): e is Error => export const ErrorCatcher = (): ErrorCatcher => { const supportsGlobalEventListeners = Global.addEventListener !== undefined; const onErrorHandlers: Set = new Set(); + const fallbackHandlers: Set = new Set(); let bound = false; const createHandler = (extractError: (e: T) => Error) => (e: T) => { - if (onErrorHandlers.size > 0) { + // Errors can arrive while nothing is running, such as a promise that rejects after the test that + // created it has settled, so fall back to the handlers that deal with those + const handlers = onErrorHandlers.size > 0 ? onErrorHandlers : fallbackHandlers; + if (handlers.size > 0) { const error = extractError(e); - onErrorHandlers.forEach((onError) => onError(error)); + handlers.forEach((onError) => onError(error)); e.preventDefault(); return false; } else { @@ -73,17 +78,18 @@ export const ErrorCatcher = (): ErrorCatcher => { } }; - const addHandler = (onError: ErrorHandler) => { - onErrorHandlers.add(onError); + const addHandler = (handlers: Set) => (onError: ErrorHandler) => { + handlers.add(onError); bind(); return { - unbind: () => onErrorHandlers.delete(onError) + unbind: () => handlers.delete(onError) }; }; return { - bind: addHandler, + bind: addHandler(onErrorHandlers), + bindFallback: addHandler(fallbackHandlers), destroy: unbind }; }; \ No newline at end of file diff --git a/modules/runner/src/main/ts/reporter/Reporter.ts b/modules/runner/src/main/ts/reporter/Reporter.ts index 599680c3..5ad1b2be 100644 --- a/modules/runner/src/main/ts/reporter/Reporter.ts +++ b/modules/runner/src/main/ts/reporter/Reporter.ts @@ -16,6 +16,7 @@ export interface TestReporter { export interface Reporter { readonly summary: () => { offset: number; passed: number; failed: number; skipped: number }; readonly test: (file: string, name: string, totalNumTests: number) => TestReporter; + readonly strayFailure: (file: string, name: string, e: LoggedError) => void; readonly waitForResults: () => Promise; readonly retry: () => Promise; readonly done: (error?: LoggedError) => void; @@ -28,6 +29,7 @@ export interface ReporterUi { readonly skip: (testTime: string, currentCount: number) => void; readonly fail: (e: LoggedError, testTime: string, currentCount: number) => void; }; + readonly error: (e: LoggedError) => void; readonly done: (totalTime: string) => void; } @@ -53,6 +55,7 @@ export const Reporter = (params: UrlParams, callbacks: Callbacks, ui: ReporterUi let passCount = 0; let skipCount = 0; let failCount = 0; + let finished = false; // A list of test results we are going to send as a batch to the server const testResults: TestReport[] = []; @@ -76,6 +79,30 @@ export const Reporter = (params: UrlParams, callbacks: Callbacks, ui: ReporterUi } }; + const reportFailure = (file: string, name: string, testTime: string, e: LoggedError, onReported: (err: LoggedError) => void): void => { + failCount++; + + // `sourcemapped-stacktrace` is async, so we need to wait for it + requestsInFlight.push(mapError(e).then((err) => { + const errorData = ErrorReporter.data(err); + const error = { + data: errorData, + text: ErrorReporter.dataText(errorData) + }; + + reportResult({ + file, + name, + passed: false, + time: testTime, + error, + skipped: null, + }); + + onReported(err); + })); + }; + const summary = () => ({ offset: Math.max(0, currentCount - 1), passed: passCount + (params.offset - params.failed - params.skipped), @@ -154,29 +181,8 @@ export const Reporter = (params: UrlParams, callbacks: Callbacks, ui: ReporterUi const fail = (e: LoggedError): void => { if (!reported) { reported = true; - failCount++; - const testTime = elapsed(starttime); - - // `sourcemapped-stacktrace` is async, so we need to wait for it - requestsInFlight.push(mapError(e).then((err) => { - const errorData = ErrorReporter.data(err); - const error = { - data: errorData, - text: ErrorReporter.dataText(errorData) - }; - - reportResult({ - file, - name, - passed: false, - time: testTime, - error, - skipped: null, - }); - - testUi.fail(err, testTime, currentCount); - })); + reportFailure(file, name, testTime, e, (err) => testUi.fail(err, testTime, currentCount)); } }; @@ -189,6 +195,23 @@ export const Reporter = (params: UrlParams, callbacks: Callbacks, ui: ReporterUi }; }; + // An error that arrives outside of a test is attributed to whichever test ran most recently, which + // replaces that test's result on the server. It deliberately doesn't advance the test count - the + // run is still going, and moving the count on would make the next page load skip a test. + const strayFailure = (file: string, name: string, e: LoggedError): void => { + // once the run is over there's nothing left to attribute this to, and sending more results would + // reopen the session on the server + if (finished) { + ui.error(e); + } else { + reportFailure(file, name, elapsed(Date.now()), e, (err) => { + ui.error(err); + // nothing is waiting on this result and the page may be about to reload, so send it now + forceReportResults(); + }); + } + }; + const waitForResults = async (): Promise => { forceReportResults(); if (requestsInFlight.length > 0) { @@ -212,6 +235,7 @@ export const Reporter = (params: UrlParams, callbacks: Callbacks, ui: ReporterUi }; const done = (error?: LoggedError): void => { + finished = true; const setAsDone = (): void => { const totalTime = elapsed(initial); ui.done(totalTime); @@ -228,6 +252,7 @@ export const Reporter = (params: UrlParams, callbacks: Callbacks, ui: ReporterUi return { summary, test, + strayFailure, retry, waitForResults, done diff --git a/modules/runner/src/main/ts/runner/Run.ts b/modules/runner/src/main/ts/runner/Run.ts index b273314c..efd41764 100644 --- a/modules/runner/src/main/ts/runner/Run.ts +++ b/modules/runner/src/main/ts/runner/Run.ts @@ -1,13 +1,28 @@ -import { Context, ExecuteFn, Failure, Runnable, RunnableState, TestThrowable } from '@ephox/bedrock-common'; +import { Context, ExecuteFn, Failure, LoggedError, Runnable, RunnableState, TestThrowable, Type } from '@ephox/bedrock-common'; import { isInternalError, MultipleDone, SkipError } from '../errors/Errors'; import { ErrorCatcher } from '../errors/ErrorCatcher'; import { Timer } from './Timer'; +type LoggedError = LoggedError.LoggedError; + const isPromiseLike = (value: unknown | undefined): value is PromiseLike => value !== undefined && (value as PromiseLike).then !== undefined; +// Promises can be rejected, and values thrown, with anything at all - including nothing - so +// substitute an error for the values that can't be reported on +const normalizeThrowable = (e: unknown): TestThrowable => + Type.isObject(e) || Type.isString(e) || typeof e === 'function' + ? e as TestThrowable + : new Error(`Failed with no or falsy error: ${e}`); + const errorCatcher = ErrorCatcher(); +// Errors that arrive while no runnable is executing are otherwise lost, as there's nothing left to +// fail. They still need reporting, so hand them to the runner to attribute to whatever ran last. +export const catchStrayErrors = (onError: (e: LoggedError) => void): void => { + errorCatcher.bindFallback((e) => onError(Failure.prepFailure(e))); +}; + export const runWithErrorCatcher = (runnable: Runnable, fn: () => Promise): Promise => { return new Promise((resolve, reject) => { let errorOccurred = false; @@ -53,13 +68,13 @@ const runExecFn = (fn: ExecuteFn, context: Context): Promise => { const retValue: any = fn.call(context, done); if (fn.length === 0) { if (isPromiseLike(retValue)) { - retValue.then(() => done(), done); + retValue.then(() => done(), (e) => done(normalizeThrowable(e))); } else { resolve(); } } } catch (e) { - const err = isInternalError(e) ? e : Failure.prepFailure(e); + const err = isInternalError(e) ? e : Failure.prepFailure(normalizeThrowable(e)); reject(err); } }); diff --git a/modules/runner/src/main/ts/runner/Runner.ts b/modules/runner/src/main/ts/runner/Runner.ts index 2bcd6a2c..a8260180 100644 --- a/modules/runner/src/main/ts/runner/Runner.ts +++ b/modules/runner/src/main/ts/runner/Runner.ts @@ -6,7 +6,8 @@ import { Callbacks } from '../reporter/Callbacks'; import { Reporter } from '../reporter/Reporter'; import { Actions } from '../ui/Actions'; import { Ui } from '../ui/Ui'; -import { RunActions, RunState, runSuite } from './TestRun'; +import { catchStrayErrors } from './Run'; +import { reportStrayError, RunActions, RunState, runSuite } from './TestRun'; import { countTests, filterOnly } from './Utils'; export interface Runner { @@ -111,6 +112,9 @@ export const Runner = (rootSuite: Suite, params: UrlParams, callbacks: Callbacks runNextChunk }; + // Don't let an error that lands between tests take down the run + catchStrayErrors(reportStrayError(runState, reporter)); + ui.setStopOnFailure(stopOnFailure); return runSuite(rootSuite, runState, runActions, reporter) .then(() => { diff --git a/modules/runner/src/main/ts/runner/TestRun.ts b/modules/runner/src/main/ts/runner/TestRun.ts index 9d06debb..9d4f7551 100644 --- a/modules/runner/src/main/ts/runner/TestRun.ts +++ b/modules/runner/src/main/ts/runner/TestRun.ts @@ -1,13 +1,13 @@ -import { Failure, LoggedError, RunnableState, Suite, Test } from '@ephox/bedrock-common'; +import { Failure, HookType, LoggedError, RunnableState, Suite, Test } from '@ephox/bedrock-common'; import * as Context from '../core/Context'; import { InternalError, isInternalError, SkipError } from '../errors/Errors'; import { Reporter, TestReporter } from '../reporter/Reporter'; import * as Hooks from './Hooks'; import { runWithErrorCatcher, runWithTimeout } from './Run'; -import { countTests, loop } from './Utils'; +import { countTests, getTests, loop } from './Utils'; type LoggedError = LoggedError.LoggedError; -type RunReporter = Pick; +type RunReporter = Pick; export interface RunState { readonly totalTests: number; @@ -15,6 +15,8 @@ export interface RunState { readonly chunk: number; readonly timeout: number; testCount: number; + // the test that is running, or the last one to have run, so stray errors have something to blame + currentTest?: Test; readonly checkSiblings: () => string[]; readonly auto: boolean; } @@ -31,8 +33,12 @@ const runTestWithRetry = (test: Test, state: RunState, testReport: TestReporter, if (test.isSkipped()) { return Promise.reject(new SkipError()); } else { + // Keep the original error if we already have one, so the failure that caused it isn't hidden const runAfterHooks = (error: boolean) => (result: T): Promise => - Hooks.runAfterEach(test).then(() => error ? Promise.reject(result) : Promise.resolve(result)); + Hooks.runAfterEach(test).then( + () => error ? Promise.reject(result) : Promise.resolve(result), + (e) => Promise.reject(error ? result : e) + ); return Hooks.runBeforeEach(test) .then(() => runWithErrorCatcher(test, () => runWithTimeout(test, Context.createContext(test, test), state.timeout))) @@ -86,6 +92,7 @@ export const runTest = (test: Test, state: RunState, actions: RunActions, report } else { const testReport = reporter.test(test.file || 'Unknown', test.fullTitle(), state.totalTests); + state.currentTest = test; actions.onStart(test); if (!state.auto) { console.log(`Starting test ${state.testCount} of ${state.totalTests}: ${test.fullTitle()} (${test.file})`); @@ -135,16 +142,53 @@ const checkSiblings = (suite: Suite, state: RunState, actions: RunActions, repor return Promise.reject(); }; +// An error that lands while nothing is running belongs to whatever ran last. A test that hasn't failed +// yet is failed by it, but one that has already been reported keeps its own error and the stray one is +// reported alongside - a timeout says far less about what went wrong than the rejection that follows it. +export const reportStrayError = (state: RunState, reporter: RunReporter) => (e: LoggedError): void => { + const test = state.currentTest; + console.error(e); + if (test === undefined) { + reporter.strayFailure('Unknown', 'Unknown', e); + } else if (test.isFailed()) { + reporter.strayFailure(test.file || 'Unknown', `${test.fullTitle()} (error after test)`, e); + } else { + test.setResult(RunnableState.Failed, e); + reporter.strayFailure(test.file || 'Unknown', test.fullTitle(), e); + } +}; + +// A suite hook isn't a test, but it's reported as one so the failure goes through the same retry and +// reporting machinery instead of aborting the entire run as an unexpected bedrock error +const failHook = (suite: Suite, type: HookType, state: RunState, actions: RunActions, reporter: RunReporter) => (e: LoggedError | InternalError): Promise => { + const file = getTests(suite)[0]?.file || 'Unknown'; + const hookReport = reporter.test(file, `${suite.fullTitle()} "${type}" hook`, state.totalTests); + const error = Failure.prepFailure(e); + + hookReport.start(); + console.error(error); + hookReport.fail(error); + actions.onFailure(); + // Test failures must be an empty reject, otherwise the error management thinks it's a bedrock error + return Promise.reject(); +}; + export const runSuite = (suite: Suite, state: RunState, actions: RunActions, reporter: RunReporter): Promise => { const numTests = countTests(suite); if (state.testCount + numTests <= state.offset) { state.testCount += numTests; return Promise.resolve(); } else { + const onAfterFailure = failHook(suite, HookType.After, state, actions, reporter); + // Keep the original error if we already have one, as that failure has already been reported const runAfterHooks = (error: boolean) => (result: T): Promise => - Hooks.runAfter(suite).then(() => error ? Promise.reject(result) : Promise.resolve(result)); + Hooks.runAfter(suite).then( + () => error ? Promise.reject(result) : Promise.resolve(result), + (e) => error ? Promise.reject(result) : onAfterFailure(e) + ); return Hooks.runBefore(suite) + .catch(failHook(suite, HookType.Before, state, actions, reporter)) .then(() => runTests(suite.tests, state, actions, reporter)) .then(() => runSuites(suite.suites, state, actions, reporter)) .then(checkSiblings(suite, state, actions, reporter)) diff --git a/modules/runner/src/test/ts/errors/ErrorCatcherTest.ts b/modules/runner/src/test/ts/errors/ErrorCatcherTest.ts index add06137..702d228b 100644 --- a/modules/runner/src/test/ts/errors/ErrorCatcherTest.ts +++ b/modules/runner/src/test/ts/errors/ErrorCatcherTest.ts @@ -113,6 +113,32 @@ describe('ErrorCatcher', () => { assert.isFalse(prevented, 'Event should not be prevented from running the default action'); }); + it('should notify the fallback handler when nothing else is bound', () => { + let prevented = false; + const fallbackErrors: Error[] = []; + const fallback = errorCatcher.bindFallback((e) => fallbackErrors.push(e)); + const unhandledPromiseRejection = { + reason: new Error('message'), + preventDefault: () => { + prevented = true; + } + } as PromiseRejectionEvent; + + // while a runnable is bound the fallback should be left alone + fireEvent('unhandledrejection', unhandledPromiseRejection); + assert.lengthOf(errors, 1, 'The bound handler should be notified'); + assert.lengthOf(fallbackErrors, 0, 'The fallback should not be notified'); + + unbind(); + fireEvent('unhandledrejection', unhandledPromiseRejection); + assert.lengthOf(errors, 1, 'The unbound handler should not be notified again'); + assert.lengthOf(fallbackErrors, 1, 'The fallback should be notified'); + assert.equal(fallbackErrors[0].message, 'Unhandled promise rejection: message'); + assert.isTrue(prevented, 'Event should be prevented from running the default action'); + + fallback.unbind(); + }); + it('should handle cross frame errors', () => { let prevented = false; // Note: As this doesn't run in the browser we can't do a proper cross frame check so simulate an Error diff --git a/modules/runner/src/test/ts/reporter/ReporterTest.ts b/modules/runner/src/test/ts/reporter/ReporterTest.ts index c2eb84a8..648cee3e 100644 --- a/modules/runner/src/test/ts/reporter/ReporterTest.ts +++ b/modules/runner/src/test/ts/reporter/ReporterTest.ts @@ -41,6 +41,7 @@ const ui = { skip: noop, fail: noop, }), + error: noop, done: noop }; @@ -167,6 +168,42 @@ describe('Reporter.test', () => { })); }); + it('should report a stray failure against the given test without advancing the test count', () => { + reset(0); + const test = reporter.test('StrayTest.ts', 'stray - test', 1); + const error = LoggedError.loggedError(new Error('Rejected after the test finished'), []); + + test.start(); + test.pass(); + reporter.strayFailure('StrayTest.ts', 'stray - test', error); + + return reporter.waitForResults().then(() => { + assert.equal(endTestData.length, 2, 'Checking the pass and the stray failure were both reported'); + const data = endTestData[1]; + assert.equal(data.file, 'StrayTest.ts', 'Checking filename'); + assert.equal(data.name, 'stray - test', 'Checking testname'); + assert.isFalse(data.passed, 'Checking passed state'); + assert.include(data.error?.text, 'Error: Rejected after the test finished', 'Checking error text'); + + assert.deepEqual(reporter.summary(), { + offset: 0, + passed: 1, + failed: 1, + skipped: 0 + }, 'The stray failure is counted, but the offset is left where the test left it'); + }); + }); + + it('should not report a stray failure once the run is done', async () => { + reset(0); + reporter.done(); + reporter.strayFailure('StrayTest.ts', 'stray - test', LoggedError.loggedError(new Error('Too late'), [])); + + await wait(150); + await reporter.waitForResults(); + assert.equal(endTestData.length, 0, 'The stray failure should not reopen the session'); + }); + it('should report done', async () => { reporter.done(); // done waits about 100ms, so we have to wait 150 diff --git a/modules/runner/src/test/ts/runner/RunnerTestUtils.ts b/modules/runner/src/test/ts/runner/RunnerTestUtils.ts index 1e0e8f3f..ced1ed9f 100644 --- a/modules/runner/src/test/ts/runner/RunnerTestUtils.ts +++ b/modules/runner/src/test/ts/runner/RunnerTestUtils.ts @@ -8,6 +8,7 @@ type LoggedError = LoggedError.LoggedError; export interface MockReporter extends Reporter { readonly failures: () => LoggedError[]; + readonly failedNames: () => string[]; } export const TEST_TIMEOUT = 200; @@ -30,8 +31,9 @@ export const MockReporter = (): MockReporter => { let passed = 0; let skipped = 0; const failures: LoggedError[] = []; + const failedNames: string[] = []; - const test = (): TestReporter => ({ + const test = (_file: string, name: string): TestReporter => ({ start: () => Promise.resolve(), retry: () => Promise.resolve(), pass: () => { @@ -40,6 +42,7 @@ export const MockReporter = (): MockReporter => { }, fail: (e) => { failures.push(e); + failedNames.push(name); return Promise.resolve(); }, skip: () => { @@ -50,11 +53,16 @@ export const MockReporter = (): MockReporter => { return { test, + strayFailure: (_file, name, e) => { + failures.push(e); + failedNames.push(name); + }, summary: () => ({ offset: 0, passed, failed: failures.length, skipped }), done: noop, waitForResults: Promise.resolve, retry: Promise.resolve, - failures: () => failures + failures: () => failures, + failedNames: () => failedNames }; }; diff --git a/modules/runner/src/test/ts/runner/TestRunTest.ts b/modules/runner/src/test/ts/runner/TestRunTest.ts index 8a75cdb6..7f57c997 100644 --- a/modules/runner/src/test/ts/runner/TestRunTest.ts +++ b/modules/runner/src/test/ts/runner/TestRunTest.ts @@ -1,10 +1,10 @@ -import { Context, HookType, RunnableState, Suite, Test } from '@ephox/bedrock-common'; +import { Context, ExecuteFn, HookType, LoggedError, RunnableState, Suite, Test } from '@ephox/bedrock-common'; import { assert } from 'chai'; import * as fc from 'fast-check'; import { beforeEach, describe, it } from 'mocha'; -import { createRootSuite } from '../../../main/ts/core/Suite'; +import { createHook } from '../../../main/ts/core/Hook'; +import { createRootSuite, createSuite } from '../../../main/ts/core/Suite'; import { createTest } from '../../../main/ts/core/Test'; -import { Reporter } from '../../../main/ts/reporter/Reporter'; import * as TestRun from '../../../main/ts/runner/TestRun'; import { noop } from '../TestUtils'; import { MockReporter } from './RunnerTestUtils'; @@ -17,6 +17,18 @@ interface MockTest extends Test { const sleep = (time: number): Promise => new Promise((resolve) => setTimeout(resolve, time)); +const addHook = (suite: Suite, type: HookType, fn: ExecuteFn): void => { + suite.hooks[type].push(createHook(type, fn)); +}; + +const expectReportedFailure = (promise: Promise, assertions: () => void): Promise => + promise.then(() => { + assert.fail('Expected the run to reject'); + }, (e) => { + assert.isUndefined(e, 'A reported failure must reject with no error, otherwise it is treated as a bedrock error'); + assertions(); + }); + const createMockTest = (name: string, suite: Suite, testFn?: (this: Context) => Promise) => { const test: MockTest = createTest(name, function (this: Context) { test.hasRun = true; @@ -276,22 +288,125 @@ describe('TestRun.runTest', () => { assert.deepEqual(states, [ RunnableState.NotRun, RunnableState.Failed ]); }); }); + + it('should report a failure when a beforeEach hook rejects', () => { + addHook(suite, HookType.BeforeEach, () => Promise.reject(new Error('beforeEach died'))); + + const test: MockTest = createMockTest('test', suite); + const state = RunnerTestUtils.createRunState(0, 100, 0); + return expectReportedFailure(TestRun.runTest(test, state, actions, reporter), () => { + assert.isNotTrue(test.hasRun); + assert.isTrue(onFailureRun); + assert.equal(reporter.failures()[0].message, 'beforeEach died'); + }); + }); + + it('should report a failure when a beforeEach hook throws', () => { + addHook(suite, HookType.BeforeEach, () => { + throw new Error('beforeEach died'); + }); + + const test: MockTest = createMockTest('test', suite); + const state = RunnerTestUtils.createRunState(0, 100, 0); + return expectReportedFailure(TestRun.runTest(test, state, actions, reporter), () => { + assert.isNotTrue(test.hasRun); + assert.isTrue(onFailureRun); + assert.equal(reporter.failures()[0].message, 'beforeEach died'); + }); + }); + + it('should report a failure when an afterEach hook rejects', () => { + addHook(suite, HookType.AfterEach, () => Promise.reject(new Error('afterEach died'))); + + const test: MockTest = createMockTest('test', suite); + const state = RunnerTestUtils.createRunState(0, 100, 0); + return expectReportedFailure(TestRun.runTest(test, state, actions, reporter), () => { + assert.isTrue(test.hasRun); + assert.isTrue(onFailureRun); + assert.equal(reporter.failures()[0].message, 'afterEach died'); + }); + }); + + it('should keep the test error when an afterEach hook fails as well', () => { + addHook(suite, HookType.AfterEach, () => Promise.reject(new Error('afterEach died'))); + + const test: MockTest = createMockTest('test', suite, () => Promise.reject(new Error('test died'))); + const state = RunnerTestUtils.createRunState(0, 100, 0); + return expectReportedFailure(TestRun.runTest(test, state, actions, reporter), () => { + assert.lengthOf(reporter.failures(), 1); + assert.equal(reporter.failures()[0].message, 'test died'); + }); + }); + + it('should report a failure when a hook rejects without a reason', () => { + addHook(suite, HookType.BeforeEach, () => Promise.reject()); + + const test: MockTest = createMockTest('test', suite); + const state = RunnerTestUtils.createRunState(0, 100, 0); + return expectReportedFailure(TestRun.runTest(test, state, actions, reporter), () => { + assert.isNotTrue(test.hasRun); + assert.equal(reporter.failures()[0].message, 'Failed with no or falsy error: undefined'); + }); + }); +}); + +describe('TestRun.reportStrayError', () => { + let reporter: MockReporter; + let suite: Suite; + let state: TestRun.RunState; + const error = LoggedError.loggedError(new Error('landed after the test'), []); + + beforeEach(() => { + reporter = RunnerTestUtils.MockReporter(); + suite = createRootSuite('root'); + state = RunnerTestUtils.createRunState(0, 100, 0); + }); + + it('should fail the test it is attributed to', () => { + const test = createMockTest('test', suite); + state.currentTest = test; + + TestRun.reportStrayError(state, reporter)(error); + + assert.isTrue(test.isFailed(), 'The test should be marked as failed'); + assert.deepEqual(reporter.failedNames(), [ 'root - test' ]); + }); + + it('should not replace the error of a test that already failed', () => { + const test = createMockTest('test', suite); + test.setResult(RunnableState.Failed, new Error('Test ran too long')); + state.currentTest = test; + + TestRun.reportStrayError(state, reporter)(error); + + assert.deepEqual(reporter.failedNames(), [ 'root - test (error after test)' ], 'The name must differ so the reported failure is kept'); + assert.equal(test.error?.message, 'Test ran too long'); + }); + + it('should still report an error that arrives before any test has run', () => { + TestRun.reportStrayError(state, reporter)(error); + + assert.deepEqual(reporter.failedNames(), [ 'Unknown' ]); + assert.equal(reporter.failures()[0].message, 'landed after the test'); + }); }); describe('TestRun.runSuite', () => { - let reporter: Reporter; + let reporter: MockReporter; let suite: Suite; let test: MockTest; let actions: TestRun.RunActions; + let onFailureRun: boolean; beforeEach(() => { + onFailureRun = false; reporter = RunnerTestUtils.MockReporter(); suite = createRootSuite('root'); test = createMockTest('test', suite); suite.tests.push(test); actions = { onStart: noop, - onFailure: noop, + onFailure: () => onFailureRun = true, onPass: noop, onSkip: noop, runNextChunk: noop @@ -354,4 +469,94 @@ describe('TestRun.runSuite', () => { assert.deepEqual(hooks, [ HookType.Before, HookType.BeforeEach, HookType.AfterEach, HookType.BeforeEach, HookType.AfterEach, HookType.After ]); }); }); + + it('should report a failure when a before hook rejects', () => { + addHook(suite, HookType.Before, () => Promise.reject(new Error('before died'))); + + const state = RunnerTestUtils.createRunState(0, 100, 0); + return expectReportedFailure(TestRun.runSuite(suite, state, actions, reporter), () => { + assert.isNotTrue(test.hasRun); + assert.isTrue(onFailureRun); + assert.deepEqual(reporter.failedNames(), [ 'root "before" hook' ]); + assert.equal(reporter.failures()[0].message, 'before died'); + }); + }); + + it('should report a failure when a before hook throws', () => { + addHook(suite, HookType.Before, () => { + throw new Error('before died'); + }); + + const state = RunnerTestUtils.createRunState(0, 100, 0); + return expectReportedFailure(TestRun.runSuite(suite, state, actions, reporter), () => { + assert.isNotTrue(test.hasRun); + assert.isTrue(onFailureRun); + assert.deepEqual(reporter.failedNames(), [ 'root "before" hook' ]); + assert.equal(reporter.failures()[0].message, 'before died'); + }); + }); + + it('should still run the after hooks when a before hook fails', () => { + const hooks: HookType[] = []; + RunnerTestUtils.populateHooks(suite, 1, (_idx, type) => { + hooks.push(type); + }); + addHook(suite, HookType.Before, () => Promise.reject(new Error('before died'))); + + const state = RunnerTestUtils.createRunState(0, 100, 0); + return expectReportedFailure(TestRun.runSuite(suite, state, actions, reporter), () => { + assert.deepEqual(hooks, [ HookType.Before, HookType.After ]); + }); + }); + + it('should report a failure when an after hook rejects', () => { + addHook(suite, HookType.After, () => Promise.reject(new Error('after died'))); + + const state = RunnerTestUtils.createRunState(0, 100, 0); + return expectReportedFailure(TestRun.runSuite(suite, state, actions, reporter), () => { + assert.isTrue(test.hasRun); + assert.isTrue(onFailureRun); + assert.deepEqual(reporter.failedNames(), [ 'root "after" hook' ]); + assert.equal(reporter.failures()[0].message, 'after died'); + }); + }); + + it('should report a failure when an after hook throws', () => { + addHook(suite, HookType.After, () => { + throw new Error('after died'); + }); + + const state = RunnerTestUtils.createRunState(0, 100, 0); + return expectReportedFailure(TestRun.runSuite(suite, state, actions, reporter), () => { + assert.isTrue(test.hasRun); + assert.isTrue(onFailureRun); + assert.deepEqual(reporter.failedNames(), [ 'root "after" hook' ]); + assert.equal(reporter.failures()[0].message, 'after died'); + }); + }); + + it('should keep the test failure when an after hook fails as well', () => { + const failingTest = createMockTest('fail test', suite, () => Promise.reject(new Error('test died'))); + suite.tests.push(failingTest); + addHook(suite, HookType.After, () => Promise.reject(new Error('after died'))); + + const state = RunnerTestUtils.createRunState(0, 100, 0); + return expectReportedFailure(TestRun.runSuite(suite, state, actions, reporter), () => { + assert.deepEqual(reporter.failedNames(), [ 'root - fail test' ]); + assert.equal(reporter.failures()[0].message, 'test died'); + }); + }); + + it('should report a failure when a nested suite before hook fails, without failing the parent', () => { + const nested = createSuite('nested', suite); + suite.suites.push(nested); + nested.tests.push(createMockTest('nested test', nested)); + addHook(nested, HookType.Before, () => Promise.reject(new Error('before died'))); + + const state = RunnerTestUtils.createRunState(0, 100, 0); + return expectReportedFailure(TestRun.runSuite(suite, state, actions, reporter), () => { + assert.isTrue(test.hasRun); + assert.deepEqual(reporter.failedNames(), [ 'root / nested "before" hook' ]); + }); + }); }); diff --git a/modules/sample/src/test/ts/client/fail/BddHookFailTest.ts b/modules/sample/src/test/ts/client/fail/BddHookFailTest.ts new file mode 100644 index 00000000..6434eb21 --- /dev/null +++ b/modules/sample/src/test/ts/client/fail/BddHookFailTest.ts @@ -0,0 +1,21 @@ +import { after, before, describe, it } from '@ephox/bedrock-client'; + +describe('BDD Hook Fail', () => { + describe('Rejected promise in before', () => { + before(() => Promise.reject(new Error('before hook rejected'))); + + it('should not run', () => { + console.log('this test should not have run'); + }); + }); + + describe('Exception in after', () => { + after(() => { + throw new Error('after hook threw'); + }); + + it('should run', () => { + console.log('after hook test'); + }); + }); +}); diff --git a/modules/sample/src/test/ts/client/fail/StrayErrorFailTest.ts b/modules/sample/src/test/ts/client/fail/StrayErrorFailTest.ts new file mode 100644 index 00000000..9511ce82 --- /dev/null +++ b/modules/sample/src/test/ts/client/fail/StrayErrorFailTest.ts @@ -0,0 +1,22 @@ +import { describe, it } from '@ephox/bedrock-client'; + +const rejectLater = (message: string, delay: number) => { + // a floating promise, like a waiter that is still polling after the test has moved on + new Promise((_resolve, reject) => { + setTimeout(() => reject(new Error(message)), delay); + }); +}; + +describe('Stray Error Fail', () => { + it('times out with a waiter still in flight', function () { + this.timeout(300); + rejectLater('waiter still in flight', 600); + return new Promise((resolve) => setTimeout(resolve, 2000)); + }); + + it('passes but leaves a rejection behind', () => { + rejectLater('rejected after the test passed', 200); + }); + + it('runs after the stray rejections land', () => new Promise((resolve) => setTimeout(resolve, 500))); +}); From 822511a932e4c267ddde32497b0c6a08f1e51e15 Mon Sep 17 00:00:00 2001 From: Andrew Herron Date: Wed, 12 Aug 2026 10:20:48 +1000 Subject: [PATCH 2/2] Fix comment --- modules/runner/src/main/ts/errors/ErrorCatcher.ts | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/modules/runner/src/main/ts/errors/ErrorCatcher.ts b/modules/runner/src/main/ts/errors/ErrorCatcher.ts index d473b2e3..2c670d48 100644 --- a/modules/runner/src/main/ts/errors/ErrorCatcher.ts +++ b/modules/runner/src/main/ts/errors/ErrorCatcher.ts @@ -25,8 +25,7 @@ export const ErrorCatcher = (): ErrorCatcher => { let bound = false; const createHandler = (extractError: (e: T) => Error) => (e: T) => { - // Errors can arrive while nothing is running, such as a promise that rejects after the test that - // created it has settled, so fall back to the handlers that deal with those + // Unhandled errors that arrive either between tests after the test suite will have no handlers available, use the fallback const handlers = onErrorHandlers.size > 0 ? onErrorHandlers : fallbackHandlers; if (handlers.size > 0) { const error = extractError(e); @@ -92,4 +91,4 @@ export const ErrorCatcher = (): ErrorCatcher => { bindFallback: addHandler(fallbackHandlers), destroy: unbind }; -}; \ No newline at end of file +};