feat: gate WebKit behind experimentalWebKitSupport in prod #23711

flotwig · 2022-09-07T21:21:04Z

For Safari (WebKit) Support #6422
Closes Show tooltips for "info" property of browsers #21769 (removes browser.info)

User facing changelog

Experimental support for running tests in WebKit, Safari's browser engine, has been added to Cypress. Set the experimentalWebKitSupport config value to true to opt-in to this experiment.

Additional details

To get around the fact that config.browsers is modifiable along with the rest of the config during setupNodeEvents, these are the detection rules:
- Always detect+display if playwright-webkit is installed.
- If experimentalWebKitSupport is not set, disable the browser and error if it's launched in run mode. In Open mode, display it greyed out and with a warning.
Adds errors to known-broken commands.
Fixes an issue with ECONNRESET on Linux - d29b5c6
Fixes the issue where playwright-webkit is resolved from the sourcedir, not the projectRoot, outside of development - e8c0d9a
Fixes an issue where Cypress failed in WK on Windows due to missing WebAssembly: f396c41
Fixes an issue where initial WK load would take up to 20 seconds: 3bd8a3c

Steps to test

How has the user experience changed?

In cypress run...
- ...Cy exits with an error if experimentalWebKitSupport is false and --browser webkit is passed:
- Otherwise Cy attempts to require the user's playwright-webkit and use it to run tests.
In cypress open...
- ...Cy does not behave differently if experimentalWebKitSupport is enabled but playwright-webkit is not installed, and no --browser --testingType is passed. Attempting to launch with the module missing results in a require error.
- ...Cy displays WebKit disabled if experimentalWebKitSupport is false but playwright-webkit IS installed:
- ...Cy errors on project load if experimentalWebKitSupport is false and --testingType --browser webkit is passed to try and quicklaunch into WK:

PR Tasks

Have tests been added/updated?
Has the original issue (or this PR, if no issue exists) been tagged with a release in ZenHub? (user-facing changes only)
Has a PR for user-facing changes been opened in cypress-documentation?
Have API changes been updated in the type definitions? docs: document experimentalWebKitSupport cypress-documentation#4710

cypress-bot · 2022-09-07T21:21:07Z

Thanks for taking the time to open a PR!

Create a Draft Pull Request if your PR is not ready for review. Mark the PR as Ready for Review when you're ready for a Cypress team member to review the PR.
Become familiar with the Code Review Checklist for guidelines on coding standards and what needs to be done before a PR can be merged.

flotwig · 2022-09-07T21:22:16Z

packages/errors/src/errors.ts

