Repository navigation
refactor(provider-webdriver): give BrowserStack and AWS Device Farm the plugin factory shape - #3312
Conversation
Size Report
Startup median (7 runs, lower is better):
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 46 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/provider-webdriver/src/aws-device-farm-connection.test.ts">
<violation number="1" location="packages/provider-webdriver/src/aws-device-farm-connection.test.ts:36">
P3: This test does not verify the flag-first or AWS-variable fallback behavior named in its title: no selector flag competes with an environment value, and the agent-device app variable masks the AWS app variable. Add cases with competing values to assert the precedence and AWS-only fallback.</violation>
</file>
<file name="packages/provider-testmu/src/connection.ts">
<violation number="1" location="packages/provider-testmu/src/connection.ts:75">
P2: `requireResolvedProfileValue` accepts an empty `providerApp`, so a hand-authored profile can pass verification without naming an app. Reject blank values here, as the previous `required` check did.</violation>
</file>
<file name="packages/provider-webdriver/src/browserstack-connection.ts">
<violation number="1" location="packages/provider-webdriver/src/browserstack-connection.ts:63">
P2: Connect persists BrowserStack feature combinations that the session builder rejects, such as `--provider-no-resign-app` on Android or both network options. Validate features against the platform and each other before saving the profile so `connect` cannot succeed with a profile that cannot create a session.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
| 'TestMu AI profile missed OS version.', | ||
| ), | ||
| app: canonicalTestMuAppReference( | ||
| requireResolvedProfileValue(flags.providerApp, 'TestMu AI profile missed app.'), |
There was a problem hiding this comment.
P2: requireResolvedProfileValue accepts an empty providerApp, so a hand-authored profile can pass verification without naming an app. Reject blank values here, as the previous required check did.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/provider-testmu/src/connection.ts, line 75:
<comment>`requireResolvedProfileValue` accepts an empty `providerApp`, so a hand-authored profile can pass verification without naming an app. Reject blank values here, as the previous `required` check did.</comment>
<file context>
@@ -57,23 +60,36 @@ export function createTestMuConnection(
+ 'TestMu AI profile missed OS version.',
+ ),
+ app: canonicalTestMuAppReference(
+ requireResolvedProfileValue(flags.providerApp, 'TestMu AI profile missed app.'),
+ ),
deviceType: readTestMuDeviceType(flags),
</file context>
| requireResolvedProfileValue(flags.providerApp, 'TestMu AI profile missed app.'), | |
| requireResolvedProfileValue(flags.providerApp?.trim() ? flags.providerApp : undefined, 'TestMu AI profile missed app.'), |
| providerProject: flags.providerProject, | ||
| providerBuild: flags.providerBuild, | ||
| providerSessionName: flags.providerSessionName, | ||
| ...readBrowserStackDeviceFeatureFields(flags), |
There was a problem hiding this comment.
P2: Connect persists BrowserStack feature combinations that the session builder rejects, such as --provider-no-resign-app on Android or both network options. Validate features against the platform and each other before saving the profile so connect cannot succeed with a profile that cannot create a session.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/provider-webdriver/src/browserstack-connection.ts, line 63:
<comment>Connect persists BrowserStack feature combinations that the session builder rejects, such as `--provider-no-resign-app` on Android or both network options. Validate features against the platform and each other before saving the profile so `connect` cannot succeed with a profile that cannot create a session.</comment>
<file context>
@@ -0,0 +1,94 @@
+ providerProject: flags.providerProject,
+ providerBuild: flags.providerBuild,
+ providerSessionName: flags.providerSessionName,
+ ...readBrowserStackDeviceFeatureFields(flags),
+ },
+ // Verification reads these flags; it must see the canonical reference the profile saved,
</file context>
| }; | ||
| } | ||
|
|
||
| test('connect resolves selectors from flags, then agent-device variables, then AWS variables', async () => { |
There was a problem hiding this comment.
P3: This test does not verify the flag-first or AWS-variable fallback behavior named in its title: no selector flag competes with an environment value, and the agent-device app variable masks the AWS app variable. Add cases with competing values to assert the precedence and AWS-only fallback.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/provider-webdriver/src/aws-device-farm-connection.test.ts, line 36:
<comment>This test does not verify the flag-first or AWS-variable fallback behavior named in its title: no selector flag competes with an environment value, and the agent-device app variable masks the AWS app variable. Add cases with competing values to assert the precedence and AWS-only fallback.</comment>
<file context>
@@ -0,0 +1,97 @@
+ };
+}
+
+test('connect resolves selectors from flags, then agent-device variables, then AWS variables', async () => {
+ const connection = createAwsDeviceFarmConnection(
+ host({
</file context>
|
I found no code problems at 1726077. The factory shape for BrowserStack and AWS Device Farm looks right, and the provider scenarios cover it with fake hubs. I did not run a live BrowserStack or AWS session, and I did not run the eager-closure, layering, fallow, or unit gates here. The check that the daemon loads no connect code comes from reading the code only. The Linux Smoke Tests job failed in the "Install Linux desktop dependencies" apt-get step with a 6-minute timeout, before any repo code ran. This diff does not touch the Linux smoke route or the workflow, so the failure looks unrelated. Please rerun that job. Before merge, please also answer the two Cubic threads that still hold. Not blocking, take or leave: (1) A selector should be the first non-empty value among the flag, the agent-device variable, and the AWS variable, at both connect and lease time. Today Is there a smaller shape that still meets #3309 and #3310? I looked and found none. Production shrinks by 7 lines and one factory shape plus a declaration replaces a definition table, a facade, a core-side profile builder, and two verify adapters. CloudWebDriverProviderHost, CloudWebDriverConnection, and CloudWebDriverProviderDeclaration mirror the types in src/sdk/plugins.ts, src/plugins/connection.ts, and src/plugins/manifest.ts, and provider-webdriver cannot import from src today. What has to change first is the package split in #3309 and #3310. After it, the declaration should come from package.json On the open threads, the BrowserStack device-feature fields thread (P2) still applies. It predates this PR, so it is not a regression. Calling buildBrowserStackDeviceFeatureCapabilities in resolve fixes it. One lower-priority thread also still applies: the AWS connection test coverage thread. It notes the test sets no flag against an env value and only AWS_DEFAULT_REGION. The blank providerApp thread (P2) does not apply, because the profile parser rejects blank strings and connect resolve rejects blanks. Only a CLI |
…he plugin factory shape
Each bundled hosted-WebDriver provider is now a factory returning { webDriver, connection }
plus a manifest-shaped declaration, the exact shape an installed WebDriver plugin returns.
The CLI composes bundled and installed providers through one connect route and one runtime
factory; the bundled definition table, the ProviderWebDriver facade with its dead
listArtifactsFromEnv, the core-side profile builders, and the per-provider verify adapters
are gone. Connect-time flag readers and resolved-profile checks are shared with TestMu.
1726077 to
586aa86
Compare
|
I reviewed 586aa86 again. The earlier findings from the first review are fixed or unchanged in the logical patch, and I found no new code problem that needs a change before merge. CI is green: 19 checks ran at this commit and none failed. The earlier Linux Smoke apt-get timeout does not recur. There are no conflicts. Not blocking, and you can take it or leave it: The three open Cubic threads need a reply and a resolve. The blank I ran no gates locally (eager-closure, layering, fallow, unit). I judged the lazy-connect behavior from the imports, and CI is green at this commit. I did not run a live BrowserStack or AWS session. The delta since the first review is a rebase only. I also did not check whether the CLI accepts a blank |
|
I found no new problems in 586aa86, and the earlier findings are fixed. CI is green on this commit, with 19 checks and none failing, and the earlier Linux Smoke apt-get timeout does not show up here. There are no conflicts. No live BrowserStack or AWS Device Farm connect or session run exists for this PR. I ran no local gates, so this result relies on CI. Two open Cubic threads still hold. The first is the BrowserStack feature-combination check at #3312 (comment). The old cloud-webdriver-profile.ts persisted those fields unchecked too, so this is not a regression and can be a follow-up. The second is the AWS precedence test title at #3312 (comment). No selector flag competes with an env value there, so the title claims more than the test asserts. This one is a non-blocking test note. The Cubic thread on the blank providerApp does not apply, so you can resolve it: #3312 (comment). A hand-written profile cannot carry a blank providerApp, because the remote-config parser refuses empty strings. Connect's own resolve also trims and checks the flag. Before merge, please reply in the two Cubic threads that still hold, and either fix them or resolve them. |
|
1 participant
Summary
Prepares the 0.22 plan to unbundle hosted device providers (#3309 BrowserStack, #3310 AWS Device Farm, #3311 Limrun) by giving BrowserStack and AWS Device Farm the exact shape an installed WebDriver plugin has, so the extraction becomes a packaging change.
BundledCloudWebDriverProvider: a manifest-shapeddeclaration(provider id,connectioncapabilities,credentialVariables) pluscreate(host) -> { webDriver, connection }, the shape TestMu returns from its factory. Connect policy and the credential fingerprint read the declaration the way they read a plugin manifest; no provider-name branch is left insrc/.resolveProviderConnectionProfile) and one runtime factory. The bundled definition table, theProviderWebDriverfacade with its test-onlylistArtifactsFromEnv,cloud-webdriver-profile.ts, and the per-provider verify adapters are gone. The CLI now fillsleaseBackendfrom the platform for every provider connection, so TestMu and the Doublespeed plugin no longer each spell the mapping.requireConnectPlatform,requireConnectFlag,resolveLocalAppArtifact) and resolved-profile checks (requireResolvedProfileValue,requireResolvedProfilePlatformin contracts) are shared with TestMu, whose connection module drops its private copies.await import, so a daemon never evaluates connect code; the eager-closure gate stays at the merge-base count with no approval row.@agent-device/provider-webdriver/connection-verificationis the subpath core tests substitute, mirroring the TestMu package.Behaviour notes: generated BrowserStack/AWS profiles now carry
stateDirlike every other provider profile; a whitespace-only BrowserStack credential is treated as unset by the fingerprint (it was already refused as missing); the lease-time AWS message now also names<arn>, matching connect; TestMu's hand-authored-profile verify errors use the sharedCOMMAND_FAILEDreconnect hint.46 files. Gross diff is above the 1,000-line budget because every test that built the bundled runtimes or mocked the facade had to be retargeted; production is net +19 lines with the superseded paths deleted, and test coverage moved with the code (new
*-connection.test.ts,*-provider.test.ts).Validation
Tested commit:
586aa86bb1(rebased ontod994d1f7e1; the only conflict was theaws-device-farm.tsimport block after #3303 removedLeaseValue). The pre-rebase head172607744cpassedpnpm check:affected --runin full.pnpm gate …: format, lint, typecheck, layering, di-seams, fallow, build, package, integration-node, macos-coverage, unit, wire-compat, production-exports, eager-closure budgets, depgraph, gate-manifest, replay-compat, maestro-conformance, …).provider-integrationis 230/231: the one failure isandroid-lifecycle.test.ts› "Android Settings flow uses scripted ADB provider", whosewaitForFileContenthas a 1 s budget. It fails 2/3 runs at the merge-base commitd994d1f7e1under the same host load and 1/3 runs on this head, so it is a pre-existing contention flake, not a regression; it is unrelated to this change (scripted ADB app-log path).fallow audit --base origin/mainreports no dead code (one 6-line warn-level clone: the lazy connect wrapper in both providers).