From 73a0dd53181d8a52a77430eaf88af00e2f0d72be Mon Sep 17 00:00:00 2001 From: Eberhard Beilharz Date: Wed, 20 May 2026 18:10:04 +0200 Subject: [PATCH 1/5] maint(web): fix Playwright TC reporter Previously the loop at the beginning of `end` had an infinite loop because the child was never removed from `childrenToVisit`. This change refactors and fixes the loop. Build-bot: skip build:web Test-bot: skip --- .../test/resources/playwright-TC-reporter.ts | 32 +++++++++++++++---- 1 file changed, 25 insertions(+), 7 deletions(-) diff --git a/common/test/resources/playwright-TC-reporter.ts b/common/test/resources/playwright-TC-reporter.ts index 84f3eede0d..c80e73e180 100644 --- a/common/test/resources/playwright-TC-reporter.ts +++ b/common/test/resources/playwright-TC-reporter.ts @@ -14,7 +14,7 @@ class TestNode { private id: string; private suiteOrTest: Suite | TestCase; private flowId: number; - private parent: TestNode; + private parent: TestNode | null; private childrenToVisit: TestNode[] = []; public constructor(suiteOrTest: Suite | TestCase) { @@ -89,12 +89,20 @@ class TestNode { private end(result: TestResult, force: boolean = false): void { if (this.childrenToVisit.length > 0 && force) { - while (this.childrenToVisit.length > 0) { - const child = this.childrenToVisit[0]; - child.end(result, force); + for (const child of [...this.childrenToVisit]) { + child.end(result, true); } + this.childrenToVisit.length = 0; } + // Only run this block on a normal (non-forced) end. Forced ends happen during endAll() + // teardown for nodes that didn't complete normally, where we have no meaningful result to + // report and the node's start/finish pairing may be incomplete. In that case we also skip the + // cleanup below because endAll()'s safety-net loop drains OpenNodes / Nodes globally, and our + // parent is already iterating its own children and clearing childrenToVisit itself — + // re-running removeFromOpenNodes / Nodes.delete / removeChild here would double-remove and + // log spurious errors, or trigger a re-entrant parent.end(null, false) cascade in the middle + // of the ongoing iteration. if (!force) { if (this.suiteOrTest.title !== '') { if (this.isTest) { @@ -103,10 +111,10 @@ class TestNode { } else { console.log(`##teamcity[testSuiteFinished name='${this.suiteOrTest.title}']`); } - console.log(`##teamcity[flowFinished flowId = '${this.flowId}']`); + console.log(`##teamcity[flowFinished flowId='${this.flowId}']`); } - this.removeFromOpenNodes(); + this.removeFromOpenNodes(); this.parent?.removeChild(this); TestNode.Nodes.delete(this.id); } @@ -122,6 +130,10 @@ class TestNode { } private removeChild(child: TestNode) { + if (this.childrenToVisit.length == 0) { + // already cleared all children in a end(result, true) call, nothing to do + return; + } const childIndex = this.childrenToVisit.indexOf(child); if (childIndex < 0) { console.error(`Can't find child '${child.id}' in parent '${this.id}'`); @@ -166,6 +178,12 @@ class TestNode { const id = TestNode.OpenNodes[TestNode.OpenNodes.length - 1]; console.log(`Closing '${id}'`); const node = TestNode.Nodes.get(id); + if (!node) { + console.error(`Node for '${id}' not found in Nodes map, removing from OpenNodes`); + TestNode.OpenNodes.pop(); + continue; + } + node.end(null); } } @@ -179,7 +197,7 @@ class TestNode { } export default class PlaywrightTeamcityReporter implements Reporter { - private root: TestNode = null; + private root!: TestNode; public constructor(options: { parentFlow?: string } = {}) { TestNode.RootFlow = options.parentFlow ?? 'unit_tests'; From 5dab671f2226ff2f4b9fff763b04879d4b1df7d1 Mon Sep 17 00:00:00 2001 From: Eberhard Beilharz Date: Thu, 21 May 2026 17:38:41 +0200 Subject: [PATCH 2/5] fix(web): address code review comments Co-authored-by: Marc Durdin --- common/test/resources/playwright-TC-reporter.ts | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/common/test/resources/playwright-TC-reporter.ts b/common/test/resources/playwright-TC-reporter.ts index c80e73e180..a6bfeb96d3 100644 --- a/common/test/resources/playwright-TC-reporter.ts +++ b/common/test/resources/playwright-TC-reporter.ts @@ -16,6 +16,7 @@ class TestNode { private flowId: number; private parent: TestNode | null; private childrenToVisit: TestNode[] = []; + private childrenCleared: boolean = false; public constructor(suiteOrTest: Suite | TestCase) { this.suiteOrTest = suiteOrTest; @@ -88,11 +89,13 @@ class TestNode { } private end(result: TestResult, force: boolean = false): void { - if (this.childrenToVisit.length > 0 && force) { + if (force) { + // use a copy of the array because child.end will mutate the array for (const child of [...this.childrenToVisit]) { child.end(result, true); } - this.childrenToVisit.length = 0; + this.childrenToVisit = []; + this.childrenCleared = true; } // Only run this block on a normal (non-forced) end. Forced ends happen during endAll() @@ -130,7 +133,7 @@ class TestNode { } private removeChild(child: TestNode) { - if (this.childrenToVisit.length == 0) { + if (this.childrenCleared) { // already cleared all children in a end(result, true) call, nothing to do return; } From 697e6972eee368cbb252ec3610ac2805038642d7 Mon Sep 17 00:00:00 2001 From: Eberhard Beilharz Date: Thu, 21 May 2026 18:00:16 +0200 Subject: [PATCH 3/5] fix(web): further simplification Turns out `end(result, true)` didn't actually do anything except calling `end(result, true)` on each child, which resulted basically in a no-op. Therefore this change removes the `force` parameter from `end()`. --- .../test/resources/playwright-TC-reporter.ts | 49 +++++-------------- 1 file changed, 12 insertions(+), 37 deletions(-) diff --git a/common/test/resources/playwright-TC-reporter.ts b/common/test/resources/playwright-TC-reporter.ts index a6bfeb96d3..ad082dedf4 100644 --- a/common/test/resources/playwright-TC-reporter.ts +++ b/common/test/resources/playwright-TC-reporter.ts @@ -16,7 +16,6 @@ class TestNode { private flowId: number; private parent: TestNode | null; private childrenToVisit: TestNode[] = []; - private childrenCleared: boolean = false; public constructor(suiteOrTest: Suite | TestCase) { this.suiteOrTest = suiteOrTest; @@ -88,39 +87,20 @@ class TestNode { } } - private end(result: TestResult, force: boolean = false): void { - if (force) { - // use a copy of the array because child.end will mutate the array - for (const child of [...this.childrenToVisit]) { - child.end(result, true); + private end(result: TestResult): void { + if (this.suiteOrTest.title !== '') { + if (this.isTest) { + const { msgTitle, details } = this.getTestResult(result) ?? { msgTitle: 'testFinished', details: '' }; + console.log(`##teamcity[${msgTitle} name='${this.suiteOrTest.title}' ${details}]`); + } else { + console.log(`##teamcity[testSuiteFinished name='${this.suiteOrTest.title}']`); } - this.childrenToVisit = []; - this.childrenCleared = true; + console.log(`##teamcity[flowFinished flowId='${this.flowId}']`); } - // Only run this block on a normal (non-forced) end. Forced ends happen during endAll() - // teardown for nodes that didn't complete normally, where we have no meaningful result to - // report and the node's start/finish pairing may be incomplete. In that case we also skip the - // cleanup below because endAll()'s safety-net loop drains OpenNodes / Nodes globally, and our - // parent is already iterating its own children and clearing childrenToVisit itself — - // re-running removeFromOpenNodes / Nodes.delete / removeChild here would double-remove and - // log spurious errors, or trigger a re-entrant parent.end(null, false) cascade in the middle - // of the ongoing iteration. - if (!force) { - if (this.suiteOrTest.title !== '') { - if (this.isTest) { - const { msgTitle, details } = this.getTestResult(result) ?? { msgTitle: 'testFinished', details: '' }; - console.log(`##teamcity[${msgTitle} name='${this.suiteOrTest.title}' ${details}]`); - } else { - console.log(`##teamcity[testSuiteFinished name='${this.suiteOrTest.title}']`); - } - console.log(`##teamcity[flowFinished flowId='${this.flowId}']`); - } - - this.removeFromOpenNodes(); - this.parent?.removeChild(this); - TestNode.Nodes.delete(this.id); - } + this.removeFromOpenNodes(); + this.parent?.removeChild(this); + TestNode.Nodes.delete(this.id); } private removeFromOpenNodes(): void { @@ -133,10 +113,6 @@ class TestNode { } private removeChild(child: TestNode) { - if (this.childrenCleared) { - // already cleared all children in a end(result, true) call, nothing to do - return; - } const childIndex = this.childrenToVisit.indexOf(child); if (childIndex < 0) { console.error(`Can't find child '${child.id}' in parent '${this.id}'`); @@ -149,7 +125,7 @@ class TestNode { } // No more children, so close this node - this.end(null, false); + this.end(null); } public static startTest(test: TestCase): void { @@ -174,7 +150,6 @@ class TestNode { if (this.childrenToVisit.length > 0) { console.error(`Root node still has ${this.childrenToVisit.length} open children`); } - this.end(null, true); if (TestNode.OpenNodes.length > 0) { console.error(`Still have ${TestNode.OpenNodes.length} open nodes`); while (TestNode.OpenNodes.length > 0) { From 81bae574b9187343e2a59cfd7bc9d45074ab5abe Mon Sep 17 00:00:00 2001 From: Eberhard Beilharz Date: Tue, 12 May 2026 18:14:46 +0200 Subject: [PATCH 4/5] maint(web): output name of TC reporter This change outputs the name of the TC reporter used for a test. This will help in troubleshooting. Build-bot: skip build:web Test-bot: skip --- common/test/resources/mocha-teamcity-reporter/teamcity.cjs | 2 ++ common/test/resources/playwright-TC-reporter.ts | 1 + common/test/resources/test-runner-TC-reporter.mjs | 6 ++++-- web/build.sh | 3 +++ 4 files changed, 10 insertions(+), 2 deletions(-) diff --git a/common/test/resources/mocha-teamcity-reporter/teamcity.cjs b/common/test/resources/mocha-teamcity-reporter/teamcity.cjs index 24c76a5f2b..fd901faaa1 100644 --- a/common/test/resources/mocha-teamcity-reporter/teamcity.cjs +++ b/common/test/resources/mocha-teamcity-reporter/teamcity.cjs @@ -135,6 +135,8 @@ function Teamcity(runner, options) { const ignoredTests = {}; const testState = { pending: 0 }; + log('Initializing Mocha TeamCity Reporter'); + runner.on(EVENT_SUITE_BEGIN, function (suite) { handleFlow(true, hasParentFlowId); if (suite.root) { diff --git a/common/test/resources/playwright-TC-reporter.ts b/common/test/resources/playwright-TC-reporter.ts index ad082dedf4..20d6ac39d4 100644 --- a/common/test/resources/playwright-TC-reporter.ts +++ b/common/test/resources/playwright-TC-reporter.ts @@ -178,6 +178,7 @@ export default class PlaywrightTeamcityReporter implements Reporter { private root!: TestNode; public constructor(options: { parentFlow?: string } = {}) { + console.log('Initializing Playwright TeamCity Reporter'); TestNode.RootFlow = options.parentFlow ?? 'unit_tests'; } diff --git a/common/test/resources/test-runner-TC-reporter.mjs b/common/test/resources/test-runner-TC-reporter.mjs index 8a6aeaccf2..33198592af 100644 --- a/common/test/resources/test-runner-TC-reporter.mjs +++ b/common/test/resources/test-runner-TC-reporter.mjs @@ -94,14 +94,16 @@ export default function teamcityReporter({ name="Web Test Runner JavaScript test /** @type {import('@web/test-runner').Reporter} */ const reporter = { - start({config, sessions}) { + start({ config, sessions }) { + logger = config.logger; + logger.log('Initializing Web Test Runner TeamCity Reporter'); + rootDir = config.rootDir; for(const session of sessions.all()) { testDefMap.set(buildSessionName(session), new Map()); } - logger = config.logger; logger.log(`##teamcity[blockOpened name='${e(name)}']`); }, stop(args) { diff --git a/web/build.sh b/web/build.sh index d047b7dece..ca253ab82c 100755 --- a/web/build.sh +++ b/web/build.sh @@ -205,12 +205,15 @@ function do_browser_tests() { pushd "${KEYMAN_ROOT}" if is_test_included dom; then + builder_echo "Running browser-based tests..." web-test-runner --config "web/src/test/auto/dom/web-test-runner${WTR_CONFIG}.config.mjs" ${WTR_INSPECT} fi if is_test_included integrated; then + builder_echo "Running integration tests..." web-test-runner --config "web/src/test/auto/integrated/web-test-runner${WTR_CONFIG}.config.mjs" ${WTR_INSPECT} fi if is_test_included e2e; then + builder_echo "Running end-to-end tests..." npx playwright test --config "web/src/test/auto/e2e/playwright.config.ts" fi popd From 6af638b9a8b94b5d9af89795fe2b027d90ec14d0 Mon Sep 17 00:00:00 2001 From: Eberhard Beilharz Date: Wed, 13 May 2026 11:33:49 +0200 Subject: [PATCH 5/5] maint(web): fix starting local dev server for e2e tests Starting the local dev server before running the e2e tests with `web/build.sh start` didn't work on Windows. This change now directly calls `node` to start the server which seems to work cross-platform. Fixes: #15895 Build-bot: skip build:web Test-bot: skip --- web/src/test/auto/e2e/playwright.config.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/web/src/test/auto/e2e/playwright.config.ts b/web/src/test/auto/e2e/playwright.config.ts index ad20aa9c15..ff888b5434 100644 --- a/web/src/test/auto/e2e/playwright.config.ts +++ b/web/src/test/auto/e2e/playwright.config.ts @@ -76,7 +76,10 @@ export default defineConfig({ /* Run your local dev server before starting the tests */ webServer: { - command: `"${KEYMAN_ROOT}/node_modules/.bin/gosh" "../../../../build.sh start"`, + // Normally we'd run `web/build.sh start` or `gosh web/build.sh start` + // here, but that's causing problems on Windows. + command: 'node web/src/tools/testing/test-server/index.cjs', + cwd: KEYMAN_ROOT, url: 'http://localhost:3000', reuseExistingServer: !process.env.KEYMAN_IS_CI_BUILD, },