-
Notifications
You must be signed in to change notification settings - Fork 3.4k
chore: bump firefox version we test against to 141 #32078
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
Changes from all commits
17901cd
e97be81
90a6ff4
4b3e693
eebd63d
d3f69ec
61876bd
250d87c
c55cc85
45769a5
f3fc00c
4aa461e
720d5c6
2913403
aea86b2
5f5e958
0bcdb4c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -409,25 +409,31 @@ export function clearInstanceState (options: GracefulShutdownOptions = {}) { | |
| } | ||
| } | ||
|
|
||
| function shouldUseBiDi (browser: Browser): boolean { | ||
| function shouldUseCdp (browser: Browser): boolean { | ||
| try { | ||
| // Gating on firefox version 135 to turn on BiDi as this is when all of our internal Cypress tests were able to pass. | ||
| return (browser.family === 'firefox' && !process.env.FORCE_FIREFOX_CDP && Number(browser.majorVersion) >= 135) | ||
| // CDP is not supported in Firefox 141+, so we always use BiDi for FF141+. | ||
| return ( | ||
| browser.family === 'firefox' && ( | ||
| Number(browser.majorVersion) < 135 || | ||
| (Number(browser.majorVersion) < 141 && !!process.env.FORCE_FIREFOX_CDP) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would say it might be worth mentioning
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's implied by the new warning message |
||
| ) | ||
| ) | ||
| } catch (err: unknown) { | ||
| return false | ||
| } | ||
| } | ||
|
|
||
| export async function connectToNewSpec (browser: Browser, options: BrowserNewTabOpts, automation: Automation) { | ||
| if (shouldUseBiDi(browser)) { | ||
| if (shouldUseCdp(browser)) { | ||
| debug('connectToNewSpec cdp') | ||
| await firefoxUtil.connectToNewSpecCDP(options, automation, browserCriClient!) | ||
| } else { | ||
| debug('connectToNewSpec bidi') | ||
| await firefoxUtil.connectToNewSpecBiDi(options, automation, browserBidiClient!) | ||
|
|
||
| debug('registering middleware') | ||
| automation.use(browserBidiClient!.automationMiddleware) | ||
| } else { | ||
| debug('connectToNewSpec cdp') | ||
| await firefoxUtil.connectToNewSpecCDP(options, automation, browserCriClient!) | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -450,9 +456,10 @@ async function recordVideo (videoApi: RunModeVideoApi) { | |
| } | ||
|
|
||
| export async function open (browser: Browser, url: string, options: BrowserLaunchOpts, automation: Automation): Promise<BrowserInstance> { | ||
| const USE_WEBDRIVER_BIDI = shouldUseBiDi(browser) | ||
| const USE_WEBDRIVER_BIDI = !shouldUseCdp(browser) | ||
|
|
||
| if (!USE_WEBDRIVER_BIDI) { | ||
| // Even if the user has set FORCE_FIREFOX_CDP, we override this and use BiDi in FF141+ | ||
| if (!USE_WEBDRIVER_BIDI || (USE_WEBDRIVER_BIDI && !!process.env.FORCE_FIREFOX_CDP)) { | ||
| errors.warning('CDP_FIREFOX_DEPRECATED') | ||
| } | ||
|
|
||
|
|
@@ -620,6 +627,8 @@ export async function open (browser: Browser, url: string, options: BrowserLaunc | |
|
|
||
| debug('launch in firefox', { url, args: launchOptions.args }) | ||
|
|
||
| const { FORCE_FIREFOX_CDP, ...launchEnvs } = process.env | ||
|
|
||
| const geckoDriverOptions: GeckodriverParameters = { | ||
| host: '127.0.0.1', | ||
| // geckodriver port is assigned under the hood by @wdio/utils | ||
|
|
@@ -636,7 +645,7 @@ export async function open (browser: Browser, url: string, options: BrowserLaunc | |
| stdio: ['ignore', 'pipe', 'pipe'], | ||
| env: { | ||
| ...BROWSER_ENVS, | ||
| ...process.env, | ||
| ...launchEnvs, | ||
| }, | ||
| }, | ||
| jsdebugger: Debug.enabled(GECKODRIVER_DEBUG_NAMESPACE_VERBOSE) || false, | ||
|
|
||
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.
I think the comment is helpful, but do we need the conditional logic as we really test the version of firefox rolling forward?
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.
We still test against FF 134 in CI
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.
That's my fault I keep thinking Cypress 15 🥴