@@ -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\`.


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

flotwig · 2022-09-07T21:31:42Z

packages/launchpad/src/setup/OpenBrowserList.vue

@@ -27,7 +27,7 @@
          }"
        >
          <Tooltip
-            v-if="!browser.isVersionSupported"
+            v-if="browser.warning"


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

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

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.

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

flotwig · 2022-09-07T21:32:25Z

packages/server/lib/browsers/index.ts

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


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

flotwig · 2022-09-07T21:33:42Z

packages/server/lib/browsers/utils.ts

@@ -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.',


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.

flotwig · 2022-09-07T21:34:05Z

packages/server/lib/browsers/webkit.ts

 export async function open (browser: Browser, url: string, options: BrowserLaunchOpts, automation: Automation): Promise<BrowserInstance> {
+  if (!options.experimentalWebKitSupport) {
+    throw new Error('WebKit was launched, but the experimental feature was not enabled. Please add `experimentalWebKitSupport: true` to your config file to launch WebKit.')


This is reached if the user passes --testingType --browser webkit and bypasses the GUI.

cypress · 2022-09-07T21:53:43Z

Test summary

59 • 0 • 11 • 0 • 0

Run details


Project	cypress
Status	Passed
Commit	`af8d3a1`
Started	Sep 12, 2022 8:41 PM
Ended	Sep 12, 2022 8:51 PM
Duration	10:14 💡
OS	Linux Debian - 11.3
Browser	Chrome 100

View run in Cypress Dashboard ➡️

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

lmiller1990

Some various nits/questions but no obvious problems, testing it now.

lmiller1990 · 2022-09-09T04:39:36Z

packages/data-context/src/data/ProjectConfigManager.ts

@@ -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) {


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 🤔

lmiller1990 · 2022-09-09T04:40:32Z

packages/data-context/src/data/ProjectLifecycleManager.ts

@@ -380,6 +380,8 @@ export class ProjectLifecycleManager {
  }

  private _setCurrentProject (projectRoot: string) {
+    process.chdir(projectRoot)


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.

packages/driver/src/cypress/script_utils.ts

lmiller1990 · 2022-09-09T04:43:27Z

packages/driver/src/cypress/source_map_utils.ts

@@ -1,6 +1,5 @@
 import _ from 'lodash'
 import { SourceMapConsumer } from 'source-map'
-import Promise from 'bluebird'


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

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

packages/errors/src/errors.ts

packages/graphql/src/schemaTypes/objectTypes/gql-Browser.ts

lmiller1990 · 2022-09-09T04:50:48Z

packages/launchpad/src/setup/OpenBrowserList.vue

@@ -27,7 +27,7 @@
          }"
        >
          <Tooltip
-            v-if="!browser.isVersionSupported"
+            v-if="browser.warning"


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

packages/server/lib/browsers/utils.ts

packages/server/lib/unhandled_exceptions.ts

lmiller1990 · 2022-09-09T05:01:25Z

packages/launchpad/src/setup/OpenBrowserList.vue

@@ -27,7 +27,7 @@
          }"
        >
          <Tooltip
-            v-if="!browser.isVersionSupported"
+            v-if="browser.warning"


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.

lmiller1990 · 2022-09-09T05:35:55Z

In run mode, I notice the stack trace is kinda ugly. I wonder if it's worth the effort to truncate the error message, or just do a console.error (probably not).

 Running:  Alert.cy.tsx                                                                   (1 of 42)
WebKit was launched, but the experimental feature was not enabled. Please add `experimentalWebKitSupport: true` to your config file to launch WebKit.
Error: WebKit was launched, but the experimental feature was not enabled. Please add `experimentalWebKitSupport: true` to your config file to launch WebKit.
    at Object.open (/home/lachlan/code/work/dev/packages/server/lib/browsers/webkit.ts:47:15)
    at Object.open (/home/lachlan/code/work/dev/packages/server/lib/browsers/index.ts:109:49)
    at OpenProject.relaunchBrowser (/home/lachlan/code/work/dev/packages/server/lib/open_project.ts:150:20)
From previous event:
    at wait (/home/lachlan/code/work/dev/packages/server/lib/modes/run.ts:387:35)
    at waitForBrowserToConnect (/home/lachlan/code/work/dev/packages/server/lib/modes/run.ts:410:12)
    at runSpec (/home/lachlan/code/work/dev/packages/server/lib/modes/run.ts:709:9)
    at runEachSpec (/home/lachlan/code/work/dev/packages/server/lib/modes/run.ts:581:29)
error Command failed with exit code 1.

Just thought I'd point this out, not obvious from the screenshots in your description.

It works great in the open mode UI - nice message appears when you don't enable the flag.

Linux error is a bit janky if you don't install the deps:

I wonder if we want a proper error for this (nice formatting?)

It works great, the actual browser window looks quite nice, too!

lmiller1990 · 2022-09-10T03:28:53Z

No code blocker for me, curious if we need to add a test for the regression you fixed w.r.t the CDP warning that wasn't displaying?

Can do separately if we are on a timeline, might be good to make an issue to track if that's the case.

lmiller1990

Code seems fine, last nice to have would be this comment: #23711 (comment) 🙏 thoughts?

flotwig · 2022-09-12T18:48:09Z

@lmiller1990 I fixed the error display:

I also noticed another bug... we aren't catching GQL errors in prod... so if you don't pass --browser --e2e to quicklaunch WK, you won't even see this error... but that's out of scope of WebKit.

I'll create tasks for the missing coverage once this merges.

lmiller1990 · 2022-09-13T00:21:19Z

Looks good, let's ship it.

The remaining CI fails are just Percy flake, so I think it's fine to squash merge?

flotwig added 4 commits September 7, 2022 16:24

chore(webkit): suppress ECONNRESET...

d29b5c6

feat: gate WebKit behind experimentalWebKitSupport in prod

9a9bf40

full build this branch

b8b5f1e

enable experiment in tests

ea32051

flotwig commented Sep 7, 2022

View reviewed changes

fix browser.disabled

804f7e7

flotwig mentioned this pull request Sep 7, 2022

Safari (WebKit) Support #6422

Closed

21 tasks

flotwig commented Sep 7, 2022

View reviewed changes

flotwig added 6 commits September 7, 2022 17:56

fix tests

7b473de

fix project chdir?

e8c0d9a

upd error snapshot

91357ed

Merge branch 'develop' into webkit-experimental

dc21b48

fix tests

1fde5a7

skip wasm source map consumption on windows

f396c41

flotwig marked this pull request as ready for review September 8, 2022 17:13

flotwig requested review from lmiller1990, tbiethman and emilyrohrbough September 8, 2022 17:13

flotwig added 4 commits September 8, 2022 13:53

fix unit tests

81925de

fix windows e2e tests

ca82602

add warnings to unsupported commands

156ae42

fix tests

95aa0b4

lmiller1990 reviewed Sep 9, 2022

View reviewed changes

lmiller1990 self-requested a review September 10, 2022 03:29

lmiller1990 approved these changes Sep 10, 2022

View reviewed changes

flotwig added 5 commits September 11, 2022 16:54

allow polling on server side

a9d2691

fix slowdown on project open when missing websocket transport

3bd8a3c

fix ts

77d3248

Merge branch 'develop' into webkit-experimental

7cc5584

fix stub

5e7fb4b

flotwig added 2 commits September 12, 2022 14:48

wrap wk errors

07a3cde

upd onlink

af8d3a1

tbiethman approved these changes Sep 12, 2022

View reviewed changes

flotwig merged commit 566a1a2 into develop Sep 13, 2022

flotwig deleted the webkit-experimental branch September 13, 2022 01:56

cypress-bot bot mentioned this pull request Sep 13, 2022

Show tooltips for "info" property of browsers #21769

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: gate WebKit behind experimentalWebKitSupport in prod #23711

feat: gate WebKit behind experimentalWebKitSupport in prod #23711

flotwig commented Sep 7, 2022 •

edited

cypress-bot bot commented Sep 7, 2022

flotwig Sep 7, 2022

flotwig Sep 7, 2022

lmiller1990 Sep 9, 2022

lmiller1990 Sep 9, 2022

flotwig Sep 9, 2022

flotwig Sep 7, 2022

flotwig Sep 7, 2022

flotwig Sep 7, 2022

cypress bot commented Sep 7, 2022 •

edited

lmiller1990 left a comment

lmiller1990 Sep 9, 2022

lmiller1990 Sep 9, 2022 •

edited

lmiller1990 Sep 9, 2022

flotwig Sep 9, 2022 •

edited

lmiller1990 Sep 9, 2022

lmiller1990 Sep 9, 2022

lmiller1990 commented Sep 9, 2022 •

edited

lmiller1990 commented Sep 10, 2022 •

edited

lmiller1990 left a comment

flotwig commented Sep 12, 2022

lmiller1990 commented Sep 13, 2022

feat: gate WebKit behind experimentalWebKitSupport in prod #23711

feat: gate WebKit behind experimentalWebKitSupport in prod #23711

Conversation

flotwig commented Sep 7, 2022 • edited

User facing changelog

Additional details

Steps to test

How has the user experience changed?

PR Tasks

cypress-bot bot commented Sep 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cypress bot commented Sep 7, 2022 • edited

Test summary

Run details

lmiller1990 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmiller1990 Sep 9, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flotwig Sep 9, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmiller1990 commented Sep 9, 2022 • edited

lmiller1990 commented Sep 10, 2022 • edited

lmiller1990 left a comment

Choose a reason for hiding this comment

flotwig commented Sep 12, 2022

lmiller1990 commented Sep 13, 2022

flotwig commented Sep 7, 2022 •

edited

cypress bot commented Sep 7, 2022 •

edited

lmiller1990 Sep 9, 2022 •

edited

flotwig Sep 9, 2022 •

edited

lmiller1990 commented Sep 9, 2022 •

edited

lmiller1990 commented Sep 10, 2022 •

edited