Conversation
Size Report
Startup median (7 runs, lower is better):
|
|
Re the review on #2368: the replacement now has live assertion-path evidence on exact head The 11 focused tests and exact-head |
|
The code looks right, but the capture path is not yet proven on a real run at c1cb496. The changed route (runScenario's catch → recordScenarioFailure → captureFailedStepEvidence, https://github.com/callstack/agent-device/blob/c1cb496/test/integration/live-device-e2e/runtime.ts#L130) is exercised by the platform Smoke Tests jobs: ios.yml's smoke-ios job runs smoke-ios-simulator.test.ts → runIosSimulatorE2E → harness.runScenario, and android.yml/linux.yml follow the same chain. No CI run or uploaded artifact yet shows this exact head's capture path firing on a real device; the manual claim in the PR is unlinked and the author himself flags it as insufficient. Merging without that artifact leaves the device-facing capture behavior verified only by reading the code, not by seeing it run. Please wait for Smoke Tests to finish on c1cb496 and link the uploaded test/artifacts/** evidence (failed-step-N.png, failed-step-N-snapshot.json, failed-step.txt) from a run that reaches a real scenario assertion, closing the gap the #2368 review asked about. Smoke Tests on c1cb496 route through this same runtime.ts, so a failure there would overlap this PR's diff, but the run was still queued or in progress at review time, so there is nothing to attribute yet. I could not check out the PR branch to run the new test file myself, and I could not confirm the author's manual iOS 26.2 simulator session (file sizes, object-identity check, cleanup) since no artifact or CI link was given for it. The next thing needed before merge is Smoke Tests completing on c1cb496 with an uploaded test/artifacts/** capture from a run that reaches a scenario assertion, per the outstanding #2368 request. |
c1cb496 to
6920816
Compare
|
Following up on the artifact request: I rebased onto the main that includes #2896 and #2919 and added a permanent live iOS assertion canary at The exact-head local gate passed ( |
|
This is a follow-up on the earlier review (c1cb496). The earlier round asked for an uploaded test/artifacts/** capture from a real Smoke Tests run reaching a scenario assertion on this head before merge. The delta at live-runner.ts:99 adds a deterministic canary, Not blocking: could the dependency of CI is pending: ios.yml's Smoke Tests job runs test/integration/smoke-ios-simulator.test.ts through runIosSimulatorE2E and executeLiveScenarios, the exact function this delta changed, and that job was still in progress at review time with no result yet to attribute. I could not observe that run complete, so I have no uploaded ios-artifacts link confirming the canary passes on real hardware or that failed-step-N.png, failed-step-N-snapshot.json, and failed-step.txt are produced as designed. I did not independently run the local gate or the unit tests the author cites; I took the author's local-pass claim on faith since it covers non-device-facing unit coverage, not the required live evidence. The next thing that has to happen before merge is the Smoke Tests run on 6920816 finishing green with that artifact linked. |
|
The requested exact-head live proof is now available. iOS Smoke Tests on I downloaded the run's ios-artifacts archive and verified the same smoke directory contains For the nonblocking ordering note: the canary's |
6920816 to
15b7d9d
Compare
|
I rebased #2921 onto The 6.84s live canary and uploaded evidence are from the prior head |
|
The code looks right at 15b7d9d, but merge readiness still comes down to one thing: live proof on this exact head. The only uploaded live-canary evidence in this thread (PNG, snapshot JSON, failed-step.txt from a real iOS session) is linked from run 36031511215 on the prior head 6920816. That head is gone after the rebase. 15b7d9d rebases onto fa1082d, which pulls in changes to the same CLI capture path this canary walks (screenshot/snapshot output shape, wait/timeout policy) in https://github.com/callstack/agent-device/blob/15b7d9d/test/integration/ios-simulator-e2e/live-runner.ts#L115. No completed iOS Smoke Tests run and no uploaded test/artifacts/** exist yet for 15b7d9d, so we don't yet know that assertLiveAssertionCapture still fires correctly on real hardware after the rebase. Merging without that would repeat the exact gap the last two review rounds asked to close. Android, Linux, and macOS Smoke Tests are already green on 15b7d9d. The iOS Smoke Tests job (run 36037593196, job 107761446770) is still in progress; it runs runIosSimulatorE2E -> executeLiveScenarios -> assertLiveAssertionCapture, the function this PR adds, so it's the one check that matters here. The path is clear once that job finishes: link the completed, green iOS Smoke Tests run for exact head 15b7d9d, with its uploaded ios-artifacts showing failed-step-N.png, failed-step-N-snapshot.json, and failed-step.txt from the canary. I didn't re-run the unit/integration suite myself for these files; I relied on the author's stated pnpm check:affected pass and confirmed the diff is unchanged from the already-reviewed 6920816 head. I also didn't read the merged upstream capture-path commits line by line, only confirmed they touch the same route, which is why we need the live run rather than another static pass. |
|
The exact-head iOS job on I downloaded the uploaded |
|
The rerun completed the missing live proof on exact head I downloaded that attempt's The rerun waited 34m31s for a runner and executed for 10m38s. The earlier attempt's Settings replay transport failure remains documented above; it never reached the canary. |
|
The live proof for 15b7d9d is now complete. iOS Smoke Tests attempt 2 ran the fixture E2E on this exact head, and the uploaded canary files show the PNG, the snapshot JSON, and the failure report. The earlier code review had no open findings, so this is ready for human review. |
Summary
Capture a screenshot, snapshot, and device facts when a live E2E scenario assertion fails after allowed CLI misses. Preserve the original error and avoid recapturing a command failure. The iOS smoke lane deliberately exercises the assertion route after a real session opens and checks its evidence. Successful product scenarios are unchanged.
This refreshes #2368 on current
main; the original implementation commit retains @billsbooth as author. Three test-harness files changed. No workflow expansion.Validation
Rebased head
15b7d9dcb5onfa1082dd:pnpm install --frozen-lockfile,pnpm build,pnpm format, focused harness tests (7/7), andAGENT_DEVICE_VITEST_MAX_WORKERS=1 pnpm check:affected --runpassed. The previous-head iOS run at6920816c1dpassed and logged a 6.84s canary; its uploaded archive contains the PNG, successful snapshot JSON, and deliberate-assertion report. The new exact-head iOS job and renewed uploaded artifact proof are pending.