[build-tools] Allow running Maestro tests with maestro-runner - #4187
[build-tools] Allow running Maestro tests with maestro-runner#4187sjchmiela wants to merge 6 commits into
maestro-runner#4187Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4187 +/- ##
==========================================
+ Coverage 63.53% 63.63% +0.10%
==========================================
Files 1028 1028
Lines 47033 47197 +164
Branches 9884 9934 +50
==========================================
+ Hits 29879 30029 +150
- Misses 17053 17067 +14
Partials 101 101 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
5f3a29b to
6d821dd
Compare
f2d4996 to
a594330
Compare
sjchmiela
changed the title
61db12a to
f2afd26
Compare
f2afd26 to
de87b9c
Compare
de87b9c to
65414f2
Compare
65414f2 to
39e37ff
Compare
39e37ff to
40892df
Compare
40892df to
ff2750d
Compare
ff2750d to
b20c996
Compare
sjchmiela
changed the title
maestro-runner
10d0c3f to
069a3c1
Compare
| const backend = resolveMaestroBackend({ | ||
| input: inputs.backend.value, | ||
| env, | ||
| }); |
There was a problem hiding this comment.
resolveMaestroBackend can throw before the required outputs are published. If the input or EAS_MAESTRO_BACKEND is invalid, the downstream if: always() upload step may fail to interpolate those outputs and obscure the original error.
This seems to be exactly what the “Outputs are published BEFORE any throw below” comment is meant to prevent. Nothing before the output assignments uses backend, so could we move this call below them?
Non-blocking—just moving the call should be enough.
| case 'maestro-runner': { | ||
| const results = await Promise.all( | ||
| reportDirectories.map(directory => parseMaestroRunnerReport(directory)) | ||
| ); | ||
| flowResults = results.some(result => result === null) | ||
| ? null | ||
| : results.flatMap(result => result?.flows ?? []); | ||
| break; | ||
| } | ||
| } | ||
| if (flowResults === null) { | ||
| logger.warn('Failed to classify failure screenshots; skipping upload.'); | ||
| return; |
There was a problem hiding this comment.
Runner screenshot classification is currently all-or-nothing: if any attempt’s report.json cannot be parsed, all previously harvested screenshots are skipped.
For example, an earlier attempt could fail normally and collect screenshots, then the final retry crashes before writing report.json, causing all of them to be discarded. The Maestro path degrades and still uploads, so the comment above about fully failed runs still uploading screenshots does not hold for the runner path.
Could we use the reports that can be parsed, or fall back to uploading everything? The 30-screenshot cap should keep that bounded.
Non-blocking, but I think it’s worth addressing in this PR.
| let report: { | ||
| flows?: { name?: string; status?: string; dataFile?: string }[]; | ||
| }; | ||
| try { | ||
| report = JSON.parse(await fs.readFile(path.join(args.reportDirectory, 'report.json'), 'utf8')); | ||
| } catch (err: any) { | ||
| args.logger.info( | ||
| { err }, | ||
| `Skipping maestro-runner screenshot harvest: cannot read ${args.reportDirectory}.` | ||
| ); | ||
| return []; | ||
| } | ||
|
|
||
| const shots: HarvestedScreenshot[] = []; | ||
| for (const flow of report.flows ?? []) { | ||
| if (flow.status !== 'failed' || !flow.name || !flow.dataFile) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
I don’t think the “Never throws” guarantee currently holds because report.flows is accessed outside the try.
Valid JSON with an unexpected shape, such as null, { "flows": 5 }, or { "flows": [null] }, can throw here. Since the caller does not catch it, a screenshot issue could fail the step and obscure the test result.
Could we move the loop into the try or validate the shape before iterating?
Non-blocking, small fix.
|
On the “maybe it’s not so bad?” point: I think the current structure is fine for now. If we decide to support both backends long-term, I’d prefer to keep the overall flow shared and resolve the backend-specific runtime once before the loop, instead of adding backend checks throughout. The divergence is still small, but there are already some signs of it: the runner path drops all screenshots if any report is unusable while the Maestro path degrades, and the retry logic has an unreachable branch just for TypeScript narrowing. Not something we need to do in this PR, but I’d prefer to refactor before the two paths diverge further. |
hSATAC
left a comment
hSATAC
left a comment
There was a problem hiding this comment.
One more non-blocking comment.
| @@ -82,6 +88,17 @@ function parseJUnitContent(content: string): JUnitTestCaseResult[] { | |||
| : (tc.error?.['#text'] ?? null) | |||
| : null; | |||
| const errorMessage: string | null = failureText ?? errorText ?? null; | |||
There was a problem hiding this comment.
From junit.go’s failure element construction, the <failure> message attribute contains the actual error from entry.Error, while resolveFailure puts the command label/type in the body.
This currently reads #text, so the runner’s errorMessage may be something like tapOn instead of the actual element not found: ... error, which will also be exposed through the API and website.
Could we prefer @_message and fall back to #text? Official Maestro only uses the body, so its behavior should remain unchanged.
Non-blocking.
- Resolve the maestro backend after publishing outputs so an invalid backend input can't throw before `if: always()` upload steps can interpolate them. - Keep runner failure screenshots from attempts that reported when a later attempt's report.json is unparseable, instead of discarding all of them. - Guard the runner screenshot report shape so a malformed report.json can't throw out of the harvest that promises never to throw. - Prefer the JUnit `<failure>`/`<error>` message attribute over the body so the runner's real error surfaces instead of the command label. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
3a0293f to
f48b2eb
Compare
Replace the hand-rolled report shape check and the flow-detail `as` cast with zod schemas: a recursive command schema (zod v4 getter) drives the flow-detail parse, and a tolerant report schema yields no flows on an unexpected shape instead of throwing. Add regression cases for malformed report.json. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…es to Sentry The zod schemas are expected to always match maestro-runner's output, so a validation failure is an anomaly worth surfacing: capture it in Sentry (still without throwing) for both report.json and the per-flow detail file. Drop the verbose schema comments. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR extends the eas/maestro_tests build-step function in @expo/build-tools to support running Maestro tests via an alternate backend (maestro-runner) selected by EAS_MAESTRO_BACKEND and/or a step input. It updates the execution, retry subsetting, and artifact/screenshot parsing paths to accommodate maestro-runner’s output formats while keeping existing Maestro behavior intact.
Changes:
- Added backend selection (
maestrovsmaestro-runner) and wired it into spawning, retry logic, and logs. - Added
maestro-runner-specific parsing forreport.json(failed flows + flow results) and screenshot harvesting. - Expanded Jest coverage for backend selection, retry behavior, and the new parsers/harvesters.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/build-tools/src/steps/functions/maestroTests.ts | Adds backend selection, runner-specific execution/output handling, and integrates runner parsing/harvesting. |
| packages/build-tools/src/steps/functions/maestroScreenshots.ts | Implements maestro-runner screenshot harvesting from report.json bundles. |
| packages/build-tools/src/steps/functions/maestroResultParser.ts | Extends JUnit parsing for runner differences and adds report.json parsing helpers. |
| packages/build-tools/src/steps/functions/tests/maestroTests.test.ts | Adds tests covering backend selection, runner invocation, retry subsetting, and runner artifact handling. |
| packages/build-tools/src/steps/functions/tests/maestroScreenshots.test.ts | Adds tests for harvesting runner failure screenshots from report bundles. |
| packages/build-tools/src/steps/functions/tests/maestroResultParser.test.ts | Adds tests for report.json parsing + runner JUnit file-property handling. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| function resolvePathInsideDirectory(directory: string, relativePath: string): string | null { | ||
| const candidate = path.resolve(directory, relativePath); | ||
| const relative = path.relative(directory, candidate); | ||
| return relative === '' || (!relative.startsWith(`..${path.sep}`) && !path.isAbsolute(relative)) | ||
| ? candidate |
There was a problem hiding this comment.
Good catch — fixed in 9064bbb. resolvePathInsideDirectory now also rejects an exact .. (relative !== '..'), so a report-provided path that resolves to the parent directory is treated as outside. Added a regression test feeding a screenshot artifact of ...
| // maestro-runner writes its JUnit report and screenshot metadata to this directory. | ||
| const runnerOutputDirectory = path.join( | ||
| testsDirectory, | ||
| `${platform}-maestro-runner-attempt-${attempt}` | ||
| ); |
There was a problem hiding this comment.
Fixed in 9064bbb — the runner output directory is now cleared best-effort (fs.rm(dir, { recursive: true, force: true })) before each attempt's spawn, so a crash before it writes fresh output can't resurrect stale results. Added a test asserting the clear happens before the spawn.
| const errorMessage: string | null = failureText ?? errorText ?? null; | ||
| // Official Maestro uses status="SUCCESS". maestro-runner uses standard JUnit semantics: | ||
| // a testcase passes when it has no failure or error child. | ||
| const statusAttribute = tc['@_status']; | ||
| const status: 'passed' | 'failed' = | ||
| typeof statusAttribute === 'string' |
There was a problem hiding this comment.
Fixed in 9064bbb — parseJUnitContent now excludes <skipped/> testcases instead of counting them as passed, matching the report.json path which already drops skipped flows. Added a test.
Note this mainly hardens the official-Maestro JUnit path; the maestro-runner path classifies off report.json, whose schema already filters status: 'skipped'. Excluding skipped is the correct, consistent behavior either way.
- Reject an exact `..` in resolvePathInsideDirectory, which previously resolved to the parent directory and slipped past the containment guard. - Clear the deterministic maestro-runner output directory before each attempt so a crash before it writes fresh output can't resurrect stale results. - Exclude skipped JUnit testcases (<skipped/>) instead of counting them as passed, matching the report.json path which already drops skipped flows. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
⏩ The changelog entry check has been skipped since the "no changelog" label is present. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (1)
packages/build-tools/src/steps/functions/maestroTests.ts:238
- backend resolution currently allows an explicit empty string (e.g. a workflow interpolation that resolves to "") to be treated as “not provided”, which can silently fall back to EAS_MAESTRO_BACKEND/default instead of failing validation. This makes misconfiguration harder to detect and can unexpectedly change which binary is invoked.
const backend = resolveMaestroBackend({
input: inputs.backend.value,
env,
});
Why
maestro-runnerpromises to be faster, more reliable alternative to the official Maestro CLI.How
Added support for
EAS_MAESTRO_BACKENDand/or input toeas/maestro_testswhich opts in to usingmaestro-runnerovermaestrocommand.maestro-runnerdoesn't support DADB, but we don't error if both are enabled.maestro-runnerhas slightly different screenshots and JUnit format -- both are handled. It also exposes a nicer JSON file that lets us parse failed flows without having to parse JUnit file.This adds relatively big chunk of code, but it's all guarded by
backend === maestro-runnerso maybe it's not so bad? I considered splitting logic intoMaestroCliUtilsandMaestroRunnerUtilsand expose similar interfaces from them, but I feel it was too complicated.Test Plan
report.jsonselected onlyflows/mixed/fail.ymlfor attempt 2.