-
Notifications
You must be signed in to change notification settings - Fork 81
fix: delay before starting maestro tests #1804
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,12 +3,12 @@ import { homedir } from "os"; | |
| import path from "path"; | ||
| import * as vscode from "vscode"; | ||
| import { Disposable } from "vscode"; | ||
| import { ExecaError } from "execa"; | ||
| import { DevicePlatform } from "../common/State"; | ||
| import { DeviceBase } from "../devices/DeviceBase"; | ||
| import { ChildProcess, exec, lineReader } from "../utilities/subprocess"; | ||
| import { getOrCreateDeviceSet, IosSimulatorDevice } from "../devices/IosSimulatorDevice"; | ||
| import { extensionContext } from "../utilities/extensionContext"; | ||
| import { Logger } from "../Logger"; | ||
|
|
||
| class MaestroPseudoTerminal implements vscode.Pseudoterminal { | ||
| private writeEmitter = new vscode.EventEmitter<string>(); | ||
|
|
@@ -108,6 +108,65 @@ export class MaestroTestRunner implements Disposable { | |
| pty.writeLine(`Starting ${fileNames.length} Maestro flows: ${fileNames.join(", ")}`); | ||
| } | ||
|
|
||
| // NOTE: maestro sets application permissions using the `applesimutils` utility, | ||
| // which does not support custom device sets. | ||
| // To work around this, we create a symlink to the target Radon device | ||
| // in the default simulator location for the duration of the test, and remove it afterwards. | ||
| const cleanupSymlink = await this.setupSimulatorSymlink(); | ||
| try { | ||
| await this.runMaestro(fileNames, pty); | ||
| pty.writeLine(`\x1b[32mMaestro test completed successfully!\x1b[0m`); | ||
| } catch (error) { | ||
| const exitCode = | ||
| error && typeof error === "object" && "exitCode" in error ? error.exitCode : null; | ||
| // SIGTERM exit code | ||
| if (exitCode !== null && exitCode !== 143) { | ||
| pty.writeLine(`\x1b[31mMaestro test failed with exit code ${exitCode}\x1b[0m`); | ||
| } | ||
| } finally { | ||
| await cleanupSymlink(); | ||
| } | ||
| this.maestroProcess = undefined; | ||
| } | ||
|
|
||
| public async stopMaestroTest(): Promise<void> { | ||
| if (!this.maestroProcess) { | ||
| return; | ||
| } | ||
| this.pty?.writeLine(""); | ||
| this.pty?.writeLine(`\x1b[33mAborting Maestro test...\x1b[0m`); | ||
|
|
||
| const proc = this.maestroProcess; | ||
| try { | ||
| proc.kill(); | ||
| } catch (e) {} | ||
|
|
||
| const killer = setTimeout(() => { | ||
| try { | ||
| proc.kill(9); | ||
| } catch (e) {} | ||
| }, 3000); | ||
|
|
||
| try { | ||
| await proc; | ||
| } catch (e) {} | ||
| clearTimeout(killer); | ||
| this.maestroProcess = undefined; | ||
|
|
||
| this.pty?.writeLine(`\x1b[33mMaestro test aborted\x1b[0m`); | ||
| } | ||
|
|
||
| public async dispose() { | ||
| await this.stopMaestroTest(); | ||
| this.pty?.exit(); | ||
| this.terminal?.dispose(); | ||
| this.onCloseTerminal?.dispose(); | ||
| this.pty = undefined; | ||
| this.terminal = undefined; | ||
| this.onCloseTerminal = undefined; | ||
| } | ||
|
|
||
| private async runMaestro(fileNames: string[], pty: MaestroPseudoTerminal) { | ||
| // Right now, for running iOS tests, Maestro uses xcodebuild test-without-building, | ||
| // which does not support simulators located outside the default device set. | ||
| // The creators provide a prebuilt XCTest runner that can be ran through simctl, | ||
|
|
@@ -117,6 +176,7 @@ export class MaestroTestRunner implements Disposable { | |
| // similar to what Maestro would do in prebuilt mode, and wrap the xcrun | ||
| // command to provide our own device set with the --set flag. | ||
| const shimPath = path.resolve(extensionContext.extensionPath, "shims", "maestro"); | ||
|
|
||
| const maestroProcess = exec("maestro", ["--device", this.device.id, "test", ...fileNames], { | ||
| buffer: false, | ||
| reject: true, | ||
|
|
@@ -145,54 +205,53 @@ export class MaestroTestRunner implements Disposable { | |
| pty.writeLine(line); | ||
| }); | ||
|
|
||
| await maestroProcess.then( | ||
| (resolved) => { | ||
| pty.writeLine(`\x1b[32mMaestro test completed successfully!\x1b[0m`); | ||
| }, | ||
| (error: ExecaError) => { | ||
| // SIGTERM exit code | ||
| if (error.exitCode !== 143) { | ||
| pty.writeLine(`\x1b[31mMaestro test failed with exit code ${error.exitCode}\x1b[0m`); | ||
| } | ||
| } | ||
| ); | ||
| this.maestroProcess = undefined; | ||
| await maestroProcess; | ||
| } | ||
|
|
||
| public async stopMaestroTest(): Promise<void> { | ||
| if (!this.maestroProcess) { | ||
| return; | ||
| private async setupSimulatorSymlink() { | ||
| if (this.device.platform !== DevicePlatform.IOS) { | ||
| return async () => { | ||
| // NOOP | ||
| }; | ||
| } | ||
| this.pty?.writeLine(""); | ||
| this.pty?.writeLine(`\x1b[33mAborting Maestro test...\x1b[0m`); | ||
|
|
||
| const proc = this.maestroProcess; | ||
| try { | ||
| proc.kill(); | ||
| } catch (e) {} | ||
| const id = this.device.id; | ||
| const devicePath = path.join(getOrCreateDeviceSet(id), id); | ||
| const coreSimulatorPath = path.join( | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it always the same even for users with beta versions of the Xcode? shouldn't we really on
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not stored within XCode, it's in your |
||
| homedir(), | ||
| "Library", | ||
| "Developer", | ||
| "CoreSimulator", | ||
| "Devices", | ||
| id | ||
| ); | ||
|
|
||
| const killer = setTimeout(() => { | ||
| async function cleanupSymlink() { | ||
| try { | ||
| proc.kill(9); | ||
| } catch (e) {} | ||
| }, 3000); | ||
| return await promises.unlink(coreSimulatorPath); | ||
| } catch { | ||
| // NOTE: not much we can do here. | ||
| // The symlink will remain, but it should not impact the user. | ||
| Logger.error("Failed to remove simulator symlink: ", coreSimulatorPath); | ||
| } | ||
| } | ||
|
|
||
| try { | ||
| await proc; | ||
| } catch (e) {} | ||
| clearTimeout(killer); | ||
| this.maestroProcess = undefined; | ||
|
|
||
| this.pty?.writeLine(`\x1b[33mMaestro test aborted\x1b[0m`); | ||
| } | ||
|
|
||
| public async dispose() { | ||
| await this.stopMaestroTest(); | ||
| this.pty?.exit(); | ||
| this.terminal?.dispose(); | ||
| this.onCloseTerminal?.dispose(); | ||
| this.pty = undefined; | ||
| this.terminal = undefined; | ||
| this.onCloseTerminal = undefined; | ||
| await promises.symlink(devicePath, coreSimulatorPath); | ||
| return cleanupSymlink; | ||
| } catch (e) { | ||
| // NOTE: check if symlink already exists and points to the correct location | ||
| const existingLink = await promises.readlink(coreSimulatorPath).catch(() => null); | ||
| if (existingLink === devicePath) { | ||
| // NOTE: the symlink might be left here from a previous run, | ||
| // for example if the extension or VSCode was closed while the test was running, | ||
| // and cleanup didn't get to run to completion. | ||
| // We clean up the symlink in that case here. | ||
| // Let's hope the user did not create the symlink themselves for whatever reason... | ||
| return cleanupSymlink; | ||
| } | ||
| } | ||
| return async () => { | ||
| // NOOP | ||
| }; | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why does it not cleanup simlinks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would it?
Stopping the process will make the
maestroProcess"promise" reject, triggering thefinallyclause set up instart.You should cleanup wherever you initialize the resource, where possible.