From 73a0dd53181d8a52a77430eaf88af00e2f0d72be Mon Sep 17 00:00:00 2001 From: Eberhard Beilharz Date: Wed, 20 May 2026 18:10:04 +0200 Subject: [PATCH 1/3] 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 84f3eede0d0..c80e73e1806 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/3] 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 c80e73e1806..a6bfeb96d33 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/3] 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 a6bfeb96d33..ad082dedf48 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) {