moved to #27911

[alexsch01]

moved to #27911

[cypress-app-bot]

• 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.

[mike-plummer]

Hi @alexsch01 , thanks for the contribution!

I believe there is already a documented pattern for situations where you have a test suite designed to only run against Chrome instead of Electron - does this recipe meet your needs? Is there a use case that this new param would solve not already addressable by that pattern?

[alexsch01]

@mike-plummer

that's only for "cypress open", my PR is used for both "cypress open" and "cypress run

---

This pull request is ready for review now

Please ask any questions that you have with this PR

[mike-plummer]

Thanks for the contribution @alexsch01

This feels more like a defaultBrowser flag to me since it only applies if the user doesn't supply a --browser flag themselves.

A better location for this logic may be packages/data-context/src/data/ProjectLifecycleManager.ts#setInitialActiveBrowser. The approach you're using here while effective would also end up setting config flags like cliBrowser which tell other places in Cypress that the user specified an override of the browser via the CLI which isn't really the case. The code I referenced already has certain fallback cases to determine the most appropriate browser to launch based on things like:

• Is a CLI flag passed?
• If not, what was the last browser the project used?
• If that's not available, is there a good default we can choose?

In addition, there will need to be some tests written for this, both in the data-context package (assuming that ends up being a good place for this live) as well as system tests to validate that Cypress actually launches as expected with that new flag. The sometimes annoying part of developing a testing tool is that the tests end up being way bigger than the feature 😆

Finally, this will need corresponding updates in cypress-documentation, but that can wait until an implementation settles

[alexsch01]

Changed browser to defaultBrowser and I agree that defaultBrowser is the more appropriate name since it only takes effect if there is no "--browser" passed by the user in the CLI

Also, added the defaultBrowser type to the cypress.d.ts file

[alexsch01]

Documentation is done

I have manually tested that this change does work successfully

[alexsch01]

@mike-plummer

Currently the way I'm reading the config object is converting the cypress config file as an object which allows me to get the defaultBrowser attribute

Is there a way of reading the config object more directly?

[mike-plummer]

@alexsch01 I left comments yesterday around the fact that this logic may have unintended side-effects and would likely belong elsewhere. Have you reviewed and considered that suggestion?

We ask that contributors at least attempt to write tests for their PRs - we're happy to answer questions or assist if you get stuck

[alexsch01]

@mike-plummer I read your suggestion......I'm still unsure how to read the cypress config object

[mike-plummer]

@mike-plummer I read your suggestion......I'm still unsure how to read the cypress config object

From within data-context you can retrieve the effective project configuration via this.ctx.project.getConfig()

[alexsch01]

I put a final commit before I look into the entirely different method

[alexsch01]

@mike-plummer

The reason I put the changes under node_modules\cypress\lib\cli.js was so that I can test the changes with my cypress project without compiling Cypress from source

What unintended side-effects do you see from this code change?

[mike-plummer]

@alexsch01 From my earlier review

The approach you're using here while effective would also end up setting config flags like cliBrowser which tell other places in Cypress that the user specified an override of the browser via the CLI which isn't really the case.

There are instructions in the CONTRIBUTING guide for how to run cypress locally - are you having trouble getting the app running in development mode?

[mike-plummer]

@jennifer-shehane @brian-mann Would appreciate any thoughts y'all have on this. I can see the potential value to change the default browser from "electron", but existing functionality to control this already exists via the CLI --browser flag if you set up dedicated Node script aliases

[alexsch01]

@lmiller1990 please allow the build to run

[lmiller1990]

@alexsch01 I'm not working at Cypress now, I can't trigger the build. Maybe @jennifer-shehane can do that for you.

[jennifer-shehane]

@alexsch01 Pulled in develop

[alexsch01]

@jennifer-shehane please assign this to someone

[jennifer-shehane]

Hi @alexsch01, we don't have anyone available currently to spend the time to review this change.

[alexsch01]

moved to #27911