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: pass list of browsers to plugins file #5068

Merged
merged 84 commits into from Nov 19, 2019

Conversation

@bahmutov
Copy link
Collaborator

bahmutov commented Aug 30, 2019

Passes list of detected browsers to the plugins file and lets the user modify the list.

User facing changelog

  • Cypress now includes the full list of browsers found and their properties within the Cypress configuration. This means that the browsers array is also editable within plugins by accessing config.browsers. The currently available browsers is also displayed within the configuration panel under Settings in the Test Runner.

Additional details

Reworking browser detection and config to do the following:

  1. find all browsers
  2. set the browsers on the config object
  3. pass the config to the plugins if any, allowing the user to adjust the list and return the updated list
  4. if running in the interactive mode (cypress open)
  • show the updated list in the browser picker
  1. else pick the browser from the updated list.
  • by default, it is electron, which if the user removes from the list will cause an error
  • if the user passed --browser <name> CLI then pick that browser from the current updated list of browsers
  • if the user passed --browser <path> disregard the updated list and discover the browser just like we do now

How has the user experience changed?

Example use: https://github.com/cypress-io/cypress-example-electron/blob/master/cypress/plugins/index.js

Config Before

Screen Shot 2019-11-14 at 9 26 24 AM

Config After

2019-11-13_16-43-09

PR Tasks

  • Have tests been added/updated for the changes in this PR?
  • Has a PR to cypress-documentation been submitted to document any user-facing changes? cypress-io/cypress-documentation#2245
  • Has the original issue been tagged with a release in ZenHub?
  • update resolved configuration display, right now shows [object], ... for the list of browsers
  • add a test to make sure user cannot list browsers in cypress.json but in the plugins only
@bahmutov

This comment has been minimized.

Copy link
Collaborator Author

bahmutov commented Aug 30, 2019

Testing it locally with command

DEBUG=cypress:launcher,cypress:server:plugins:child npm run dev -- --project ~/git/cypress-example-electron/

Which shows the browser detection happens first, and then the plugins file loading, so we do have this information

  cypress:launcher checking one browser canary +0ms
  cypress:launcher looking up canary on darwin platform +0ms
  cypress:launcher looking for app Contents/MacOS/Google Chrome Canary id com.google.Chrome.canary +0ms
  cypress:launcher looking for bundle id com.google.Chrome.canary using command: mdfind 'kMDItemCFBundleIdentifier=="com.google.Chrome.canary"' | head -1 +0ms
  cypress:launcher found com.google.Chrome.canary at /Applications/Google Chrome Canary.app +49ms
  cypress:launcher reading property file "/Applications/Google Chrome Canary.app/Contents/Info.plist" +1ms
  cypress:launcher setting major version for {"name":"canary","family":"chrome","displayName":"Canary","version":"78.0.3896.6","path":"/Applications/Google Chrome Canary.app/Contents/MacOS/Google Chrome Canary"} +17ms
  cypress:launcher browser canary version 78.0.3896.6 major version 78 +0ms
  cypress:server:plugins:child pluginsFile: /Users/gleb/git/cypress-example-electron/cypress/plugins/index.js +0ms
  cypress:server:plugins:child require pluginsFile +2ms
/Users/gleb/git/cypress-example-electron/cypress/plugins/index.js
[ '/Users/gleb/git/cypress/packages/electron/dist/Cypress/Cypress.app/Contents/Frameworks/Cypress Helper.app/Contents/MacOS/Cypress Helper',
  '/Users/gleb/git/cypress/packages/server/lib/plugins/child/index.js',
  '--file',
  '/Users/gleb/git/cypress-example-electron/cypress/plugins/index.js' ]
  cypress:server:plugins:child plugins load file "/Users/gleb/git/cypress-example-electron/cypress/plugins/index.js" +14ms
  cypress:server:plugins:child run plugins function +1ms
  cypress:server:plugins:child register event _get:task:body with id 0 +0ms
  cypress:server:plugins:child register event _get:task:keys with id 1 +0ms
