-
Notifications
You must be signed in to change notification settings - Fork 302
WIP - Add experimental Firefox integration test support #3345
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
5d5c836 to
d729fa8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cursor Bugbot has reviewed your changes and found 4 potential issues.
| export function getManifestVersion() { | ||
| return process.env.npm_lifecycle_event === 'playwright-mv2' ? 2 : 3; | ||
| return process.env.npm_lifecycle_event === 'playwright' ? 3 : 2; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Default manifest version changed breaks direct test execution
Medium Severity
The getManifestVersion() logic change inverts the default return value. Previously it returned 3 unless playwright-mv2 was specified; now it returns 2 unless playwright is specified. When running npx playwright test directly (documented in the README for re-running tests without rebuilding), npm_lifecycle_event is undefined, causing manifestVersion to be 2 instead of 3. This breaks the documented workflow because the test harness would try to load the extension from build/chrome-mv2/dev instead of build/chrome/dev.
Please tell me if this was useful or not with a 👍 or 👎.
|
|
||
| await context.route('**/*', routeHandler); | ||
|
|
||
| console.log(`Installed Firefox extension: ${addonId}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Debug console.log not commented out
Low Severity
An active console.log statement prints to output for every Firefox test. The established convention in this file is to comment out debug logging statements (as seen on lines 66, 75, and 169). This appears to be debug code that was accidentally left enabled given the PR mentions tidying up Cursor-generated code.
Please tell me if this was useful or not with a 👍 or 👎.
| } | ||
| await new Promise((resolve) => setTimeout(resolve, polling)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Firefox waitForFunction signature incompatible with caller
Medium Severity
The FirefoxBackgroundPage.waitForFunction has signature (pageFunction, options = {}, ...args) but the existing forFunction in backgroundWait.js calls it expecting Playwright's signature (pageFunction, arg?, options?). When forSetting(bgPage, key) is called, the key argument gets misinterpreted as options instead of being passed to the evaluated function. This would cause silent failures when Firefox tests use functions like forSetting that pass arguments.
Please tell me if this was useful or not with a 👍 or 👎.
Additional Locations (1)
| context._firefoxEvalResults = rdpResult.evalResults; | ||
|
|
||
| return { context, rdpResult }; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Firefox browser and temp directory leak on setup failure
Medium Severity
In createFirefoxContext, if installExtensionViaRDP fails after Firefox has been launched (line 506), the function throws without closing the browser context or cleaning up the temp directory created at line 490. Since context._firefoxUserDataDir is only assigned after the RDP call succeeds, even calling cleanupFirefoxContext wouldn't remove the orphaned temp directory. This causes leaked Firefox processes and accumulating temp directories when extension installation fails.
Please tell me if this was useful or not with a 👍 or 👎.
d729fa8 to
4903710
Compare
Warning: These changes were initially generated with Cursor, and I am now in the
process of tidying them up.
4903710 to
95959b0
Compare
Warning: These changes were initially generated with Cursor, and I am now in the
process of tidying them up.
Note
Adds experimental Firefox support to integration tests and CI.
helpers/firefoxHarness.jsimplements RDP client, background evaluation (FirefoxBackgroundPage), context setup/cleanuphelpers/playwrightHarness.jswith Firefox detection, context wiring, unified routing,addScriptTagshim to bypass CSP on Firefoxbackground-eval.spec.jsvalidating background evaluation API across browsers; updates fingerprint tests to useaddScriptTagplaywright.firefox.config.jslimiting supported tests, single-worker stability; new npm scriptplaywright-firefoxplaywright-tests-firefoxjob (sharded, continue-on-error) installing Firefox and running the new scriptbackgroundWait.js; README documents experimental Firefox testingWritten by Cursor Bugbot for commit 95959b0. This will update automatically on new commits. Configure here.