From 98d776f48eef42ee1f929b36b96eb44ad2422312 Mon Sep 17 00:00:00 2001 From: Jochen Delabie Date: Thu, 1 Oct 2026 11:08:02 +0200 Subject: [PATCH] fix(maestro): decide flow failure from status and success, not error_messages (1.4.1) error_messages also carries stderr noise from flows that passed, such as Maestro's log4j 'Unable to write to stream ... maestro.log' at shutdown, so a passing run exited 2. Cancelled flows, and cancelled runs whose flow finished with success 1, still count as failed. --- CHANGELOG.md | 6 ++++ package-lock.json | 4 +-- package.json | 2 +- src/providers/maestro.ts | 20 ++++++----- tests/providers/maestro.test.ts | 60 +++++++++++++++++++++++++++------ 5 files changed, 71 insertions(+), 21 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 541000d..ac53cc5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,12 @@ All notable changes to `@testingbot/cli` are documented here. Releases are published to npm from GitHub releases. +## 1.4.1 - 2026-10-01 + +### Fixed + +- A Maestro flow that passed (`status: DONE`, `success: 1`) no longer counts as failed just because `error_messages` is non-empty. Maestro's log4j sometimes writes "Unable to write to stream ... maestro.log" to stderr at shutdown, and that turned passing runs into exit code 2. Pass/fail now comes from `status` and `success`, and cancelled flows and runs still fail. + ## 1.4.0 - 2026-09-05 ### Added diff --git a/package-lock.json b/package-lock.json index c51c4b6..2090612 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "@testingbot/cli", - "version": "1.4.0", + "version": "1.4.1", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "@testingbot/cli", - "version": "1.4.0", + "version": "1.4.1", "license": "MIT", "dependencies": { "archiver": "^7.0.1", diff --git a/package.json b/package.json index 276a282..aaeee87 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@testingbot/cli", - "version": "1.4.0", + "version": "1.4.1", "description": "CLI tool to run Espresso, XCUITest and Maestro tests on TestingBot's cloud infrastructure", "main": "dist/index.js", "bin": { diff --git a/src/providers/maestro.ts b/src/providers/maestro.ts index 6065a45..462853a 100644 --- a/src/providers/maestro.ts +++ b/src/providers/maestro.ts @@ -2647,11 +2647,16 @@ export default class Maestro extends BaseProvider { : text; } + /** + * Decided by status and success only. error_messages also carries stderr + * noise from flows that passed (e.g. Maestro's log4j complaining at + * shutdown), so its presence alone does not mean the flow failed. + */ private isFlowFailed(flow: MaestroFlowInfo): boolean { return ( (flow.status === 'DONE' && flow.success !== 1) || flow.status === 'FAILED' || - (flow.error_messages != null && flow.error_messages.length > 0) + flow.status === 'CANCELLED' ); } @@ -2670,6 +2675,8 @@ export default class Maestro extends BaseProvider { /** A run passes if every logical flow's latest attempt passed. */ private runPassed(run: MaestroRunInfo): boolean { + // A flow can finish with success 1 after its run was cancelled. + if (run.status === 'CANCELLED') return false; const groups = this.groupLatest(run.flows ?? []); if (groups.length === 0) return run.success === 1; return groups.every((flow) => !this.isFlowFailed(flow)); @@ -3137,7 +3144,9 @@ export default class Maestro extends BaseProvider { for (const flow of flows.slice().sort((a, b) => a.id - b.id)) { const display = this.getFlowStatusDisplay(flow); const errors = - flow.error_messages && flow.error_messages.length > 0 + this.isFlowFailed(flow) && + flow.error_messages && + flow.error_messages.length > 0 ? pc.red(` ${flow.error_messages[0]}`) : ''; console.log( @@ -3247,12 +3256,7 @@ export default class Maestro extends BaseProvider { } private hasAnyFlowFailed(flows: MaestroFlowInfo[]): boolean { - return flows.some( - (flow) => - (flow.status === 'DONE' && flow.success !== 1) || - flow.status === 'FAILED' || - (flow.error_messages && flow.error_messages.length > 0), - ); + return flows.some((flow) => this.isFlowFailed(flow)); } private calculateFlowDuration(flow: MaestroFlowInfo): string { diff --git a/tests/providers/maestro.test.ts b/tests/providers/maestro.test.ts index 6ed4f1a..405de55 100644 --- a/tests/providers/maestro.test.ts +++ b/tests/providers/maestro.test.ts @@ -5277,7 +5277,7 @@ flows: errorSpy.mockRestore(); }); - it('should treat flows with error_messages as failed even if success is 1', async () => { + it('should not fail a flow with success 1 just because error_messages is set', async () => { const consoleSpy = jest.spyOn(console, 'log').mockImplementation(); const errorSpy = jest.spyOn(logger, 'error').mockImplementation(); @@ -5288,27 +5288,30 @@ flows: id: 5678, status: 'DONE', capabilities: { deviceName: 'Pixel 9', platformName: 'Android' }, - success: 0, + success: 1, flows: [ { id: 1, - name: 'flaky.yaml', + name: 'login.yaml', status: 'DONE', success: 1, - error_messages: ['Element not found'], + error_messages: [ + '2026-10-01T07:37:29.468224744Z Thread-5 ERROR Unable to write to stream /home/testingbot/.maestro/tests/2026-10-01_072332/maestro.log for appender File\n', + ], }, ], }, ], - success: false, + success: true, completed: true, }, }; axios.get = jest.fn().mockResolvedValue(responseFlowWithErrors); - await maestro['waitForCompletion'](); + const result = await maestro['waitForCompletion'](); - expect(errorSpy).toHaveBeenCalledWith('1 flow(s) failed across 1 run(s)'); + expect(result.success).toBe(true); + expect(errorSpy).not.toHaveBeenCalled(); consoleSpy.mockRestore(); errorSpy.mockRestore(); @@ -5394,14 +5397,15 @@ flows: expect(result).toBe(true); }); - it('should return true when a flow has error_messages', () => { + it('should return true when a flow was cancelled', () => { const flows: MaestroFlowInfo[] = [ { id: 1, name: 'flow1.yaml', status: 'DONE', success: 1 }, { id: 2, name: 'flow2.yaml', - status: 'READY', - error_messages: ['Error occurred'], + status: 'CANCELLED', + success: 0, + error_messages: ['Cancelled by user'], }, ]; @@ -5410,6 +5414,22 @@ flows: expect(result).toBe(true); }); + it('should return false when a passed flow has error_messages', () => { + const flows: MaestroFlowInfo[] = [ + { + id: 1, + name: 'flow1.yaml', + status: 'DONE', + success: 1, + error_messages: ['Thread-5 ERROR Unable to write to stream'], + }, + ]; + + const result = maestro['hasAnyFlowFailed'](flows); + + expect(result).toBe(false); + }); + it('should return false when all flows passed', () => { const flows: MaestroFlowInfo[] = [ { id: 1, name: 'flow1.yaml', status: 'DONE', success: 1 }, @@ -6914,6 +6934,26 @@ onFlowStart: ); }); + it('status() reports a cancelled run as failed even if its flow finished with success 1', async () => { + const flows = [ + { + id: 1, + name: 'login', + status: 'DONE', + success: 1, + error_messages: ['Cancelled by user'], + }, + ]; + maestro['getStatus'] = jest.fn().mockResolvedValue({ + runs: [run({ status: 'CANCELLED', success: 0, flows })], + success: false, + completed: true, + }); + const result = await maestro.status(1234); + expect(result.outcome).toBe('failed'); + expect(result.success).toBe(false); + }); + it('status() reports failed using last-attempt-wins', async () => { const flows = [ { id: 1, name: 'login', status: 'DONE', success: 1 },