From a9307d82025eef5ab3eb7dfc3d43e05cc12366ce Mon Sep 17 00:00:00 2001 From: Juliusz Wajgelt Date: Thu, 17 Sep 2026 08:09:56 -0700 Subject: [PATCH] Split the iOS E2E template-app workflow and harden the Maestro runner (#58484) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Summary: This PR separates the template app's iOS build from its Maestro test run and updates the shared iOS runner: - Extract the existing template-app build steps into a `build` job. The project initialization, CocoaPods setup, and `xcodebuild` commands come from the previous combined `test` job; `Prepare artifacts` is renamed to `Build the app`. - Add a separate `test` job that depends on `build` and runs the Debug and Release apps. It repeats the checkout, Node, and Yarn setup from the old job and moves the existing Maestro execution and status reporting into this job. Both jobs keep `macos-26-large`. - Upload the built apps as artifacts and download them in the test job. Enable artifact replacement so the existing workflow retries can upload rebuilt apps under the same names. - Add `Prepare project for Metro` for Debug tests. Its package lookup, template-branch selection, and `init-project-e2e.js` invocation are copied from the existing `Prepare artifacts` step. The new job has a fresh filesystem, so it needs to recreate the JavaScript project for Metro; this step only repeats project initialization, leaving CocoaPods and the native build in the build job. - Update Maestro's `app-path` to point to the downloaded `RNTestProject.app`, replacing the path to the previous job's local Xcode build output. - Pass the selected simulator's UDID to app installation and video recording instead of using `booted`, so both target the same simulator as Maestro. - Make flow execution asynchronous and await the recorder's exit after sending SIGINT, so the video finishes writing and Node processes the child exit before the next flow starts. Send SIGKILL if the recorder has not exited within 30 seconds. (The missing wait predates this PR. Recordings were already uploaded as artifacts, but CI did not validate the videos, so recording problems could go unnoticed. [https://github.com/react/react-native/issues/57749](https://github.com/react/react-native/pull/57749) introduced a separate recorder per flow, reusing the same filenames, which allows the next recording to start before the previous one finishes.) - Skip `helpers/` when collecting flows, since these files are fragments included by other tests through `runFlow`. - Update unit tests to await flow execution and simulate recorder exit events. Add coverage for waiting on the recorder, skipping helpers, and reporting failure after all retries are exhausted. ## Changelog: [INTERNAL] [CHANGED] - Separate iOS template-app E2E builds from test execution and improve Maestro runner reliability. Pull Request resolved: https://github.com/react/react-native/pull/58484 Test Plan: - Author-reported E2E validation used `macos-26` instead of the checked-in `macos-26-large`: RNTester passed 38/38 flows in both Debug and Release, and the template app passed in both configurations. These runs were not repeated during this review. - Runner unit tests with the repository's Jest configuration: **8/8 passed** — `yarn jest .github/workflow-scripts/__tests__/maestro-ios-test.js`. - `node --check .github/workflow-scripts/maestro-ios.js` — passed. - Prettier check on the updated unit tests and workflow — passed. - `git diff --check` — passed. - E2E with the local fixes and artifact replacement during a GitHub Actions retry were not run during this review. Reviewed By: christophpurrer Differential Revision: D120135261 Pulled By: cipolleschi fbshipit-source-id: 8cd04d8a034d595d14d380831b418183ea697feb --- .../__tests__/maestro-ios-test.js | 97 +++++++++++++++++-- .github/workflow-scripts/maestro-ios.js | 96 ++++++++++++++---- .github/workflows/e2e-ios-templateapp.yml | 65 ++++++++++++- 3 files changed, 230 insertions(+), 28 deletions(-) diff --git a/.github/workflow-scripts/__tests__/maestro-ios-test.js b/.github/workflow-scripts/__tests__/maestro-ios-test.js index 8b23a47a341..f5f9686e4e0 100644 --- a/.github/workflow-scripts/__tests__/maestro-ios-test.js +++ b/.github/workflow-scripts/__tests__/maestro-ios-test.js @@ -18,24 +18,33 @@ jest.mock('fs', () => ({ })); const childProcess = require('child_process'); +const {EventEmitter} = require('events'); const fs = require('fs'); const {executeFlows, findAvailableSimulator} = require('../maestro-ios'); describe('Maestro iOS runner', () => { beforeEach(() => { - jest.clearAllMocks(); - childProcess.spawn.mockReturnValue({pid: 1, kill: jest.fn()}); + jest.resetAllMocks(); + childProcess.spawn.mockImplementation(() => { + const recordingProcess = new EventEmitter(); + recordingProcess.pid = 1; + recordingProcess.kill = jest.fn(() => { + recordingProcess.emit('exit', 0, null); + return true; + }); + return recordingProcess; + }); }); - it('executes each YAML flow separately and skips other files', () => { + it('executes each YAML flow separately and skips other files', async () => { fs.existsSync.mockReturnValue(true); fs.lstatSync.mockImplementation(path => ({ isDirectory: () => path === 'flows/', })); fs.readdirSync.mockReturnValue(['second.yaml', 'image.png', 'first.yml']); - executeFlows('com.example', 'device-id', 'flows/', 'Hermes'); + await executeFlows('com.example', 'device-id', 'flows/', 'Hermes'); expect(childProcess.execSync).toHaveBeenCalledTimes(2); expect(childProcess.execSync.mock.calls[0][0]).toContain( @@ -46,13 +55,13 @@ describe('Maestro iOS runner', () => { ); }); - it('retries only the failing flow', () => { + it('retries only the failing flow', async () => { fs.existsSync.mockReturnValue(false); childProcess.execSync.mockImplementationOnce(() => { throw new Error('Maestro driver failed'); }); - executeFlows('com.example', 'device-id', 'flow.yml', 'Hermes'); + await executeFlows('com.example', 'device-id', 'flow.yml', 'Hermes'); expect(childProcess.execSync).toHaveBeenCalledTimes(2); for (const call of childProcess.execSync.mock.calls) { @@ -60,6 +69,82 @@ describe('Maestro iOS runner', () => { } }); + it('waits for the recorder to exit before starting the next flow', async () => { + fs.existsSync.mockReturnValue(true); + fs.lstatSync.mockImplementation(path => ({ + isDirectory: () => path === 'flows/', + })); + fs.readdirSync.mockReturnValue(['first.yml', 'second.yml']); + + const recordingProcess = new EventEmitter(); + recordingProcess.pid = 1; + recordingProcess.kill = jest.fn(() => true); + childProcess.spawn.mockReturnValueOnce(recordingProcess); + + const execution = executeFlows( + 'com.example', + 'device-id', + 'flows/', + 'Hermes', + ); + + await new Promise(resolve => + jest.requireActual('timers').setImmediate(resolve), + ); + + expect(recordingProcess.kill).toHaveBeenCalledWith('SIGINT'); + expect(childProcess.execSync).toHaveBeenCalledTimes(1); + expect(childProcess.spawn).toHaveBeenCalledTimes(1); + + recordingProcess.emit('exit', 0, null); + await execution; + + expect(childProcess.execSync).toHaveBeenCalledTimes(2); + expect(childProcess.spawn).toHaveBeenCalledTimes(2); + }); + + it('skips helper directories while recursing into flow directories', async () => { + fs.existsSync.mockReturnValue(true); + fs.lstatSync.mockImplementation(path => ({ + isDirectory: () => !path.endsWith('.yml'), + })); + fs.readdirSync.mockImplementation(path => + path === 'flows/' ? ['helpers', 'nested'] : ['flow.yml'], + ); + + await executeFlows('com.example', 'device-id', 'flows/', 'Hermes'); + + expect(fs.readdirSync).not.toHaveBeenCalledWith('flows/helpers'); + expect(childProcess.execSync).toHaveBeenCalledTimes(1); + expect(childProcess.execSync.mock.calls[0][0]).toContain( + 'test "flows/nested/flow.yml"', + ); + }); + + it('rejects after exhausting retries and stops every recorder', async () => { + const consoleError = jest + .spyOn(console, 'error') + .mockImplementation(() => {}); + fs.existsSync.mockReturnValue(false); + const error = new Error('Maestro driver failed'); + childProcess.execSync.mockImplementation(() => { + throw error; + }); + + await expect( + executeFlows('com.example', 'device-id', 'flow.yml', 'Hermes'), + ).rejects.toBe(error); + + expect(childProcess.execSync).toHaveBeenCalledTimes(5); + expect(childProcess.spawn).toHaveBeenCalledTimes(5); + for (const {value: recordingProcess} of childProcess.spawn.mock.results) { + expect(recordingProcess.kill).toHaveBeenCalledWith('SIGINT'); + } + expect(consoleError).toHaveBeenCalledWith( + 'Failed to execute flow flow.yml after 5 attempts.', + ); + }); + it('selects an iPhone Pro simulator from the latest runtime', () => { childProcess.execSync.mockReturnValue( JSON.stringify({ diff --git a/.github/workflow-scripts/maestro-ios.js b/.github/workflow-scripts/maestro-ios.js index 02ed4a08d5d..a9782ec6506 100644 --- a/.github/workflow-scripts/maestro-ios.js +++ b/.github/workflow-scripts/maestro-ios.js @@ -76,9 +76,9 @@ function launchSimulator(simulator) { } } -function installAppOnSimulator(appPath) { +function installAppOnSimulator(appPath, udid) { console.log(`Installing app at path ${appPath}`); - childProcess.execSync(`xcrun simctl install booted "${appPath}"`); + childProcess.execSync(`xcrun simctl install "${udid}" "${appPath}"`); } function bringSimulatorInForeground() { @@ -102,13 +102,13 @@ async function launchAppOnSimulator(appId, udid, isDebug) { } } -function startVideoRecording(jsengine, currentAttempt) { +function startVideoRecording(udid, currentAttempt) { console.log( `Start video record using pid: video_record_${currentAttempt}.pid`, ); const recordingArgs = - `simctl io booted recordVideo --force video_record_${currentAttempt}.mov`.split( + `simctl io ${udid} recordVideo --force video_record_${currentAttempt}.mov`.split( ' ', ); const recordingProcess = childProcess.spawn('xcrun', recordingArgs, { @@ -119,19 +119,70 @@ function startVideoRecording(jsengine, currentAttempt) { return recordingProcess; } +// The movie is only written after SIGINT, so returning early truncates it. +const RECORDING_SHUTDOWN_TIMEOUT_MS = 30 * 1000; + function stopVideoRecording(recordingProcess) { if (!recordingProcess) { console.log("Passed a null recording process. Can't kill it"); - return; + return Promise.resolve(); } console.log(`Stop video record using pid: ${recordingProcess.pid}`); - recordingProcess.kill('SIGINT'); + if ( + recordingProcess.exitCode != null || + recordingProcess.signalCode != null + ) { + return Promise.resolve(); + } + + // Awaiting the exit is also what reaps the child: the flows run in a + // synchronous loop, so nothing else turns the event loop. + return new Promise(resolve => { + let settled = false; + const done = () => { + if (settled) { + return; + } + settled = true; + clearTimeout(timer); + resolve(); + }; + const timer = setTimeout(() => { + console.log( + `Recorder ${recordingProcess.pid} did not exit in time, killing it`, + ); + recordingProcess.kill('SIGKILL'); + // SIGKILL on an already-exited process is a no-op that emits no `exit`, + // so resolve here instead of waiting for an event that may never come. + done(); + }, RECORDING_SHUTDOWN_TIMEOUT_MS); + timer.unref?.(); + + recordingProcess.once('exit', done); + recordingProcess.once('error', done); + recordingProcess.kill('SIGINT'); + + // The process may have exited between the guard above and attaching the + // listeners; that `exit` event is already gone, so re-check and resolve. + if ( + recordingProcess.exitCode != null || + recordingProcess.signalCode != null + ) { + done(); + } + }); } -function executeFlowWithRetries(appId, udid, flow, jsengine, currentAttempt) { - const recProcess = startVideoRecording(jsengine, currentAttempt); +async function executeFlowWithRetries( + appId, + udid, + flow, + jsengine, + currentAttempt, +) { + const recProcess = startVideoRecording(udid, currentAttempt); try { const timeout = 1000 * 60 * 10; // 10 minutes const command = `$HOME/.maestro/bin/maestro --udid="${udid}" test "${flow}" --format junit -e APP_ID="${appId}"`; @@ -142,13 +193,19 @@ function executeFlowWithRetries(appId, udid, flow, jsengine, currentAttempt) { timeout, }); - stopVideoRecording(recProcess); + await stopVideoRecording(recProcess); } catch (error) { - stopVideoRecording(recProcess); + await stopVideoRecording(recProcess); if (currentAttempt < MAX_ATTEMPTS) { console.info(`Retrying flow: ${flow}`); - executeFlowWithRetries(appId, udid, flow, jsengine, currentAttempt + 1); + await executeFlowWithRetries( + appId, + udid, + flow, + jsengine, + currentAttempt + 1, + ); } else { console.error( `Failed to execute flow ${flow} after ${MAX_ATTEMPTS} attempts.`, @@ -158,18 +215,23 @@ function executeFlowWithRetries(appId, udid, flow, jsengine, currentAttempt) { } } -function executeFlows(appId, udid, maestroFlow, jsengine) { +async function executeFlows(appId, udid, maestroFlow, jsengine) { if (!fs.existsSync(maestroFlow) || !fs.lstatSync(maestroFlow).isDirectory()) { - executeFlowWithRetries(appId, udid, maestroFlow, jsengine, 1); + await executeFlowWithRetries(appId, udid, maestroFlow, jsengine, 1); return; } for (const file of fs.readdirSync(maestroFlow).sort()) { const filePath = `${maestroFlow.replace(/\/$/, '')}/${file}`; if (fs.lstatSync(filePath).isDirectory()) { - executeFlows(appId, udid, filePath, jsengine); + // Fragments pulled in via `runFlow`; they have no `launchApp` of their + // own and fail when run standalone. + if (file === 'helpers') { + continue; + } + await executeFlows(appId, udid, filePath, jsengine); } else if (file.endsWith('.yml') || file.endsWith('.yaml')) { - executeFlowWithRetries(appId, udid, filePath, jsengine, 1); + await executeFlowWithRetries(appId, udid, filePath, jsengine, 1); } } } @@ -202,10 +264,10 @@ async function main(args = process.argv.slice(2)) { const simulator = findAvailableSimulator(deviceModel, deviceOS); launchSimulator(simulator); - installAppOnSimulator(appPath); + installAppOnSimulator(appPath, simulator.udid); bringSimulatorInForeground(); await launchAppOnSimulator(appId, simulator.udid, isDebug); - executeFlows(appId, simulator.udid, maestroFlow, jsengine); + await executeFlows(appId, simulator.udid, maestroFlow, jsengine); console.log('Test finished'); } diff --git a/.github/workflows/e2e-ios-templateapp.yml b/.github/workflows/e2e-ios-templateapp.yml index 20e5a9c5dff..7ef8845069c 100644 --- a/.github/workflows/e2e-ios-templateapp.yml +++ b/.github/workflows/e2e-ios-templateapp.yml @@ -15,10 +15,8 @@ on: value: ${{ jobs.report.outputs.status }} jobs: - test: + build: runs-on: macos-26-large - outputs: - status: ${{ steps.report-status.outputs.status }} strategy: fail-fast: false matrix: @@ -62,7 +60,7 @@ jobs: run: | git config --global user.email "react-native-bot@meta.com" git config --global user.name "React Native Bot" - - name: Prepare artifacts + - name: Build the app run: | REACT_NATIVE_PKG=$(find /tmp/react-native-tmp -type f -name "*.tgz") echo "React Native tgs is $REACT_NATIVE_PKG" @@ -92,12 +90,69 @@ jobs: -sdk "iphonesimulator" \ -destination "generic/platform=iOS Simulator" \ -derivedDataPath "/tmp/RNTestProject" + - name: Upload app + uses: actions/upload-artifact@v6 + with: + name: RNTestProject-${{ matrix.flavor }} + overwrite: true + path: /tmp/RNTestProject/Build/Products/${{ matrix.flavor }}-iphonesimulator/RNTestProject.app + + test: + needs: build + runs-on: macos-26-large + outputs: + status: ${{ steps.report-status.outputs.status }} + strategy: + fail-fast: false + matrix: + flavor: [Debug, Release] + steps: + - name: Checkout + uses: actions/checkout@v6 + - name: Setup node.js + uses: ./.github/actions/setup-node + - name: Run yarn + uses: ./.github/actions/yarn-install + - name: Download app + uses: actions/download-artifact@v7 + with: + name: RNTestProject-${{ matrix.flavor }} + path: /tmp/RNTestProjectBuild/RNTestProject.app + - name: Check downloaded folder content + run: ls -l /tmp/RNTestProjectBuild/RNTestProject.app + - name: Download React Native Package + if: ${{ matrix.flavor == 'Debug' }} + uses: actions/download-artifact@v7 + with: + name: react-native-package + path: /tmp/react-native-tmp + - name: Configure git + if: ${{ matrix.flavor == 'Debug' }} + shell: bash + run: | + git config --global user.email "react-native-bot@meta.com" + git config --global user.name "React Native Bot" + - name: Prepare project for Metro + if: ${{ matrix.flavor == 'Debug' }} + # In Debug the app loads its bundle from Metro, which must run from an + # initialized project. Re-initialize it here (JS only — no pods); the + # native app itself comes prebuilt from the `build` job. + run: | + REACT_NATIVE_PKG=$(find /tmp/react-native-tmp -type f -name "*.tgz") + echo "React Native tgs is $REACT_NATIVE_PKG" + + BRANCH=${{ github.ref_name }} + if ! [[ $BRANCH == *-stable* ]]; then + BRANCH=main + fi + + node ./scripts/e2e/init-project-e2e.js --projectName RNTestProject --currentBranch $BRANCH --directory /tmp/RNTestProject --pathToLocalReactNative $REACT_NATIVE_PKG - name: Run E2E Tests id: run-tests continue-on-error: true uses: ./.github/actions/maestro-ios with: - app-path: '/tmp/RNTestProject/Build/Products/${{ matrix.flavor }}-iphonesimulator/RNTestProject.app' + app-path: '/tmp/RNTestProjectBuild/RNTestProject.app' app-id: org.reactjs.native.example.RNTestProject maestro-flow: ./scripts/e2e/.maestro/ flavor: ${{ matrix.flavor }}