Adds initial support for Sauce Labs as a driver for Mobilewright. #223
Open
diemol wants to merge 16 commits into
Open
Adds initial support for Sauce Labs as a driver for Mobilewright. #223diemol wants to merge 16 commits into
diemol wants to merge 16 commits into
Conversation
Config types, factory wiring, driver instantiation, public exports, and build config
The screen fixture now respects a configurable screenshot
mode ('on' or 'only-on-failure') from use.screenshot, while
still falling back to the original behavior of always capturing
a screenshot on test failure when no mode is set.
…ced timeout handling
# Conflicts: # package-lock.json # packages/mobilewright/package.json # packages/mobilewright/tsconfig.json # tsconfig.json
…user instead of guessing it
…_BUNDLE_ID env var
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis PR adds a new 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 7
🧹 Nitpick comments (4)
packages/driver-saucelabs/src/rest-client.ts (1)
316-435: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting the shared App Storage upload logic.
uploadToStorage,uploadUrlToStorage, anduploadWdaToStorageeach repeat the same FormData POST +item.idvalidation. A small private helper (e.g.postToStorage(filename, content): Promise<string>returning the storage id) would remove the triplicated error handling and keep the response-shape contract in one place.🤖 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 `@packages/driver-saucelabs/src/rest-client.ts` around lines 316 - 435, The App Storage upload flow is duplicated across uploadToStorage, uploadUrlToStorage, and uploadWdaToStorage, including the FormData POST, response.ok check, and item.id validation. Extract that shared logic into a small private helper in RestClient, such as a postToStorage-style method that accepts the filename and payload content and returns the storage id, then have each of the three methods call it after handling their unique download/repackage steps. Keep the response-shape parsing and error handling in that helper so the contract stays in one place.packages/driver-saucelabs/src/wda-client.ts (1)
105-174: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
ensureInitialised()is not concurrency-safe.
this.initialisedis only set totrueat the very end, so two concurrent callers (e.g.Promise.all([getSource(), listApps()])) both observefalseand each launch WDA and open a WebDriver/session, causing duplicate launches / sessions. Memoize the in-flight promise so concurrent callers share one initialization.♻️ Suggested memoization
- private initialised = false; + private initialised = false; + private initPromise: Promise<void> | null = null; @@ private async ensureInitialised(): Promise<void> { if (this.initialised) return; + if (this.initPromise) return this.initPromise; + this.initPromise = this.doInitialise().finally(() => { this.initPromise = null; }); + return this.initPromise; + } + + private async doInitialise(): Promise<void> {Please confirm whether Mobilewright can drive these WDA methods concurrently.
🤖 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 `@packages/driver-saucelabs/src/wda-client.ts` around lines 105 - 174, `ensureInitialised()` can be entered by multiple concurrent callers before `this.initialised` flips to true, so the same WDA launch/session setup may run twice. Fix this by memoizing the in-flight initialization in `WdaClient` (around `ensureInitialised`, `this.initialised`, and the `/session` creation path) so all callers await the same promise, and clear/reset it on failure while preserving the existing success assignment. Do not rely on Mobilewright calling `getSource()` and `listApps()` sequentially; make the initialization path itself concurrency-safe.packages/driver-saucelabs/src/integration.test.ts (1)
26-150: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared driver construction into a helper.
The same
new SauceLabsDriver({ region: REGION, ...(IOS_WDA_STORAGE_REF ? { iosWdaStorageRef: IOS_WDA_STORAGE_REF } : {}) })pattern is repeated across all 8 tests. Extracting a helper or using a Playwright fixture would reduce duplication and make config changes easier to maintain.♻️ Proposed refactor
+function createDriver() { + return new SauceLabsDriver({ + region: REGION, + ...(IOS_WDA_STORAGE_REF ? { iosWdaStorageRef: IOS_WDA_STORAGE_REF } : {}), + }); +} test.describe('SauceLabsDriver integration', () => { // ... test('connects and returns a session with the correct platform', async () => { - const driver = new SauceLabsDriver({ - region: REGION, - ...(IOS_WDA_STORAGE_REF ? { iosWdaStorageRef: IOS_WDA_STORAGE_REF } : {}), - }); + const driver = createDriver();🤖 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 `@packages/driver-saucelabs/src/integration.test.ts` around lines 26 - 150, The `integration.test.ts` file repeats the same `new SauceLabsDriver({ region: REGION, ...(IOS_WDA_STORAGE_REF ? { iosWdaStorageRef: IOS_WDA_STORAGE_REF } : {}) })` setup in every test, so extract that construction into a shared helper or fixture. Add a single reusable factory (for example near the test suite setup) and update each test to call it before `connect`, keeping the existing `SauceLabsDriver`, `REGION`, and `IOS_WDA_STORAGE_REF` behavior unchanged.packages/test/src/fixtures.ts (1)
214-234: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSimplify the two-block screenshot logic into a single condition.
The two separate
ifblocks are logically equivalent toscreenshotMode === 'on' || failed— the second block is a fallback that fires when the test failed but the mode wasn't already handled by the first block. Merging them eliminates duplicatedevice.screen.screenshot()calls and try/catch blocks.♻️ Proposed refactor
const screenshotMode = typeof screenshot === 'object' ? (screenshot as any).mode : screenshot; const failed = testInfo.status !== testInfo.expectedStatus; - if (screenshotMode === 'on' || (screenshotMode === 'only-on-failure' && failed)) { + if (screenshotMode === 'on' || failed) { try { const buf = await device.screen.screenshot(); const label = failed ? 'screenshot-on-failure' : 'screenshot'; await testInfo.attach(label, { body: buf, contentType: 'image/png' }); } catch (err) { debug('screenshot attach failed: %o', err); } } - - if (failed) { - if (screenshotMode !== 'on' && screenshotMode !== 'only-on-failure') { - try { - const buf = await device.screen.screenshot(); - await testInfo.attach('screenshot-on-failure', { body: buf, contentType: 'image/png' }); - } catch (err) { - debug('screenshot-on-failure attach failed: %o', err); - } - }🤖 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 `@packages/test/src/fixtures.ts` around lines 214 - 234, Simplify the screenshot attachment flow in the fixture logic by merging the two `if` blocks around `screenshotMode`, `failed`, and `device.screen.screenshot()` into one condition. Update the logic in the screenshot handling section to use a single branch equivalent to `screenshotMode === 'on' || failed`, while preserving the existing attach label behavior and the `debug(...)` error logging path. Keep the change localized to the screenshot handling code in `fixtures.ts`.
🤖 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 `@e2e/mobilewright.config.ts`:
- Around line 18-22: The saucelabs branch in mobilewright.config.ts should fail
fast by validating inputs instead of using a type cast. Update the logic that
builds the saucelabs config so it explicitly checks SAUCE_REGION against the
supported values and throws a clear error for anything else, and add the same
kind of presence validation used in mobilenext for SAUCE_USERNAME and
SAUCE_ACCESS_KEY. Keep the fix localized to the configuration switch case and
the helper that returns the saucelabs connection settings.
In `@packages/driver-saucelabs/src/driver.ts`:
- Line 315: The paired connect call in driver.ts can leave one WebSocket open if
`Promise.all` fails partway through. Update the connection flow around
`ioSocket.connect()` and `companionSocket.connect()` in `Driver` so any socket
that successfully connects is explicitly closed when the other connect rejects,
since `disconnect()` cannot be relied on before `this.session` is set. Use the
existing socket variables in that block to ensure partial failures always clean
up connected resources.
- Around line 641-650: In launchApp, the install+launch polling loop can exit on
the deadline without ever hitting FINISHED or ERROR, which leaves the method
returning successfully even though the app may not have launched. Update the
launchApp flow in driver.ts so it explicitly detects the timeout after the while
loop and throws an error, mirroring the timeout handling used by installApp;
keep the fix localized around the polling logic and the launchApp method.
- Around line 577-588: The iOS pressButton flow in driver.ts incorrectly tries
socket first in the WDA fallback branch, but sendKey(button) does not throw for
unsupported keys when the socket is connected, so the WDA fallback never runs.
Update the pressButton logic to route buttons that are not handled by the socket
mapping directly to wda.pressButton(...) using BUTTON_WDA and
requireWda(session), and keep the unsupported-button error only for buttons with
no WDA mapping.
- Around line 477-481: The clearText() implementation in Driver is sending a
literal key instead of selecting all text, so it only removes one character.
Update Driver.clearText() to use the proper select-all chord via
ioSocket.sendKey, then Backspace, so the whole field is cleared reliably.
In `@packages/driver-saucelabs/src/integration.test.ts`:
- Around line 16-22: The skip guard in SauceLabsDriver integration tests only
checks INTEGRATION, but the message also requires SAUCE_USERNAME and
SAUCE_ACCESS_KEY. Update the test.skip condition in integration.test.ts so it
skips when any required credential is missing, not just when SAUCE_INTEGRATION
is unset. Use the existing INTEGRATION constant and add checks for the relevant
environment variables near the top-level setup so the skip reason matches the
actual runtime requirement.
In `@packages/mobilewright/src/device-pool/adapters/saucelabs-allocator.ts`:
- Around line 34-42: The release flow in SaucelabsAllocator.release removes the
device-to-session mapping before SauceLabsDriver.releaseSession succeeds, which
can orphan an active session if releaseSession throws. Update release() so the
sessionsByDeviceId.delete(deviceId) call happens only after a successful
releaseSession call, keeping the session recoverable on failure. Use the
release() method and sessionsByDeviceId map in SaucelabsAllocator to locate and
reorder the cleanup logic.
---
Nitpick comments:
In `@packages/driver-saucelabs/src/integration.test.ts`:
- Around line 26-150: The `integration.test.ts` file repeats the same `new
SauceLabsDriver({ region: REGION, ...(IOS_WDA_STORAGE_REF ? { iosWdaStorageRef:
IOS_WDA_STORAGE_REF } : {}) })` setup in every test, so extract that
construction into a shared helper or fixture. Add a single reusable factory (for
example near the test suite setup) and update each test to call it before
`connect`, keeping the existing `SauceLabsDriver`, `REGION`, and
`IOS_WDA_STORAGE_REF` behavior unchanged.
In `@packages/driver-saucelabs/src/rest-client.ts`:
- Around line 316-435: The App Storage upload flow is duplicated across
uploadToStorage, uploadUrlToStorage, and uploadWdaToStorage, including the
FormData POST, response.ok check, and item.id validation. Extract that shared
logic into a small private helper in RestClient, such as a postToStorage-style
method that accepts the filename and payload content and returns the storage id,
then have each of the three methods call it after handling their unique
download/repackage steps. Keep the response-shape parsing and error handling in
that helper so the contract stays in one place.
In `@packages/driver-saucelabs/src/wda-client.ts`:
- Around line 105-174: `ensureInitialised()` can be entered by multiple
concurrent callers before `this.initialised` flips to true, so the same WDA
launch/session setup may run twice. Fix this by memoizing the in-flight
initialization in `WdaClient` (around `ensureInitialised`, `this.initialised`,
and the `/session` creation path) so all callers await the same promise, and
clear/reset it on failure while preserving the existing success assignment. Do
not rely on Mobilewright calling `getSource()` and `listApps()` sequentially;
make the initialization path itself concurrency-safe.
In `@packages/test/src/fixtures.ts`:
- Around line 214-234: Simplify the screenshot attachment flow in the fixture
logic by merging the two `if` blocks around `screenshotMode`, `failed`, and
`device.screen.screenshot()` into one condition. Update the logic in the
screenshot handling section to use a single branch equivalent to `screenshotMode
=== 'on' || failed`, while preserving the existing attach label behavior and the
`debug(...)` error logging path. Keep the change localized to the screenshot
handling code in `fixtures.ts`.
🪄 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: 4414fc76-b39f-4e38-b078-e98678fd556d
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (30)
e2e/mobilewright.config.tse2e/package.jsonpackages/driver-saucelabs/package.jsonpackages/driver-saucelabs/src/companion-socket.test.tspackages/driver-saucelabs/src/companion-socket.tspackages/driver-saucelabs/src/device-control-socket.test.tspackages/driver-saucelabs/src/device-control-socket.tspackages/driver-saucelabs/src/driver.test.tspackages/driver-saucelabs/src/driver.tspackages/driver-saucelabs/src/index.tspackages/driver-saucelabs/src/integration.test.tspackages/driver-saucelabs/src/rest-client.test.tspackages/driver-saucelabs/src/rest-client.tspackages/driver-saucelabs/src/wda-client.tspackages/driver-saucelabs/tsconfig.jsonpackages/mobilewright-core/src/locator.test.tspackages/mobilewright-core/src/locator.tspackages/mobilewright/package.jsonpackages/mobilewright/src/config.tspackages/mobilewright/src/device-pool/adapters/saucelabs-allocator.tspackages/mobilewright/src/device-pool/allocator-factory.tspackages/mobilewright/src/device-pool/application/device-pool.tspackages/mobilewright/src/device-pool/application/ports.tspackages/mobilewright/src/device-pool/domain/device-slot.tspackages/mobilewright/src/index.tspackages/mobilewright/src/launchers.tspackages/mobilewright/tsconfig.jsonpackages/protocol/src/types.tspackages/test/src/fixtures.tstsconfig.json
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@packages/driver-saucelabs/src/driver.ts`:
- Around line 322-329: The setup failure path in `Driver.connect`/the
`Promise.all([ioSocket.connect(), companionSocket.connect()])` block only
disconnects sockets and leaves an already-created owned Sauce Labs session
unreleased because `this.session` is still unset. Update the catch block to also
release that owned session using the session handle returned earlier in the
setup flow (the same one later assigned to `this.session`), and preserve the
existing socket cleanup before rethrowing the error.
🪄 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: 06716ca6-55a1-4f5f-a004-77639c25effa
📒 Files selected for processing (5)
e2e/mobilewright.config.tspackages/driver-saucelabs/src/driver.test.tspackages/driver-saucelabs/src/driver.tspackages/driver-saucelabs/src/integration.test.tspackages/mobilewright/src/device-pool/adapters/saucelabs-allocator.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- e2e/mobilewright.config.ts
- packages/driver-saucelabs/src/integration.test.ts
- packages/mobilewright/src/device-pool/adapters/saucelabs-allocator.ts
- packages/driver-saucelabs/src/driver.test.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/driver-saucelabs/src/driver.ts (1)
338-348: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winOwned session cleanup is still missing on early
connect()failures
waitForActiveSession()anduploadWdaToStorage()can both throw beforethis.sessionis assigned, which leaves the newly created Sauce session behind; the WDA path can also leak the already-connected sockets. Wrap this setup so any failure releases the owned session, and disconnect the sockets on the post-connect path.🤖 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 `@packages/driver-saucelabs/src/driver.ts` around lines 338 - 348, The iOS setup in driver.ts can fail before this.session is assigned, leaving an owned Sauce session and connected sockets behind. Update the connect flow around waitForActiveSession() and uploadWdaToStorage() in the driver class so any exception triggers cleanup of the newly created session, and ensure the post-connect WDA path also disconnects the sockets when setup fails. Use the existing driver/session handling code paths in Driver.connect() and the WDA upload logic to place the cleanup in the right failure branches.
🤖 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.
Outside diff comments:
In `@packages/driver-saucelabs/src/driver.ts`:
- Around line 338-348: The iOS setup in driver.ts can fail before this.session
is assigned, leaving an owned Sauce session and connected sockets behind. Update
the connect flow around waitForActiveSession() and uploadWdaToStorage() in the
driver class so any exception triggers cleanup of the newly created session, and
ensure the post-connect WDA path also disconnects the sockets when setup fails.
Use the existing driver/session handling code paths in Driver.connect() and the
WDA upload logic to place the cleanup in the right failure branches.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f5bce834-0ed9-440a-ba20-e2ff3dec9ffd
📒 Files selected for processing (1)
packages/driver-saucelabs/src/driver.ts
…is is the actual way it should work.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/driver-saucelabs/src/driver.ts (1)
498-503: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
clearText()still sends a literala, not a select-all chord.The docstring says "Selects all text and deletes it using Ctrl+A", but
ioSocket.sendKey('a')just types the charactera. Backspace then deletes a single character, so existing text is left in place (and anamay be appended). This is the same issue raised in a prior review that was marked addressed but is still present in the current code.
sendKeyonly takes a single key name, so a proper select-all requires either a modifier/chord API or a platform-specific approach (Android:input keyevent KEYCODE_MOVE_END+ repeatedKEYCODE_DEL, orCtrl+Aif supported; iOS: WDA clear). Please verify what chord/modifier supportDeviceControlSocket.sendKeyactually provides before landing the fix.#!/bin/bash set -euo pipefail # Inspect sendKey to confirm whether modifier/chord input is supported. fd -t f 'device-control-socket.ts' packages/driver-saucelabs/src --exec ast-grep outline {} --view expanded echo '---' rg -nP '\bsendKey\s*\(|sendChord|modifier|Ctrl|KEYCODE' packages/driver-saucelabs/src -g '!**/*.test.ts'🤖 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 `@packages/driver-saucelabs/src/driver.ts` around lines 498 - 503, The clearText() implementation still sends a literal key instead of a true select-all action. Update clearText() in Driver to use the actual chord/modifier support exposed by DeviceControlSocket.sendKey, or switch to the appropriate platform-specific clear behavior if chords are not supported. Verify the input path in clearText(), ioSocket.sendKey, and any related DeviceControlSocket APIs so the sequence genuinely selects all text before deleting it.
🤖 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.
Duplicate comments:
In `@packages/driver-saucelabs/src/driver.ts`:
- Around line 498-503: The clearText() implementation still sends a literal key
instead of a true select-all action. Update clearText() in Driver to use the
actual chord/modifier support exposed by DeviceControlSocket.sendKey, or switch
to the appropriate platform-specific clear behavior if chords are not supported.
Verify the input path in clearText(), ioSocket.sendKey, and any related
DeviceControlSocket APIs so the sequence genuinely selects all text before
deleting it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9872d356-d94e-4cdd-b16d-8f687e88ebde
📒 Files selected for processing (2)
packages/driver-saucelabs/src/driver.test.tspackages/driver-saucelabs/src/driver.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/driver-saucelabs/src/driver.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR includes:
@mobilewright/driver-saucelabsthat implementsprotocole2e/mobilewright.config.tsande2e/package.jsonto support the new Sauce Labs driver (I need to understand how these tests are executed because I got some failures here).Claude Code was used to generate most of the code. I reviewed the generated code and made a few adjustments.
EDIT: Docs are still missing. I can add them when we are all happy with this implementation.