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

feat: gate WebKit behind experimentalWebKitSupport in prod #23711

Merged
merged 22 commits into from
Sep 13, 2022
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
11 changes: 4 additions & 7 deletions circle.yml
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,7 @@ macWorkflowFilters: &darwin-workflow-filters
when:
or:
- equal: [ develop, << pipeline.git.branch >> ]
- equal: [ 'tbiethman/22272-globbing-working-dir', << pipeline.git.branch >> ]
- equal: [ 'skip-or-fix-flaky-tests-2', << pipeline.git.branch >> ]
- equal: [ 'webkit-experimental', << pipeline.git.branch >> ]
- matches:
pattern: "-release$"
value: << pipeline.git.branch >>
Expand All @@ -45,8 +44,7 @@ linuxArm64WorkflowFilters: &linux-arm64-workflow-filters
when:
or:
- equal: [ develop, << pipeline.git.branch >> ]
- equal: [ "lmiller/experimental-single-tab-component-testing", << pipeline.git.branch >> ]
- equal: [ 'skip-or-fix-flaky-tests-2', << pipeline.git.branch >> ]
- equal: [ 'webkit-experimental', << pipeline.git.branch >> ]
- matches:
pattern: "-release$"
value: << pipeline.git.branch >>
Expand All @@ -66,8 +64,7 @@ windowsWorkflowFilters: &windows-workflow-filters
or:
- equal: [ develop, << pipeline.git.branch >> ]
- equal: [ linux-arm64, << pipeline.git.branch >> ]
- equal: [ 'lmiller/fixing-flake-1', << pipeline.git.branch >> ]
- equal: [ 'skip-or-fix-flaky-tests-2', << pipeline.git.branch >> ]
- equal: [ 'webkit-experimental', << pipeline.git.branch >> ]
- matches:
pattern: "-release$"
value: << pipeline.git.branch >>
Expand Down Expand Up @@ -132,7 +129,7 @@ commands:
- run:
name: Check current branch to persist artifacts
command: |
if [[ "$CIRCLE_BRANCH" != "develop" && "$CIRCLE_BRANCH" != "tbiethman/23380-root-spec-pattern" ]]; then
if [[ "$CIRCLE_BRANCH" != "develop" && "$CIRCLE_BRANCH" != "webkit-experimental" ]]; then
echo "Not uploading artifacts or posting install comment for this branch."
circleci-agent step halt
fi
Expand Down
7 changes: 6 additions & 1 deletion cli/types/cypress.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ declare namespace Cypress {

type BrowserChannel = 'stable' | 'canary' | 'beta' | 'dev' | 'nightly' | string

type BrowserFamily = 'chromium' | 'firefox'
type BrowserFamily = 'chromium' | 'firefox' | 'webkit'

/**
* Describes a browser Cypress can control
Expand Down Expand Up @@ -2876,6 +2876,11 @@ declare namespace Cypress {
* @default false
*/
experimentalStudio: boolean
/**
* Adds support for testing in the WebKit browser engine used by Safari. See https://on.cypress.io/webkit for more information.
* @default false
*/
experimentalWebKitSupport: boolean
/**
* Number of times to retry a failed test.
* If a number is set, tests will retry in both runMode and openMode.
Expand Down
3 changes: 3 additions & 0 deletions packages/config/__snapshots__/index.spec.ts.js
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ exports['config/src/index .getDefaultValues returns list of public config keys 1
"experimentalSourceRewriting": false,
"experimentalSingleTabRunMode": false,
"experimentalStudio": false,
"experimentalWebKitSupport": false,
"fileServerFolder": "",
"fixturesFolder": "cypress/fixtures",
"excludeSpecPattern": "*.hot-update.js",
Expand Down Expand Up @@ -123,6 +124,7 @@ exports['config/src/index .getDefaultValues returns list of public config keys f
"experimentalSourceRewriting": false,
"experimentalSingleTabRunMode": false,
"experimentalStudio": false,
"experimentalWebKitSupport": false,
"fileServerFolder": "",
"fixturesFolder": "cypress/fixtures",
"excludeSpecPattern": "*.hot-update.js",
Expand Down Expand Up @@ -202,6 +204,7 @@ exports['config/src/index .getPublicConfigKeys returns list of public config key
"experimentalSourceRewriting",
"experimentalSingleTabRunMode",
"experimentalStudio",
"experimentalWebKitSupport",
"fileServerFolder",
"fixturesFolder",
"excludeSpecPattern",
Expand Down
6 changes: 6 additions & 0 deletions packages/config/src/options.ts
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,12 @@ const driverConfigOptions: Array<DriverConfigOption> = [
validation: validate.isBoolean,
isExperimental: true,
requireRestartOnChange: 'browser',
}, {
name: 'experimentalWebKitSupport',
defaultValue: false,
validation: validate.isBoolean,
isExperimental: true,
requireRestartOnChange: 'server',
}, {
name: 'fileServerFolder',
defaultValue: '',
Expand Down
2 changes: 2 additions & 0 deletions packages/config/test/project/utils.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1044,6 +1044,7 @@ describe('config/src/project/utils', () => {
experimentalSingleTabRunMode: { value: false, from: 'default' },
experimentalStudio: { value: false, from: 'default' },
experimentalSourceRewriting: { value: false, from: 'default' },
experimentalWebKitSupport: { value: false, from: 'default' },
fileServerFolder: { value: '', from: 'default' },
fixturesFolder: { value: 'cypress/fixtures', from: 'default' },
hosts: { value: null, from: 'default' },
Expand Down Expand Up @@ -1137,6 +1138,7 @@ describe('config/src/project/utils', () => {
experimentalSingleTabRunMode: { value: false, from: 'default' },
experimentalStudio: { value: false, from: 'default' },
experimentalSourceRewriting: { value: false, from: 'default' },
experimentalWebKitSupport: { value: false, from: 'default' },
env: {
foo: {
value: 'foo',
Expand Down
18 changes: 13 additions & 5 deletions packages/data-context/src/data/ProjectConfigManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -500,14 +500,22 @@ export class ProjectConfigManager {
}

fullConfig.browsers = fullConfig.browsers?.map((browser) => {
if (browser.family === 'chromium' || fullConfig.chromeWebSecurity) {
return browser
if (browser.family === 'webkit' && !fullConfig.experimentalWebKitSupport) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I see what you mean by confusing now, there's about 20 places we modify the config, and it's not clear if this is the right place to do it, or one of the other 19 🤔

return {
...browser,
disabled: true,
warning: '`playwright-webkit` is installed and WebKit is detected, but `experimentalWebKitSupport` is not enabled in your Cypress config. Set it to `true` to use WebKit.',
}
}

return {
...browser,
warning: browser.warning || getError('CHROME_WEB_SECURITY_NOT_SUPPORTED', browser.name).message,
if (browser.family !== 'chromium' && fullConfig.chromeWebSecurity) {
return {
...browser,
warning: browser.warning || getError('CHROME_WEB_SECURITY_NOT_SUPPORTED', browser.name).message,
}
}

return browser
})

// If we have withBrowsers set to false, it means we're coming from the legacy config.get API
Expand Down
19 changes: 10 additions & 9 deletions packages/driver/cypress.config.ts
Original file line number Diff line number Diff line change
@@ -1,21 +1,22 @@
import { defineConfig } from 'cypress'

export default defineConfig({
'projectId': 'ypt4pf',
'experimentalStudio': true,
'hosts': {
projectId: 'ypt4pf',
experimentalStudio: true,
experimentalWebKitSupport: true,
hosts: {
'*.foobar.com': '127.0.0.1',
'*.idp.com': '127.0.0.1',
'localalias': '127.0.0.1',
},
'reporter': 'cypress-multi-reporters',
'reporterOptions': {
'configFile': '../../mocha-reporter-config.json',
reporter: 'cypress-multi-reporters',
reporterOptions: {
configFile: '../../mocha-reporter-config.json',
},
'e2e': {
'setupNodeEvents': (on, config) => {
e2e: {
setupNodeEvents: (on, config) => {
return require('./cypress/plugins')(on, config)
},
'baseUrl': 'http://localhost:3500',
baseUrl: 'http://localhost:3500',
},
})

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

2 changes: 1 addition & 1 deletion packages/errors/src/errors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ export const AllCypressErrors = {
},
CHROME_WEB_SECURITY_NOT_SUPPORTED: (browser: string) => {
return errTemplate`\
Your project has set the configuration option: ${fmt.highlight(`chromeWebSecurity`)} to ${fmt.highlightTertiary(`false`)}
Your project has set the configuration option: \`chromeWebSecurity\` to \`false\`.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

launchpad is not rendering ANSI escape sequences like desktop-gui once did, so I changed this to a plain text representation

lmiller1990 marked this conversation as resolved.
Show resolved Hide resolved

This option will not have an effect in ${fmt.off(_.capitalize(browser))}. Tests that rely on web security being disabled will not run as expected.`
},
Expand Down
8 changes: 6 additions & 2 deletions packages/frontend-shared/cypress/fixtures/config.json
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,7 @@
"displayName": "Electron",
"version": "94.0.4606.81",
"path": "",
"majorVersion": 94,
"info": "Electron is the default browser that comes with Cypress. This is the default browser that runs in headless mode. Selecting this browser is useful when debugging. The version number indicates the underlying Chromium version that Electron uses."
"majorVersion": 94
}
],
"from": "default",
Expand Down Expand Up @@ -103,6 +102,11 @@
"from": "default",
"field": "experimentalSessionAndOrigin"
},
{
"value": false,
"from": "default",
"field": "experimentalWebKitSupport"
},
{
"value": "",
"from": "default",
Expand Down
4 changes: 4 additions & 0 deletions packages/frontend-shared/src/locales/en-US.json
Original file line number Diff line number Diff line change
Expand Up @@ -507,6 +507,10 @@
"experimentalStudio": {
"name": "Studio",
"description": "Generate and save commands directly to your test suite by interacting with your app as an end user would."
},
"experimentalWebKitSupport": {
"name": "WebKit Support",
"description": "Adds support for testing in the WebKit browser engine used by Safari. See https://on.cypress.io/webkit for more information."
}
},
"device": {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ export const Browser = objectType({
definition (t) {
t.nonNull.string('channel')
t.nonNull.boolean('disabled', {
lmiller1990 marked this conversation as resolved.
Show resolved Hide resolved
resolve: () => false,
resolve: (source) => source.disabled || false,
})

t.nonNull.boolean('isSelected', {
Expand Down
7 changes: 5 additions & 2 deletions packages/launchpad/src/setup/OpenBrowserList.vue
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
}"
>
<Tooltip
v-if="!browser.isVersionSupported"
v-if="browser.warning"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This also fixes an issue where the chromeWebSecurity warning did not display.

Copy link
Contributor

Choose a reason for hiding this comment

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

Did we add a regression test for this somewhere? Not sure I see it

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you'd add it in this file potentially: https://github.com/cypress-io/cypress/blob/develop/packages/launchpad/cypress/e2e/config-files-error-handling.cy.ts#L96

Just pick a system-tests project that has chromeWebSecurity: false and it should show up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are percy snapshots for the warning icon itself, does that cover the need for a test?

popper-class="max-w-lg"
>
<i-cy-circle-bg-question-mark_x16
Expand All @@ -37,7 +37,10 @@
/>
<template #popper>
<div class="text-center p-2 text-gray-300 text-size-14px leading-20px">
<div class="font-medium text-white mb-2">
<div
v-if="!browser.isVersionSupported"
class="font-medium text-white mb-2"
>
Unsupported browser
</div>
{{ browser.warning }}
Expand Down
2 changes: 1 addition & 1 deletion packages/server/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ if (process.env.CY_NET_PROFILE && isRunningElectron) {
process.stdout.write(`Network profiler writing to ${netProfiler.logPath}\n`)
}

require('./lib/unhandled_exceptions')
require('./lib/unhandled_exceptions').handle()

process.env.UV_THREADPOOL_SIZE = 128

Expand Down
12 changes: 2 additions & 10 deletions packages/server/lib/browsers/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,8 @@ export = {

formatBrowsersToOptions: utils.formatBrowsersToOptions,

setFocus,

_setInstance (_instance: BrowserInstance) {
// for testing
instance = _instance
Expand All @@ -110,15 +112,6 @@ export = {
return instance
},

getAllBrowsersWith (nameOrPath?: string) {
debug('getAllBrowsersWith %o', { nameOrPath })
if (nameOrPath) {
return utils.ensureAndGetByNameOrPath(nameOrPath, true)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This code path was never used so I removed this function entirely.

}

return utils.getBrowsers()
},

async connectToExisting (browser: Browser, options: BrowserLaunchOpts, automation: Automation): Promise<BrowserInstance | null> {
const browserLauncher = await getBrowserLauncher(browser, options.browsers)

Expand Down Expand Up @@ -196,5 +189,4 @@ export = {

return instance
},
setFocus,
} as const
23 changes: 10 additions & 13 deletions packages/server/lib/browsers/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -204,10 +204,10 @@ const getWebKitBrowserVersion = async () => {
}
}

const getWebKitBrowser = async () => {
async function getWebKitBrowser () {
try {
const modulePath = require.resolve('playwright-webkit', { paths: [process.cwd()] })
const mod = require(modulePath) as typeof import('playwright-webkit')
const mod = await import(modulePath) as typeof import('playwright-webkit')
lmiller1990 marked this conversation as resolved.
Show resolved Hide resolved
const version = await getWebKitBrowserVersion()

const browser: FoundBrowser = {
Expand All @@ -218,7 +218,7 @@ const getWebKitBrowser = async () => {
version,
path: mod.webkit.executablePath(),
majorVersion: version.split('.')[0],
warning: 'WebKit support is not currently available in production.',
warning: 'WebKit support is currently experimental. Some functions may not work as expected.',
}

return browser
Expand All @@ -232,8 +232,12 @@ const getWebKitBrowser = async () => {
const getBrowsers = async () => {
debug('getBrowsers')

const browsers = await launcher.detect()
let majorVersion
const [browsers, wkBrowser] = await Promise.all([
launcher.detect(),
getWebKitBrowser(),
])

if (wkBrowser) browsers.push(wkBrowser)

debug('found browsers %o', { browsers })

Expand All @@ -243,8 +247,8 @@ const getBrowsers = async () => {
return browsers
}

// @ts-ignore
const version = process.versions.chrome || ''
let majorVersion

if (version) {
majorVersion = getMajorVersion(version)
Expand All @@ -258,17 +262,10 @@ const getBrowsers = async () => {
version,
path: '',
majorVersion,
info: 'Electron is the default browser that comes with Cypress. This is the default browser that runs in headless mode. Selecting this browser is useful when debugging. The version number indicates the underlying Chromium version that Electron uses.',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the subject of #21769 - info display has been broken since 10.0.0. I've just removed it in lieu of adding the info display back in.

}

browsers.push(electronBrowser)

if (process.env.CYPRESS_INTERNAL_ENV !== 'production') {
const wkBrowser = await getWebKitBrowser()

if (wkBrowser) browsers.push(wkBrowser)
}

return browsers
}

Expand Down
Loading