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 all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 4 additions & 6 deletions circle.yml
Expand Up @@ -36,8 +36,7 @@ macWorkflowFilters: &darwin-workflow-filters
when:
or:
- equal: [ develop, << pipeline.git.branch >> ]
- equal: [ 'correct-dashboard-results', << 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 @@ -46,7 +45,7 @@ linuxArm64WorkflowFilters: &linux-arm64-workflow-filters
when:
or:
- equal: [ develop, << 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 +65,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 +130,7 @@ commands:
- run:
name: Check current branch to persist artifacts
command: |
if [[ "$CIRCLE_BRANCH" != "develop" && "$CIRCLE_BRANCH" != "correct-dashboard-results" ]]; 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
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 @@ -2883,6 +2883,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-experiment 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
2 changes: 1 addition & 1 deletion packages/app/src/main.ts
Expand Up @@ -22,7 +22,7 @@ const app = createApp(App)

const config = getRunnerConfigFromWindow()

const ws = createWebsocket(config.socketIoRoute)
const ws = createWebsocket(config)

window.ws = ws

Expand Down
17 changes: 5 additions & 12 deletions packages/app/src/runner/index.ts
Expand Up @@ -30,18 +30,11 @@ import { useStudioStore } from '../store/studio-store'

let _eventManager: EventManager | undefined

export function createWebsocket (socketIoRoute: string) {
const socketConfig = {
path: socketIoRoute,
transports: ['websocket'],
}

const ws = client(socketConfig)

ws.on('connect_error', () => {
// fall back to polling if websocket fails to connect (webkit)
// https://github.com/socketio/socket.io/discussions/3998#discussioncomment-972316
ws.io.opts.transports = ['polling', 'websocket']
export function createWebsocket (config: Cypress.Config) {
const ws = client({
path: config.socketIoRoute,
// TODO(webkit): the websocket socket.io transport is busted in WebKit, need polling
transports: config.browser.family === 'webkit' ? ['polling'] : ['websocket'],
})

ws.on('connect', () => {
Expand Down
3 changes: 3 additions & 0 deletions packages/config/__snapshots__/index.spec.ts.js
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
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
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
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
2 changes: 2 additions & 0 deletions packages/data-context/src/data/ProjectLifecycleManager.ts
Expand Up @@ -380,6 +380,8 @@ export class ProjectLifecycleManager {
}

private _setCurrentProject (projectRoot: string) {
process.chdir(projectRoot)
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder why we needed to add this specifically during this PR? Or was it just a drive-by fix or sorts, not actually required for WK?
Edit: we need it for this reason:

Fixes the issue where playwright-webkit is resolved from the sourcedir, not the projectRoot, outside of development - e8c0d9a

Will leave this note to assist other reviews who might have the same question.


this._projectRoot = projectRoot
this._initializedProject = undefined

Expand Down
19 changes: 10 additions & 9 deletions packages/driver/cypress.config.ts
@@ -1,22 +1,23 @@
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',
'*.barbaz.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',
},
})
3 changes: 2 additions & 1 deletion packages/driver/cypress/e2e/cypress/proxy-logging.cy.ts
Expand Up @@ -321,7 +321,8 @@ describe('Proxy Logging', () => {
})
})

it('works with forceNetworkError', () => {
// TODO(webkit): fix forceNetworkError and unskip
it('works with forceNetworkError', { browser: '!webkit' }, () => {
const logs: any[] = []

cy.on('log:added', (log) => {
Expand Down
31 changes: 31 additions & 0 deletions packages/driver/cypress/e2e/webkit.cy.ts
@@ -0,0 +1,31 @@
describe('WebKit-specific behavior', { browser: 'webkit' }, () => {
it('cy.origin() is disabled', (done) => {
cy.on('fail', (err) => {
expect(err.message).to.equal('`cy.origin()` is not currently supported in experimental WebKit.')
expect(err.docsUrl).to.equal('https://on.cypress.io/webkit-experiment')
done()
})

cy.origin('foo', () => {})
})

it('cy.session() is disabled', (done) => {
cy.on('fail', (err) => {
expect(err.message).to.equal('`cy.session()` is not currently supported in experimental WebKit.')
expect(err.docsUrl).to.equal('https://on.cypress.io/webkit-experiment')
done()
})

cy.session('foo', () => {})
})

it('cy.session() is disabled', (done) => {
cy.on('fail', (err) => {
expect(err.message).to.include('`forceNetworkError` was passed, but it is not currently supported in experimental WebKit.')
expect(err.docsUrl).to.equal('https://on.cypress.io/intercept')
done()
})

cy.intercept('http://foo.com', { forceNetworkError: true })
})
})
4 changes: 4 additions & 0 deletions packages/driver/src/cy/commands/origin/index.ts
Expand Up @@ -54,6 +54,10 @@ export default (Commands, Cypress: Cypress.Cypress, cy: Cypress.cy, state: State

Commands.addAll({
origin<T> (urlOrDomain: string, optionsOrFn: { args: T } | (() => {}), fn?: (args?: T) => {}) {
if (Cypress.isBrowser('webkit')) {
return $errUtils.throwErrByPath('webkit.origin')
}

const userInvocationStack = state('current').get('userInvocationStack')

// store the invocation stack in the case that `cy.origin` errors
Expand Down
4 changes: 4 additions & 0 deletions packages/driver/src/cy/commands/sessions/index.ts
Expand Up @@ -53,6 +53,10 @@ export default function (Commands, Cypress, cy) {

Commands.addAll({
session (id, setup?: Function, options: { validate?: Function } = {}) {
if (Cypress.isBrowser('webkit')) {
return $errUtils.throwErrByPath('webkit.session')
}

throwIfNoSessionSupport()

if (!id || !_.isString(id) && !_.isObject(id)) {
Expand Down
4 changes: 4 additions & 0 deletions packages/driver/src/cy/net-stubbing/static-response-utils.ts
Expand Up @@ -24,6 +24,10 @@ export function validateStaticResponse (cmd: string, staticResponse: StaticRespo
err('`forceNetworkError`, if passed, must be the only option in the StaticResponse.')
}

if (forceNetworkError && Cypress.isBrowser('webkit')) {
err('`forceNetworkError` was passed, but it is not currently supported in experimental WebKit.')
}

if (body && fixture) {
err('`body` and `fixture` cannot both be set, pick one.')
}
Expand Down
6 changes: 6 additions & 0 deletions packages/driver/src/cypress/error_messages.ts
Expand Up @@ -2297,6 +2297,12 @@ export default {
},
},

webkit: {
docsUrl: 'https://on.cypress.io/webkit-experiment',
origin: '`cy.origin()` is not currently supported in experimental WebKit.',
session: '`cy.session()` is not currently supported in experimental WebKit.',
},

window: {
iframe_doc_undefined: 'The remote iframe\'s document is `undefined`',
iframe_undefined: 'The remote iframe is `undefined`',
Expand Down
6 changes: 5 additions & 1 deletion packages/driver/src/cypress/script_utils.ts
Expand Up @@ -17,7 +17,11 @@ const extractSourceMap = ([script, contents]) => {
const sourceMap = $sourceMapUtils.extractSourceMap(script, contents)

return $sourceMapUtils.initializeSourceMapConsumer(script, sourceMap)
.return([script, contents])
.catch((_err) => {
flotwig marked this conversation as resolved.
Show resolved Hide resolved
// if WebAssembly is missing, we can't consume source maps, but it shouldn't block Cy
// like in WebKit on Windows: https://github.com/microsoft/playwright/issues/2876
})
.then(() => [script, contents])
}

const evalScripts = (specWindow, scripts: any = []) => {
Expand Down
13 changes: 6 additions & 7 deletions packages/driver/src/cypress/source_map_utils.ts
@@ -1,6 +1,5 @@
import _ from 'lodash'
import { SourceMapConsumer } from 'source-map'
import Promise from 'bluebird'
Copy link
Contributor

Choose a reason for hiding this comment

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

Bluebird is not not bad but it contributes to "the code base showing it's age". I hope we can just rip all of bluebird out in one fowl swoop sometime.

pun absolute intended

Copy link
Contributor Author

@flotwig flotwig Sep 9, 2022

Choose a reason for hiding this comment

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

😆 Yeah, this one in particular was needed to fix typings.


// @ts-ignore
import mappingsWasm from 'source-map/lib/mappings.wasm'
Expand All @@ -12,19 +11,19 @@ const regexDataUrl = /data:[^;\n]+(?:;charset=[^;\n]+)?;base64,([a-zA-Z0-9+/]+={

let sourceMapConsumers = {}

const initializeSourceMapConsumer = (file, sourceMap) => {
if (!sourceMap) return Promise.resolve(null)
const initializeSourceMapConsumer = async (file, sourceMap) => {
if (!sourceMap) return null

// @ts-ignore
SourceMapConsumer.initialize({
'lib/mappings.wasm': mappingsWasm,
})

return Promise.resolve(new SourceMapConsumer(sourceMap)).then((consumer) => {
sourceMapConsumers[file.fullyQualifiedUrl] = consumer
const consumer = await new SourceMapConsumer(sourceMap)

return consumer
})
sourceMapConsumers[file.fullyQualifiedUrl] = consumer

return consumer
}

const extractSourceMap = (file, fileContents) => {
Expand Down

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
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
1 change: 1 addition & 0 deletions packages/frontend-shared/cypress/e2e/e2ePluginSetup.ts
Expand Up @@ -174,6 +174,7 @@ async function makeE2ETasks () {
* Called before each test to do global setup/cleanup
*/
async __internal__beforeEach () {
process.chdir(cachedCwd)
testState = {}
await DataContext.waitForActiveRequestsToFlush()
await globalPubSub.emitThen('test:cleanup')
Expand Down
8 changes: 6 additions & 2 deletions packages/frontend-shared/cypress/fixtures/config.json
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