argument config
{ animationDistanceThreshold: 5,
@cypress

This comment has been minimized.

Copy link

cypress bot commented Aug 30, 2019



Test summary

3511 0 45 0


Run details

Project cypress
Status Passed
Commit ecb3a03
Started Nov 19, 2019 1:54 PM
Ended Nov 19, 2019 1:58 PM
Duration 03:51 💡
OS Linux Debian - 9.8
Browser Multiple

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

@bahmutov bahmutov requested a review from brian-mann Aug 30, 2019
@bahmutov bahmutov changed the title WIP: pass list of browsers to plugins file Pass list of browsers to plugins file Sep 3, 2019
@bahmutov bahmutov mentioned this pull request Sep 4, 2019
2 of 4 tasks complete
@jennifer-shehane jennifer-shehane requested a review from flotwig Sep 9, 2019
packages/server/lib/config.coffee Show resolved Hide resolved
packages/server/lib/config.coffee Outdated Show resolved Hide resolved
packages/server/lib/config.coffee Outdated Show resolved Hide resolved
bahmutov added 2 commits Sep 10, 2019
Copy link
Member

jennifer-shehane left a comment

In terms of the API changes, @brian-mann mentioned that we should have a method call like setBrowser or setBrowsers so that we could actually do some validation on what is passed in - instead of the static overriding of the object. A user could mutate it and just completely mess up the launching of Cypress browser and we want to try and avoid that situation if possible.

@bahmutov

This comment has been minimized.

Copy link
Collaborator Author

bahmutov commented Oct 18, 2019

I think simplicity right now wins - if you mutate the other configuration values, you can screw things up, but how often users would screw the list of browsers and leave the invalid configuration? If they send an invalid browser, we could validate - just like we should validate the entire modified config object after getting it back.

@bahmutov bahmutov changed the title Pass list of browsers to plugins file WIP: Pass list of browsers to plugins file Oct 18, 2019
@bahmutov bahmutov mentioned this pull request Oct 18, 2019
4 of 6 tasks complete
@bahmutov

This comment has been minimized.

Copy link
Collaborator Author

bahmutov commented Oct 19, 2019

This PR also enables to run tests in Microsft Edge Beta (Chromium-based) or Brave browser by simply returning them from the plugins file

module.exports = (on, config) => {
  // `on` is used to hook into various events Cypress emits
  // `config` is the resolved Cypress config

  const pathToBrave =
    '/Applications/Brave Browser.app/Contents/MacOS/Brave Browser'
  config.browsers.push({
    name: 'brave',
    family: 'chrome',
    displayName: 'Brave',
    version: 'unknown',
    path: pathToBrave,
    majorVersion: 'unknown'
  })

  return config
}

Screen Shot 2019-10-19 at 8 09 23 AM

@jennifer-shehane jennifer-shehane self-requested a review Oct 21, 2019
bahmutov added 5 commits Oct 22, 2019
bahmutov added 3 commits Oct 25, 2019
…ress-io/cypress into pass-list-of-browsers-to-config-5067
packages/server/lib/browsers/index.coffee Outdated Show resolved Hide resolved
.then (browsers = []) ->
debug("searching for browser '%s' among %s",
nameOrPath, pluralize("browser", browsers.length, true))

This comment has been minimized.

Copy link
@flotwig

flotwig Nov 15, 2019

Member

this seems unnecessary, debug logs should be consistent and not change wording based on what's being logged

packages/server/lib/browsers/index.coffee Outdated Show resolved Hide resolved
packages/server/lib/browsers/utils.coffee Outdated Show resolved Hide resolved
…5067' into pass-list-of-browsers-to-config-5067

# Conflicts:
#	packages/desktop-gui/src/settings/settings.scss
Copy link
Member

brian-mann left a comment

@bahmutov

  • add an e2e test that mutates config.browsers with an object - so that the error is handled and is snapshotted
  • ensure that we are validating invalid config after its returned from the pluginsFile
    • added e2e tests

Screen Shot 2019-11-15 at 3 49 54 PM

@andrew-codes

  • fix nested objects such as browsers jumping when you click on the arrow and expands them
  • add in the missing : colon when nested objects are expanded
  • show Array(len) when arrays are expanded in the preview area of the array
  • think about maybe adding a "Copy" button that copies the derived final configuration object (non blocking, low priority)
  • reduce the line height to match dev tools
andrew-codes and others added 18 commits Nov 15, 2019
- adjust spacing of items
- distinguish between arrays and objects (via expanded arrays having the text `"Array ({length})"`  after the colon
Co-Authored-By: Zach Bloomquist <github@chary.us>
Co-Authored-By: Zach Bloomquist <github@chary.us>
Co-Authored-By: Zach Bloomquist <github@chary.us>
…ress-io/cypress into pass-list-of-browsers-to-config-5067
@bahmutov bahmutov requested a review from brian-mann Nov 18, 2019
@bahmutov bahmutov changed the title Pass list of browsers to plugins file feat: pass list of browsers to plugins file Nov 19, 2019
@bahmutov bahmutov merged commit b03b25c into develop Nov 19, 2019
38 checks passed
38 checks passed
WIP Ready for review
Details
ci/circleci: Linux lint Your tests passed on CircleCI!
Details
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: build-binary Your tests passed on CircleCI!
Details
ci/circleci: build-npm-package Your tests passed on CircleCI!
Details
ci/circleci: desktop-gui-integration-tests-2x Your tests passed on CircleCI!
Details
ci/circleci: driver-integration-tests-chrome Your tests passed on CircleCI!
Details
ci/circleci: lint-types Your tests passed on CircleCI!
Details
ci/circleci: reporter-integration-tests Your tests passed on CircleCI!
Details
ci/circleci: run-launcher Your tests passed on CircleCI!
Details
ci/circleci: server-e2e-tests-chrome-1 Your tests passed on CircleCI!
Details
ci/circleci: server-e2e-tests-chrome-2 Your tests passed on CircleCI!
Details
ci/circleci: server-e2e-tests-chrome-3 Your tests passed on CircleCI!
Details
ci/circleci: server-e2e-tests-chrome-4 Your tests passed on CircleCI!
Details
ci/circleci: server-e2e-tests-chrome-5 Your tests passed on CircleCI!
Details
ci/circleci: server-e2e-tests-chrome-6 Your tests passed on CircleCI!
Details
ci/circleci: server-e2e-tests-chrome-7 Your tests passed on CircleCI!
Details
ci/circleci: server-e2e-tests-chrome-8 Your tests passed on CircleCI!
Details
ci/circleci: server-e2e-tests-electron-1 Your tests passed on CircleCI!
Details
ci/circleci: server-e2e-tests-electron-2 Your tests passed on CircleCI!
Details
ci/circleci: server-e2e-tests-electron-3 Your tests passed on CircleCI!
Details
ci/circleci: server-e2e-tests-electron-4 Your tests passed on CircleCI!
Details
ci/circleci: server-e2e-tests-electron-5 Your tests passed on CircleCI!
Details
ci/circleci: server-e2e-tests-electron-6 Your tests passed on CircleCI!
Details
ci/circleci: server-e2e-tests-electron-7 Your tests passed on CircleCI!
Details
ci/circleci: server-e2e-tests-electron-8 Your tests passed on CircleCI!
Details
ci/circleci: server-integration-tests Your tests passed on CircleCI!
Details
ci/circleci: server-performance-tests Your tests passed on CircleCI!
Details
ci/circleci: server-unit-tests Your tests passed on CircleCI!
Details
ci/circleci: test binary as a non-root user Your tests passed on CircleCI!
Details
ci/circleci: test binary as a root user Your tests passed on CircleCI!
Details
ci/circleci: test-kitchensink Your tests passed on CircleCI!
Details
ci/circleci: unit-tests Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
cypress: 2x-desktop-gui 395 tests passed in 01:58
Details
cypress: 5x-driver-chrome 3086 tests passed in 03:45
Details
cypress: reporter 30 tests passed in 00:35
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants
You can’t perform that action at this time.