fix: Detect user-configured browsers #23446

ryanjduffy · 2022-08-18T23:02:47Z

Closes

User facing changelog

Adding a custom browser configuration will no longer emit a warning when trying to use that browser

Additional details

If you specify a custom browser (either directly in setupNodeEvents or via a plugin) and try to use that browser via --browser, Cypress logs a warning

I expect this isn't the correct fix for this problem hence the draft status but hoping to get some feedback. The issue seems to be that the logic within makeDataContext only resolves the machine browsers based on the known browsers and ignores any user-configured browsers. Perhaps I'm just configuring things wrong and there's another way to do it.

Steps to test

Add a new browser like the Brave example on https://docs.cypress.io/guides/guides/launching-browsers#Customize-available-browsers
Launch npx cypress run --browser Brave (or whatever you've called the custom browser*

Expected:

Tests are run

Actual:

Test are run but a warning is issued beforehand

How has the user experience changed?

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?

cypress-bot · 2022-08-18T23:02:50Z

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.

ryanjduffy

I think this is ready-ish. Left a couple discussion points below. Could also use some advice on adding tests for this.

ryanjduffy · 2022-08-29T22:37:19Z

packages/server/lib/makeDataContext.ts

@@ -46,9 +47,20 @@ export function makeDataContext (options: MakeDataContextOptions): DataContext {
      close: browsers.close,
      getBrowsers,
      async ensureAndGetByNameOrPath (nameOrPath: string) {
+        const projectConfig: FullConfig = await (ctx as DataContext).project.getConfig()


This type assertion seems like it should be unnecessary (and isn't used below) but I couldn't avoid the any without it. Improvements welcome!

Hi 👋

You can probably do const projectConfig = await getCtx().project.getConfig() to get the correct type inference (which will return a reference to ctx).

ryanjduffy · 2022-08-29T22:48:03Z

packages/server/lib/makeDataContext.ts

@@ -46,9 +47,20 @@ export function makeDataContext (options: MakeDataContextOptions): DataContext {
      close: browsers.close,
      getBrowsers,
      async ensureAndGetByNameOrPath (nameOrPath: string) {
+        const projectConfig: FullConfig = await (ctx as DataContext).project.getConfig()
+        const declaredBrowsers = projectConfig.browsers?.map<FoundBrowser | undefined>((b) => {
+          if (!b.path) return


It seems that only browsers injected by the user or plugins have a path set so I'm using it as the proxy for custom browsers. Seem reasonable?

@ryanjduffy Thank you for opening a change to update improve this check! Considering where this is used, it;'d likely be cheaper and cleaner from a start-up perspective to update ensureAndGetByNameOrPath to be a pass through to the imported ensureAndGetByNameOrPath

getBrowsers, - async ensureAndGetByNameOrPath (nameOrPath: string) { - ... - }, + ensureAndGetByNameOrPath

and instead update where this is called in the data-context package to supply the list of browsers.
Jumping through the flow:

we set the active browser after we finish loading the config

this will then call ensureAndGetByNameOrPath if a browser was provided via the CLI. Seems like we should be able to pull up the machine browsers check and pass it in here.

@emilyrohrbough Appreciate the feedback! It's helped me track how this data is used. I've taken a slightly different approach by consolidating both browser lookups (machine and user) into BrowserDataSource and then exposed an allBrowsers method to fetch both. I like that this approach keeps the "concerns" of the browser config in one spot and makes the diff more clear.

The added bonus of this was that it allowed me to fix cypress open to remember the last browser when it was a user browser since that logic can now rely on allBrowsers.

lmiller1990 · 2022-09-08T04:31:32Z

packages/server/lib/makeDataContext.ts

@@ -46,9 +47,20 @@ export function makeDataContext (options: MakeDataContextOptions): DataContext {
      close: browsers.close,
      getBrowsers,
      async ensureAndGetByNameOrPath (nameOrPath: string) {
+        const projectConfig: FullConfig = await (ctx as DataContext).project.getConfig()


Hi 👋

You can probably do const projectConfig = await getCtx().project.getConfig() to get the correct type inference (which will return a reference to ctx).

lmiller1990 · 2022-09-08T04:32:39Z

packages/server/lib/makeDataContext.ts

        const browsers = await ctx.browser.machineBrowsers()

-        return await ensureAndGetByNameOrPath(nameOrPath, false, browsers)
+        return await ensureAndGetByNameOrPath(nameOrPath, false, [...declaredBrowsers, ...browsers])


I wonder if we want to do a check to make sure there isn't any duplicates here - it shouldn't really happen, but if someone adds a default browser via cypress.config.ts for whatever reason, it'd show up twice.

What would you de-dupe on? family / major version / minor version / path?

Just ran across a removeDuplicates method elsewhere which uses name and version

const removeDuplicates = (val) => { return _.uniqBy(val, (browser: FoundBrowser) => { return `${browser.name}-${browser.version}` }) }

If this is the pattern, maybe we just go with that. I wonder if we can make that a reused utility? Thoughts?

I've consolidated this into BrowserDataSource and exported it from there.

ryanjduffy · 2022-09-09T17:15:55Z

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

@@ -296,7 +296,7 @@ export class ProjectLifecycleManager {

    // lastBrowser is cached per-project.
    const prefs = await this.ctx.project.getProjectPreferences(path.basename(this.projectRoot))
-    const browsers = await this.ctx.browser.machineBrowsers()
+    const browsers = await this.ctx.browser.allBrowsers()


This also fixes restoring the last browser if it's added via the config

emilyrohrbough · 2022-09-09T18:18:56Z

packages/data-context/src/sources/BrowserDataSource.ts

+      const p = this.ctx.project.getConfig()
+
+      this.ctx.coreData.userBrowsers = p.then(async (cfg) => {
+        return cfg.browsers?.map<FoundBrowser | undefined>((b) => {


at this point, won't the cfg.browsers include the found machine browsers as well as the user-defined browsers?

It does! I didn't catch that. What is the authoritative source for "browsers from the config file" (is this a thing?) vs "browsers added via a plugin" vs "browsers on the machine" vs "resolved browsers from the full config" ?

I'll push a commit that tries to get "1" and "2" via "4" - "3" but it seems suspect.

@emilyrohrbough - following up to see if you have feedback on the updates I pushed. Thanks!!

lmiller1990

Looks good, just some nits. Also we don't appear to have any coverage around this - I'm still thinking about how best to add that. It might be quicker if someone internally picks that up, just so we can get this over the line.

Curious about the catch you've added - can you think of a specific example we might need to test for that where the logic you've added might error?

lmiller1990 · 2022-09-16T06:43:41Z

packages/data-context/src/sources/BrowserDataSource.ts

+}
+
+export function removeDuplicateBrowsers (browsers: FoundBrowser[]) {
+  return _.uniqBy(browsers, getBrowserKey)


Oh neat, I forgot about uniqBy!

lmiller1990 · 2022-09-16T06:45:12Z

packages/data-context/src/sources/BrowserDataSource.ts

+  /**
+   * Gets the browsers from the machine and the project config
+   */
+  allBrowsers () {


I think we can move this to use the async/await style - I know you are just matching the code style, but in general I think we are moving away from big nested then chains in favor of the more modern async/await. Thoughts?

Fine with me. As you guessed, I was just matching style

lmiller1990 · 2022-09-16T06:46:56Z

packages/data-context/src/sources/BrowserDataSource.ts

+      this.ctx.coreData.allBrowsers = Promise.all([p, machineBrowsers]).then(async ([cfg, m]) => {
+        if (!cfg.browsers) return m
+
+        const userBrowsers = cfg.browsers.map<FoundBrowser | undefined>((b) => {


Whenever I see a map then filter combo I like to opt for reduce to decrease the number of iterations. Unless you think that would be more difficult to read?

Not a strong opinion. Some folks find reduce to be less readable. If there's a preference for reduce, I can make that change.

lmiller1990 · 2022-09-16T06:48:13Z

packages/data-context/src/sources/BrowserDataSource.ts

+      }).catch((e) => {
+        this.ctx.update((coreData) => {
+          coreData.allBrowsers = null
+          coreData.diagnostics.error = e


Nice job figuring out where/how to propagate this error, the data-context is pretty confusing. Did you actually verify this error manifests in the UI/CLI?

I'm also not entirely clear on the condition that would lead to this - is there one, or did you just put it here for good measure?

Gonna need a quick test for both cases, I don't think we have much coverage around detecting the browsers in general, I could pick this up.

I don't think the new logic will likely error. machineBrowsers() re-throws so that'll be caught here but really you're just getting two copies of the same error then. Lemme experiment a bit to see if I can break it. ;)

Looks like the config validation logic would catch the cases in which this code would fail so I can remove the error handling.

ryanjduffy · 2022-09-19T18:37:38Z

@lmiller1990 - Pushed a commit to address you feedback. Thanks!

lmiller1990

Looks good, I wrote some tests 41c296d then made a small refactor (code style - I really prefer return early than nesting lots of logic in an if statement, fairly confident everything still works since I wrote the tests before the refactor 👍 )

Not related to this PR but I think we should not be mutating coreData in *DataSource, but in *Actions (so in BrowserActions.ts). I think changing this is a big outside the scope of this, so going to approve this for now - code does what it's supposed to.

lmiller1990 · 2022-09-20T07:40:55Z

@emilyrohrbough WDYT? Any other comments?

lmiller1990 · 2022-09-21T08:00:02Z

Oops, CI is failing - lemme take a look.

9ce44b4 should fix it.

lmiller1990 · 2022-09-26T00:48:36Z

Just need one more ✔️

ryanjduffy · 2022-09-26T18:05:48Z

ZachJW34

Works great! Let's get CI to run and then get this merged

lmiller1990 · 2022-09-27T00:18:03Z

I will get it merged

lmiller1990 · 2022-09-27T01:33:20Z

Hmm I think the remaining failures are not unique to this branch. Let me take a look.

lmiller1990 · 2022-09-27T01:44:35Z

I pushed a commit make the timeout for the npm/schematic test longer.

lmiller1990 · 2022-09-27T02:15:58Z

This is passing on CI https://app.circleci.com/pipelines/github/cypress-io/cypress/44027/workflows/3c5e7436-590d-4360-afcb-114d18b56c53

I don't know why the status did not update. I'm going to perform an admin merge, since I've visually inspected this is all passing on CI, and I've also run the tests locally that are related to this code change.

Merged... took a while, but we did it 👍

cypress-bot · 2022-09-27T16:55:16Z

Released in 10.9.0.

This comment thread has been locked. If you are still experiencing this issue after upgrading to
Cypress v10.9.0, please open a new issue.

ryanjduffy marked this pull request as ready for review August 29, 2022 22:35

ryanjduffy commented Aug 29, 2022

View reviewed changes

nagash77 requested a review from lmiller1990 September 7, 2022 18:16

lmiller1990 reviewed Sep 8, 2022

View reviewed changes

fix: Detect user-configured browsers

074fcfd

ryanjduffy force-pushed the bug-detect-configured-browsers branch from 670fead to 074fcfd Compare September 8, 2022 20:23

ryanjduffy added 3 commits September 9, 2022 07:57

move user browser lookup into BrowserDataSource

6d5c4da

Merge branch 'develop' into bug-detect-configured-browsers

afbf854

refactor out common browser dedupe logic

a04c61c

ryanjduffy commented Sep 9, 2022

View reviewed changes

simplify allBrowsers

00d0d9b

emilyrohrbough reviewed Sep 9, 2022

View reviewed changes

ryanjduffy and others added 2 commits September 9, 2022 12:14

resolve non-machine browsers

de8bcee

Merge branch 'develop' into bug-detect-configured-browsers

5d90c5b

lmiller1990 reviewed Sep 16, 2022

View reviewed changes

ryanjduffy added 2 commits September 19, 2022 09:48

Merge branch 'develop' into bug-detect-configured-browsers

c10ef54

pr feedback

26c87e6

added tests

41c296d

lmiller1990 self-requested a review September 20, 2022 07:36

lmiller1990 approved these changes Sep 20, 2022

View reviewed changes

lmiller1990 and others added 4 commits September 21, 2022 18:03

fix test

9ce44b4

Merge branch 'develop' into bug-detect-configured-browsers

68704a0

Merge branch 'develop' into bug-detect-configured-browsers

996a97f

Merge branch 'develop' into bug-detect-configured-browsers

d72e6cf

ZachJW34 approved these changes Sep 26, 2022

View reviewed changes

Merge branch 'develop' into bug-detect-configured-browsers

7b6a15a

longer timeout

894b333

lmiller1990 merged commit a75d3ec into cypress-io:develop Sep 27, 2022

cypress-bot bot locked as resolved and limited conversation to collaborators Sep 27, 2022

ryanjduffy deleted the bug-detect-configured-browsers branch September 27, 2022 19:51

fix: Detect user-configured browsers #23446

fix: Detect user-configured browsers #23446

Conversation

ryanjduffy commented Aug 18, 2022

User facing changelog

Additional details

Steps to test

How has the user experience changed?

PR Tasks

cypress-bot bot commented Aug 18, 2022

ryanjduffy left a 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

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

ryanjduffy Sep 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmiller1990 left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanjduffy commented Sep 19, 2022

lmiller1990 left a comment

Choose a reason for hiding this comment

lmiller1990 commented Sep 20, 2022

lmiller1990 commented Sep 21, 2022 • edited Loading

lmiller1990 commented Sep 26, 2022

ryanjduffy commented Sep 26, 2022

ZachJW34 left a comment

Choose a reason for hiding this comment

lmiller1990 commented Sep 27, 2022

lmiller1990 commented Sep 27, 2022

lmiller1990 commented Sep 27, 2022

lmiller1990 commented Sep 27, 2022 • edited Loading

cypress-bot bot commented Sep 27, 2022

ryanjduffy Sep 9, 2022 •

edited

Loading

lmiller1990 commented Sep 21, 2022 •

edited

Loading

lmiller1990 commented Sep 27, 2022 •

edited

Loading