test: migrate e2e tests from mocha to playwright#248
Conversation
WalkthroughThis PR migrates the test suite from Mocha/Chai to Playwright test runner. The infrastructure layer adds 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 1
🧹 Nitpick comments (2)
test/server.spec.ts (1)
87-91: ⚡ Quick winUse
ifguard pattern for consistency.Lines 89-90 use the non-null assertion operator (
!) to accesserror.codeanderror.data, while the rest of the test suite consistently uses anif (jsonResp.error)guard (e.g., lines 65-68, 113-116, 139-142). Aligning this test with the established pattern improves readability and consistency.♻️ Align with the if-guard pattern used elsewhere
expect(data.jsonrpc).toBe('2.0'); expect(data.error).toBeDefined(); expect(data.error).not.toBeNull(); - expect(data.error!.code).toBe(ErrCodeInvalidRequest); - expect(data.error!.data).toBe("'jsonrpc' must be '2.0'"); + + if (data.error) { + expect(data.error.code).toBe(ErrCodeInvalidRequest); + expect(data.error.data).toBe("'jsonrpc' must be '2.0'"); + } });🤖 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.spec.ts` around lines 87 - 91, Replace the non-null assertions on data.error with the same if-guard pattern used elsewhere: check if (data.error) and then inside that block assert data.error.code equals ErrCodeInvalidRequest and data.error.data equals "'jsonrpc' must be '2.0'"; ensure the test still fails if error is absent (the if-guard preserves behavior and matches the pattern used in other tests like the jsonResp.error checks).test/simulator.spec.ts (1)
257-257: 💤 Low valueConsider using more idiomatic Playwright assertions for array checks.
The pattern
expect(Array.isArray(x)).toBe(true)works but is less idiomatic than directly asserting on array properties.♻️ Alternative approaches
-expect(Array.isArray(entries)).toBe(true); +// Option 1: Just assert on array properties directly +expect(entries.length).toBeGreaterThanOrEqual(0); +// Option 2: If you want to explicitly check type +expect(entries).toBeInstanceOf(Array);Similarly for line 586:
-expect(Array.isArray(rawData.children)).toBe(true); +expect(rawData.children).toBeInstanceOf(Array);Also applies to: 586-586
🤖 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.spec.ts` at line 257, Replace the non-idiomatic check expect(Array.isArray(entries)).toBe(true) with the Playwright-style assertion expect(entries).toBeInstanceOf(Array) (and do the same for the other occurrence around line 586); this targets the entries variable directly and uses the built-in expect matcher to assert it's an Array.
🤖 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/simulator.spec.ts`:
- Around line 290-296: The assertion is checking for the exact name
'mobilecli-test' but the created directory uses a timestamp suffix; update the
test "should remove the test directory from the app container" to detect the
timestamped directory by inspecting the fsList result (used via
fsList(simulatorId, `${containerPath}/Documents`) and variable names) and assert
that no entry name startsWith or matches the pattern 'mobilecli-test-' (e.g.,
use names.some(name => name.startsWith('mobilecli-test-')) and expect that to be
false, or use a regex match check) after calling fsRm(simulatorId, remoteDir,
true).
---
Nitpick comments:
In `@test/server.spec.ts`:
- Around line 87-91: Replace the non-null assertions on data.error with the same
if-guard pattern used elsewhere: check if (data.error) and then inside that
block assert data.error.code equals ErrCodeInvalidRequest and data.error.data
equals "'jsonrpc' must be '2.0'"; ensure the test still fails if error is absent
(the if-guard preserves behavior and matches the pattern used in other tests
like the jsonResp.error checks).
In `@test/simulator.spec.ts`:
- Line 257: Replace the non-idiomatic check
expect(Array.isArray(entries)).toBe(true) with the Playwright-style assertion
expect(entries).toBeInstanceOf(Array) (and do the same for the other occurrence
around line 586); this targets the entries variable directly and uses the
built-in expect matcher to assert it's an Array.
🪄 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: b52d1520-d677-4b56-80ed-cb767507bfd8
⛔ Files ignored due to path filters (1)
test/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
.github/workflows/build.yml.gitignoreMakefiletest/emulator.spec.tstest/index.tstest/package.jsontest/playwright.config.tstest/server.spec.tstest/simulator.spec.tstest/tsconfig.json
💤 Files with no reviewable changes (1)
- test/index.ts
| test('should remove the test directory from the app container', async () => { | ||
| test.skip(!simulatorId, 'simulator not found'); | ||
| fsRm(simulatorId, remoteDir, true); | ||
| const entries = fsList(simulatorId, `${containerPath}/Documents`); | ||
| const names = entries.map((e: any) => e.name); | ||
| expect(names).to.not.include('mobilecli-test'); | ||
| expect(names).not.toContain('mobilecli-test'); | ||
| }); |
There was a problem hiding this comment.
Directory removal check may not detect test directory.
The test directory is created with a timestamp suffix (mobilecli-test-${timestamp}), but line 295 checks for the exact string 'mobilecli-test' without the suffix. Since .toContain() checks for exact element matches in the array, this assertion won't detect if the timestamped directory still exists.
🐛 Proposed fix
fsRm(simulatorId, remoteDir, true);
const entries = fsList(simulatorId, `${containerPath}/Documents`);
const names = entries.map((e: any) => e.name);
-expect(names).not.toContain('mobilecli-test');
+// Check that no directory starts with 'mobilecli-test-'
+const hasTestDir = names.some(name => name.startsWith('mobilecli-test-'));
+expect(hasTestDir).toBe(false);🤖 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.spec.ts` around lines 290 - 296, The assertion is checking for
the exact name 'mobilecli-test' but the created directory uses a timestamp
suffix; update the test "should remove the test directory from the app
container" to detect the timestamped directory by inspecting the fsList result
(used via fsList(simulatorId, `${containerPath}/Documents`) and variable names)
and assert that no entry name startsWith or matches the pattern
'mobilecli-test-' (e.g., use names.some(name =>
name.startsWith('mobilecli-test-')) and expect that to be false, or use a regex
match check) after calling fsRm(simulatorId, remoteDir, true).
1 participant
What
Migrates the
test/E2E suite from mocha + chai to @playwright/test (Playwright's built-in runner andexpect).Changes
server.ts→server.spec.ts,simulator.ts→simulator.spec.ts,emulator.ts→emulator.spec.ts.describe/it/before/after→test.describe/test/beforeAll/afterAll; chaiexpect().to.*→ Playwrightexpect().to*;this.skip()→test.skip(condition, reason);this.timeout()removed in favor of a global config timeout.playwright.config.tswith one project per suite (server/simulator/emulator),workers: 1,retries: 0,fullyParallel: false,timeout: 180000,reporter: 'list'to preserve mocha's serial, single-device behavior.index.ts(the mocha aggregator — Playwright discovers files itself).package.json: droppedmocha,chai,@types/mocha,@types/chai,ts-node,tsx; added@playwright/test. Scripts now runplaywright test --project=…(addedtestandtest:ios).tsconfig.json:typesreduced to["node"].build.yml): dropped the no-op--grep "Android API …"from the Android job; iOS/server jobs unchanged.Makefiletest-e2e: fixedtest-simulator→test:simulator..gitignore: ignore Playwright output dirs.Verification
All three suites pass against real targets locally:
device lifecycletest)Go coverage collection (
GOCOVERDIR) is preserved unchanged.