Repository navigation
Conversation
smkjason
changed the title
Contributor License AgreementAll contributors are covered by a CLA. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Devin Review found 3 potential issues.
1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
| data.nodeVersion = process.version; | ||
| const connected = browser; | ||
| const page = await step('page', async () => { | ||
| context = await withTimeout(connected.newContext({ viewport: VIEWPORT }), CONNECT_TIMEOUT_MS, 'Context creation timed out'); |
There was a problem hiding this comment.
🔴 Timed-out contexts escape cleanup
When newContext() finishes after its timeout, context remains unset and cleanup cannot close the new context. The session records successful cleanup and continues allocating provider resources.
Learn more
A timed-out newContext() continues running because withTimeout only races promises. The assignment to context occurs only if the raced promise resolves first. The cleanup loop then has no reference to a late-created context, even if browser.close() fails or only detaches the provider connection. This also applies to late connection establishment after the chromium.connect() timeout.
Example: A provider creates a context at 31 seconds. The local 30-second race has already rejected, and context is undefined during cleanup. The later context is never explicitly closed, while the attempt can report cleanupSuccess: true.
Recommended fix: Retain and reconcile the underlying context-creation and connection promises after timeout. Await or cancel them within bounded cleanup, explicitly close any late resource, and mark cleanup uncertain and stop further allocations if ownership cannot be established.
Was this helpful? React with 👍 or 👎 to provide feedback.
| data = await retrieve(signal); | ||
| } | ||
| if (data.status !== 'READY') throw new Error('Momentic session did not become READY'); | ||
| if (data.playwrightVersion !== PLAYWRIGHT_VERSION) throw new Error('Momentic returned an incompatible Playwright version'); |
There was a problem hiding this comment.
🔴 Compatible Momentic sessions are rejected
When Momentic reports a compatible 1.60.x patch other than 1.60.0, provision() rejects every session. environment() accepts that version, so a valid configured provider cannot run.
| if (data.playwrightVersion !== PLAYWRIGHT_VERSION) throw new Error('Momentic returned an incompatible Playwright version'); | |
| if (typeof data.playwrightVersion !== 'string' || !/^1\.60\.\d+$/.test(data.playwrightVersion)) throw new Error('Momentic returned an incompatible Playwright version'); |
Was this helpful? React with 👍 or 👎 to provide feedback.
| export const nativeConfig = { | ||
| iterations: 100, | ||
| concurrency: 1, | ||
| groupBy: 'round' as const, | ||
| participants: nativeParticipants, | ||
| }; |
There was a problem hiding this comment.
🟡 Missing participant permits one-sided comparison
When either provider lacks credentials, runBenchmark() skips it and runs the other without failing. The default command writes a successful result with no paired provider observations.
Learn more
The runner's resolveParticipants silently removes participants whose required environment variables are missing and only fails when none remain. These suites declare both participants but have no default-run preflight requiring both. The output writer then includes only the available participant, so its success rates omit the missing comparison entirely.
Example: With Momentic credentials and no Azure token, pnpm bench:playwright-readiness --no-ingest runs 100 Momentic sessions and produces a single-participant results file rather than refusing the proposed paired run.
Recommended fix: Preflight that both participants are available for the default comparison, while retaining an explicit single-provider diagnostic mode if desired; represent unavailable participants separately from attempted sessions.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Adds two
chromium.connect()benchmarks for Momentic Browser Fleet and Azure Playwright Workspaces:Each suite defaults to 100 fresh sessions per provider, one at a time, alternating providers each round. Results include failed attempts, partial action records, success rates, and untrimmed median/P95/P99 timings. Existing CDP benchmarks and historical results stay unchanged.
Please review the timing boundaries, region/version requirements, and provider setup and cleanup before we publish a comparison. Azure session IDs, server-version reporting, and cleanup verification still need work. Scoring and SDK/package publication are deferred. Details and run instructions are in PLAYWRIGHT_NATIVE_BENCHMARKS.md.
Local end-to-end fixture tests, typecheck, and whitespace checks pass. The tests cover both CLI entrypoints, native connections, the 50-action loop, failure handling, cleanup, and credential redaction. CI is waiting for maintainer approval.
I’m affiliated with Momentic, which has offered to cover Azure costs for this comparison. This PR includes no provider performance results or real credentials.