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

moved #27911

Closed
wants to merge 10 commits into from
Closed

moved #27911

wants to merge 10 commits into from

Conversation

alexsch01
Copy link
Contributor

@alexsch01 alexsch01 commented Sep 26, 2023

moved to #30517 since I can't reopen this pull request

@cypress-app-bot
Copy link
Collaborator

This was referenced Sep 26, 2023
@jennifer-shehane jennifer-shehane requested review from dkasper-was-taken and removed request for dkasper-was-taken September 28, 2023 20:46
Copy link
Member

@jennifer-shehane jennifer-shehane left a comment

Choose a reason for hiding this comment

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

Outlining the requirements from product

Internal initiative link.

In Scope

  • The config option should be named browser since it is the same behavior as the --browser flag
  • The config option when used during cypress run should run the browser
  • The config option when used during cypress open should pre-select browser - (behave the same as --browser flag)
  • The –browser flag overrides any value of configuration browser via cypress run or cypress open
  • The browser config should available on top level config and also via the e2e and ct testing types to specify a different browser for each testing type
  • If CYPRESS_CONFIG={browser: chrome} –browser firefox passed to terminal, –browser overrides browser config
  • The browser config should accept args just like the --browser flag, including channels (chromiu, chrome:beta, etc)
  • Should error when an invalid or not found browser is passed to browser config, just like –browser flag
  • Should be able to write config.browser within the setupNodeEvents since browser is now on main config

Not in scope

  • Allowing to specify a headed or headless option for the browser is not in scope
  • The browser config will not be settable on spec/suite level config
  • The browser config will not be available via Module API (could always pass browser flag at this point)
  • Do not need to update the GitHub action

@alexsch01 As this PR stands, we would not accept these changes because they don't match the requirements that we'd want in the product for this feature.

@alexsch01
Copy link
Contributor Author

alexsch01 commented Sep 29, 2023

@jennifer-shehane

There's an issue, variable browser (type: Cypress.Browser) is used in Cypress.RuntimeServerConfigOptions

If we have a new variable browser (type: string) in Cypress.ResolvedConfigOptions, then browser in Cypress.RuntimeServerConfigOptions & Cypress.ResolvedConfigOptions would have a type of Cypress.Browser & string which doesn't work

image
image

Also config.defaultBrowser makes a lot of sense, considering a person could get confused that config.browser is the same as the browser that will get launched - since it's not in cases where --browser chrome is passed to the CLI


Please look this over with the product team

@dkasper-was-taken
Copy link
Contributor

@alexsch01 I kicked off the build, but I also pulled develop into the branch, there have been some build/cli changes. Apologies if there's any inconvenience.

@alexsch01
Copy link
Contributor Author

I really believe this should be merged, but unforunately it won't be accepted

@alexsch01 alexsch01 closed this Oct 17, 2023
@alexsch01 alexsch01 changed the title feat: ability to specify the default browser in cypress config file moved Nov 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants