Skip to content
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

misc: replace marionette-client with geckodriver as b2g marionette client is no longer supported #30250

Merged
merged 18 commits into from
Sep 30, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .circleci/cache-version.txt
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
# Bump this version to force CI to re-create the cache from scratch.

09-12-24
09-24-24
10 changes: 5 additions & 5 deletions .circleci/workflows.yml
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ mainBuildFilters: &mainBuildFilters
- /^release\/\d+\.\d+\.\d+$/
# use the following branch as well to ensure that v8 snapshot cache updates are fully tested
- 'update-v8-snapshot-cache-on-develop'
- 'ryanm/chore/fix-full-snapshot'
- 'misc/remove_marionette_for_geckodriver'
- 'publish-binary'

# usually we don't build Mac app - it takes a long time
Expand All @@ -42,7 +42,7 @@ macWorkflowFilters: &darwin-workflow-filters
- equal: [ develop, << pipeline.git.branch >> ]
# use the following branch as well to ensure that v8 snapshot cache updates are fully tested
- equal: [ 'update-v8-snapshot-cache-on-develop', << pipeline.git.branch >> ]
- equal: [ 'ryanm/chore/fix-full-snapshot', << pipeline.git.branch >> ]
- equal: [ 'misc/remove_marionette_for_geckodriver', << pipeline.git.branch >> ]
- matches:
pattern: /^release\/\d+\.\d+\.\d+$/
value: << pipeline.git.branch >>
Expand All @@ -53,7 +53,7 @@ linuxArm64WorkflowFilters: &linux-arm64-workflow-filters
- equal: [ develop, << pipeline.git.branch >> ]
# use the following branch as well to ensure that v8 snapshot cache updates are fully tested
- equal: [ 'update-v8-snapshot-cache-on-develop', << pipeline.git.branch >> ]
- equal: [ 'ryanm/chore/fix-full-snapshot', << pipeline.git.branch >> ]
- equal: [ 'misc/remove_marionette_for_geckodriver', << pipeline.git.branch >> ]
- matches:
pattern: /^release\/\d+\.\d+\.\d+$/
value: << pipeline.git.branch >>
Expand All @@ -76,7 +76,7 @@ windowsWorkflowFilters: &windows-workflow-filters
- equal: [ develop, << pipeline.git.branch >> ]
# use the following branch as well to ensure that v8 snapshot cache updates are fully tested
- equal: [ 'update-v8-snapshot-cache-on-develop', << pipeline.git.branch >> ]
- equal: [ 'ryanm/chore/fix-full-snapshot', << pipeline.git.branch >> ]
- equal: [ 'misc/remove_marionette_for_geckodriver', << pipeline.git.branch >> ]
- matches:
pattern: /^release\/\d+\.\d+\.\d+$/
value: << pipeline.git.branch >>
Expand Down Expand Up @@ -152,7 +152,7 @@ commands:
name: Set environment variable to determine whether or not to persist artifacts
command: |
echo "Setting SHOULD_PERSIST_ARTIFACTS variable"
echo 'if ! [[ "$CIRCLE_BRANCH" != "develop" && "$CIRCLE_BRANCH" != "release/"* && "$CIRCLE_BRANCH" != "ryanm/chore/fix-full-snapshot" ]]; then
echo 'if ! [[ "$CIRCLE_BRANCH" != "develop" && "$CIRCLE_BRANCH" != "release/"* && "$CIRCLE_BRANCH" != "misc/remove_marionette_for_geckodriver" ]]; then
export SHOULD_PERSIST_ARTIFACTS=true
fi' >> "$BASH_ENV"
# You must run `setup_should_persist_artifacts` command and be using bash before running this command
Expand Down
8 changes: 8 additions & 0 deletions cli/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,4 +1,12 @@
<!-- See the ../guides/writing-the-cypress-changelog.md for details on writing the changelog. -->
## 13.15.1

_Released 10/1/2024 (PENDING)_

**Misc:**

