test: simplifying local testing framework#232
Conversation
WalkthroughThe test suite was refactored to require pre-existing iOS simulators and Android emulators instead of automatically creating and managing them. The removal of 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/emulator.ts`:
- Around line 19-21: Replace uses of execSync with execFileSync to avoid shell
interpolation: call execFileSync(mobilecliBinary, ['devices'], {encoding:
'utf8'}) instead of execSync(`${mobilecliBinary} devices`, ...), and for any
other execSync invocations that interpolate values (e.g., commands that include
device.id) switch to execFileSync(mobilecliBinary, [<command>, device.id, ...],
...) so arguments are passed as an array; update the code where execSync is
imported/used and ensure device lookup (result?.data?.devices) still parses JSON
from the execFileSync output.
In `@test/server.ts`:
- Around line 282-285: The timeout cleanup currently only runs on successful
fetch; declare the timer and AbortController as shown (controller, timer) and
wrap the await fetch(url, { signal: controller.signal }) in a try...finally so
that clearTimeout(timer) is invoked in the finally block (and controller.abort()
behavior remains unchanged) to ensure timers are always cleared even when fetch
throws or times out.
In `@test/simulator.ts`:
- Around line 25-30: The current try/catch around both findSimulatorByName and
installDeviceKitAgent swallows any install failures and calls this.skip(); split
the logic so that findSimulatorByName(simulatorName) is called inside its own
try/catch that, on lookup failure, logs and calls this.skip() (using
simulatorName/simulatorId for context), but do not catch errors from
installDeviceKitAgent(simulatorId) — let installDeviceKitAgent errors propagate
(or be handled separately) so real failures are not hidden.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5e457bf9-de90-4613-9167-62dee02f427a
⛔ Files ignored due to path filters (1)
test/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
test/README.mdtest/avdctl.tstest/emulator.tstest/package.jsontest/server.tstest/simctl.tstest/simulator.ts
💤 Files with no reviewable changes (1)
- test/avdctl.ts
| const output = execSync(`${mobilecliBinary} devices`, {encoding: 'utf8'}); | ||
| const result = JSON.parse(output); | ||
| return result?.data?.devices?.find((d: Device) => d.platform === 'android') ?? null; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find and read the emulator.ts file
find . -type f -name "emulator.ts" | head -5Repository: mobile-next/mobilecli
Length of output: 84
🏁 Script executed:
# Read the emulator.ts file to check lines 19-21 and 82-86
cat -n ./test/emulator.tsRepository: mobile-next/mobilecli
Length of output: 2818
Use execFileSync with argv arrays instead of shell-interpolated strings.
Line 19 and lines 82-86 currently execute shell-interpolated commands via execSync. This risks shell injection if device.id or other interpolated values contain special characters. Switch to execFileSync(binary, args, ...) with array arguments to avoid shell parsing entirely.
Proposed fix
-import {execSync} from 'child_process';
+import {execFileSync} from 'child_process';
@@
function getFirstAndroidDevice(): Device | null {
try {
- const output = execSync(`${mobilecliBinary} devices`, {encoding: 'utf8'});
+ const output = execFileSync(mobilecliBinary, ['devices'], {encoding: 'utf8'});
const result = JSON.parse(output);
return result?.data?.devices?.find((d: Device) => d.platform === 'android') ?? null;
} catch (error) {
return null;
}
}
@@
- mobilecli(`screenshot --device ${device.id} --format png --output ${screenshotPath}`);
+ mobilecli(['screenshot', '--device', device.id, '--format', 'png', '--output', screenshotPath]);
@@
- mobilecli(`url "https://example.com" --device ${device.id}`);
+ mobilecli(['url', 'https://example.com', '--device', device.id]);
@@
- mobilecli(`device info --device ${device.id}`);
+ mobilecli(['device', 'info', '--device', device.id]);
@@
-function mobilecli(args: string): void {
- const command = `${mobilecliBinary} ${args}`;
+function mobilecli(args: string[]): void {
+ execFileSync(mobilecliBinary, args, {
encoding: 'utf8',
timeout: 180000,
stdio: ['pipe', 'pipe', 'pipe'],
- execSync(command, {
- encoding: 'utf8',
- timeout: 180000,
- stdio: ['pipe', 'pipe', 'pipe'],
});
} catch (error: any) {
- console.log(`Command failed: ${command}`);
+ console.log(`Command failed: ${mobilecliBinary} ${args.join(' ')}`);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const output = execSync(`${mobilecliBinary} devices`, {encoding: 'utf8'}); | |
| const result = JSON.parse(output); | |
| return result?.data?.devices?.find((d: Device) => d.platform === 'android') ?? null; | |
| const output = execFileSync(mobilecliBinary, ['devices'], {encoding: 'utf8'}); | |
| const result = JSON.parse(output); | |
| return result?.data?.devices?.find((d: Device) => d.platform === 'android') ?? null; |
🧰 Tools
🪛 OpenGrep (1.20.0)
[ERROR] 19-19: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/emulator.ts` around lines 19 - 21, Replace uses of execSync with
execFileSync to avoid shell interpolation: call execFileSync(mobilecliBinary,
['devices'], {encoding: 'utf8'}) instead of execSync(`${mobilecliBinary}
devices`, ...), and for any other execSync invocations that interpolate values
(e.g., commands that include device.id) switch to execFileSync(mobilecliBinary,
[<command>, device.id, ...], ...) so arguments are passed as an array; update
the code where execSync is imported/used and ensure device lookup
(result?.data?.devices) still parses JSON from the execFileSync output.
| const controller = new AbortController(); | ||
| const timer = setTimeout(() => controller.abort(), 1000); | ||
| const response = await fetch(url, {signal: controller.signal}); | ||
| clearTimeout(timer); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Get the file and examine the relevant section with context
cat -n test/server.ts | sed -n '270,300p'Repository: mobile-next/mobilecli
Length of output: 1043
Move timer cleanup to finally block.
The clearTimeout(timer) on line 285 is only executed on successful fetch. When fetch fails or times out (throwing an exception), the cleanup is skipped and timers accumulate across retry attempts. Move clearTimeout(timer) to a finally block to ensure it runs regardless of success or failure.
Proposed fix
try {
const controller = new AbortController();
const timer = setTimeout(() => controller.abort(), 1000);
- const response = await fetch(url, {signal: controller.signal});
- clearTimeout(timer);
+ const response = await fetch(url, {signal: controller.signal});
if (response.status === 200) {
return;
}
} catch (error) {
// Server not ready yet, continue waiting
+ } finally {
+ clearTimeout(timer);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const controller = new AbortController(); | |
| const timer = setTimeout(() => controller.abort(), 1000); | |
| const response = await fetch(url, {signal: controller.signal}); | |
| clearTimeout(timer); | |
| try { | |
| const controller = new AbortController(); | |
| const timer = setTimeout(() => controller.abort(), 1000); | |
| const response = await fetch(url, {signal: controller.signal}); | |
| if (response.status === 200) { | |
| return; | |
| } | |
| } catch (error) { | |
| // Server not ready yet, continue waiting | |
| } finally { | |
| clearTimeout(timer); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/server.ts` around lines 282 - 285, The timeout cleanup currently only
runs on successful fetch; declare the timer and AbortController as shown
(controller, timer) and wrap the await fetch(url, { signal: controller.signal })
in a try...finally so that clearTimeout(timer) is invoked in the finally block
(and controller.abort() behavior remains unchanged) to ensure timers are always
cleared even when fetch throws or times out.
| try { | ||
| findIOSRuntime(iosVersion); | ||
| simulatorId = createAndLaunchSimulator(iosVersion); | ||
| simulatorId = findSimulatorByName(simulatorName); | ||
| installDeviceKitAgent(simulatorId); | ||
| } catch (error) { | ||
| console.log(`iOS ${iosVersion} runtime not available, skipping tests: ${error}`); | ||
| console.log(`Simulator "${simulatorName}" not found, skipping tests: ${error}`); | ||
| this.skip(); |
There was a problem hiding this comment.
Don’t swallow installDeviceKitAgent failures with this.skip().
Line 29 currently treats any setup error as “simulator not found” and skips the suite. That can hide real regressions (e.g., agent install failures). Limit skipping to lookup failure only.
Proposed fix
const simulatorName = `Test-iOS-${iosVersion}`;
try {
simulatorId = findSimulatorByName(simulatorName);
- installDeviceKitAgent(simulatorId);
} catch (error) {
console.log(`Simulator "${simulatorName}" not found, skipping tests: ${error}`);
this.skip();
+ return;
}
+ installDeviceKitAgent(simulatorId);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| findIOSRuntime(iosVersion); | |
| simulatorId = createAndLaunchSimulator(iosVersion); | |
| simulatorId = findSimulatorByName(simulatorName); | |
| installDeviceKitAgent(simulatorId); | |
| } catch (error) { | |
| console.log(`iOS ${iosVersion} runtime not available, skipping tests: ${error}`); | |
| console.log(`Simulator "${simulatorName}" not found, skipping tests: ${error}`); | |
| this.skip(); | |
| const simulatorName = `Test-iOS-${iosVersion}`; | |
| try { | |
| simulatorId = findSimulatorByName(simulatorName); | |
| } catch (error) { | |
| console.log(`Simulator "${simulatorName}" not found, skipping tests: ${error}`); | |
| this.skip(); | |
| return; | |
| } | |
| installDeviceKitAgent(simulatorId); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/simulator.ts` around lines 25 - 30, The current try/catch around both
findSimulatorByName and installDeviceKitAgent swallows any install failures and
calls this.skip(); split the logic so that findSimulatorByName(simulatorName) is
called inside its own try/catch that, on lookup failure, logs and calls
this.skip() (using simulatorName/simulatorId for context), but do not catch
errors from installDeviceKitAgent(simulatorId) — let installDeviceKitAgent
errors propagate (or be handled separately) so real failures are not hidden.
1 participant
No description provided.