- Cypress now consumes [geckodriver](https://firefox-source-docs.mozilla.org/testing/geckodriver/index.html) to help automate the Firefox browser instead of [marionette-client](https://github.com/cypress-io/marionette-client). Addresses [#30217](https://github.com/cypress-io/cypress/issues/30217).

## 13.15.0

_Released 9/25/2024_
Expand Down
16 changes: 16 additions & 0 deletions cli/lib/exec/spawn.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ const state = require('../tasks/state')
const xvfb = require('./xvfb')
const verify = require('../tasks/verify')
const errors = require('../errors')
const readline = require('readline')

const isXlibOrLibudevRe = /^(?:Xlib|libudev)/
const isHighSierraWarningRe = /\*\*\* WARNING/
Expand Down Expand Up @@ -236,6 +237,21 @@ module.exports = {
child.on('exit', resolveOn('exit'))
child.on('error', reject)

if (isPlatform('win32')) {
const rl = readline.createInterface({
input: process.stdin,
output: process.stdout,
})

// on windows, SIGINT does not propagate to the child process when ctrl+c is pressed
// this makes sure all nested processes are closed(ex: firefox inside the server)
rl.on('SIGINT', function () {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to handle anything else? SIGTERM? SIGUSR1 or SIGUSR2?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not that I am aware of but Im not sure on all the scenarios the process could be killed in the terminal. Either way it should be pretty easy to add support for if needed if a regression is detected that we missed

let kill = require('tree-kill')

kill(child.pid, 'SIGINT')
})
}

// if stdio options is set to 'pipe', then
// we should set up pipes:
// process STDIN (read stream) => child STDIN (writeable)
Expand Down
1 change: 1 addition & 0 deletions cli/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@
"semver": "^7.5.3",
"supports-color": "^8.1.1",
"tmp": "~0.2.3",
"tree-kill": "1.2.2",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jennifer-shehane should we mention in the changelog that we are adding the tree-kill dependency to the CLI since we mention dependency updates?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't recall adding a line for added dependencies before

"untildify": "^4.0.0",
"yauzl": "^2.10.0"
},
Expand Down
23 changes: 23 additions & 0 deletions cli/test/lib/exec/spawn_spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@ const tty = require('tty')
const path = require('path')
const EE = require('events')
const mockedEnv = require('mocked-env')
const readline = require('readline')
const proxyquire = require('proxyquire')

const debug = require('debug')('test')

const state = require(`${lib}/tasks/state`)
Expand All @@ -22,6 +25,7 @@ const execPath = process.execPath
const nodeVersion = process.versions.node

const defaultBinaryDir = '/default/binary/dir'
let mockReadlineEE

describe('lib/exec/spawn', function () {
beforeEach(function () {
Expand Down Expand Up @@ -49,8 +53,11 @@ describe('lib/exec/spawn', function () {

// process.stdin is both an event emitter and a readable stream
this.processStdin = new EE()
mockReadlineEE = new EE()

this.processStdin.pipe = sinon.stub().returns(undefined)
sinon.stub(process, 'stdin').value(this.processStdin)
sinon.stub(readline, 'createInterface').returns(mockReadlineEE)
sinon.stub(cp, 'spawn').returns(this.spawnedProcess)
sinon.stub(xvfb, 'start').resolves()
sinon.stub(xvfb, 'stop').resolves()
Expand Down Expand Up @@ -387,6 +394,22 @@ describe('lib/exec/spawn', function () {
})
})

it('propagates treeKill if SIGINT is detected in windows console', async function () {
this.spawnedProcess.pid = 7
this.spawnedProcess.on.withArgs('close').yieldsAsync(0)

os.platform.returns('win32')

const treeKillMock = sinon.stub().returns(0)

const spawn = proxyquire(`${lib}/exec/spawn`, { 'tree-kill': treeKillMock })

await spawn.start([], { env: {} })

mockReadlineEE.emit('SIGINT')
expect(treeKillMock).to.have.been.calledWith(7, 'SIGINT')
})

it('does not set windowsHide property when in darwin', function () {
this.spawnedProcess.on.withArgs('close').yieldsAsync(0)

Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions packages/errors/src/errors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1218,11 +1218,11 @@ export const AllCypressErrors = {

The error was: ${fmt.highlightSecondary(errMsg)}`
},
FIREFOX_MARIONETTE_FAILURE: (origin: string, err: Error) => {
FIREFOX_GECKODRIVER_FAILURE: (origin: string, err: Error) => {
return errTemplate`\
Cypress could not connect to Firefox.

An unexpected error was received from Marionette: ${fmt.highlightSecondary(origin)}
An unexpected error was received from GeckoDriver: ${fmt.highlightSecondary(origin)}

To avoid this error, ensure that there are no other instances of Firefox launched by Cypress running.

Expand Down
2 changes: 1 addition & 1 deletion packages/errors/test/unit/visualSnapshotErrors_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1134,7 +1134,7 @@ describe('visual error templates', () => {
default: ['spec', '1', 'spec must be a string or comma-separated list'],
}
},
FIREFOX_MARIONETTE_FAILURE: () => {
FIREFOX_GECKODRIVER_FAILURE: () => {
const err = makeErr()

return {
Expand Down
2 changes: 1 addition & 1 deletion packages/graphql/schemas/schema.graphql
Original file line number Diff line number Diff line change
Expand Up @@ -1203,7 +1203,7 @@ enum ErrorTypeEnum {
EXTENSION_NOT_LOADED
FIREFOX_COULD_NOT_CONNECT
FIREFOX_GC_INTERVAL_REMOVED
FIREFOX_MARIONETTE_FAILURE
FIREFOX_GECKODRIVER_FAILURE
FIXTURE_NOT_FOUND
FOLDER_NOT_WRITABLE
FREE_PLAN_EXCEEDS_MONTHLY_PRIVATE_TESTS
Expand Down
3 changes: 2 additions & 1 deletion packages/launcher/lib/browsers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ export const debug = Debug('cypress:launcher:browsers')
/** starts a found browser and opens URL if given one */
export type LaunchedBrowser = cp.ChildProcessByStdio<null, Readable, Readable>

// NOTE: For Firefox, geckodriver is used to launch the browser
export function launch (
browser: FoundBrowser,
url: string,
Expand All @@ -28,7 +29,7 @@ export function launch (

const spawnOpts: cp.SpawnOptionsWithStdioTuple<cp.StdioNull, cp.StdioPipe, cp.StdioPipe> = {
stdio: ['ignore', 'pipe', 'pipe'],
// allow setting default env vars such as MOZ_HEADLESS_WIDTH
// allow setting default env vars
// but only if it's not already set by the environment
env: { ...browserEnv, ...process.env },
}
Expand Down
2 changes: 1 addition & 1 deletion packages/server/lib/browsers/browser-cri-client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,7 @@ export class BrowserCriClient {
*
* @param {BrowserCriClientCreateOptions} options the options for creating the browser cri client
* @param options.browserName the display name of the browser being launched
* @param options.fullyManageTabs whether or not to fully manage tabs. This is useful for firefox where some work is done with marionette and some with CDP. We don't want to handle disconnections in this class in those scenarios
* @param options.fullyManageTabs whether or not to fully manage tabs. This is useful for firefox where some work is done with GeckoDriver and some with CDP. We don't want to handle disconnections in this class in those scenarios
* @param options.hosts the hosts to which to attempt to connect
* @param options.onAsynchronousError callback for any cdp fatal errors
* @param options.onReconnect callback for when the browser cri client reconnects to the browser
Expand Down
Loading
Loading