Skip to content

Comments

use Node path from config file to run plugins#4436

Merged
jennifer-shehane merged 61 commits intodevelopfrom
external-node-for-plugins-4432
Oct 1, 2019
Merged

use Node path from config file to run plugins#4436
jennifer-shehane merged 61 commits intodevelopfrom
external-node-for-plugins-4432

Conversation

@bahmutov
Copy link
Contributor

@bahmutov bahmutov commented Jun 11, 2019

Description of change

Added a config option nodeVersion that will be used to determine which version of Node Cypress is run within. Users can use the bundled version that comes with Cypress (which is the default) or override it with the system Node version installed on their machine.

How to use the feature

In configuration file (cypress.json by default), to use the bundled version of Node with Cypress. default or bundled = use the Node that comes with Cypress Test Runner

{
  "nodeVersion": "bundled"
}

In configuration file (cypress.json by default), to use the system version of Node with Cypress. system = tries to find system Node using cross-platform https://github.com/npm/node-which from NPM. If fails to find, tries again but first fixing path using https://github.com/sindresorhus/fix-path

{
  "nodeVersion": "system"
}

How the design has changed

  • add frontend view in "Settings" to indicate path to node + node version in use
  • update cypress run mode to indicate the system version of node (likely in the table header at the top of a run shown below)
  ┌────────────────────────────────────────────────────────────────────────────────────────────────┐
  │ Cypress:        1.2.3                                                                          │
  │ Browser:        FooBrowser 88                                                                  │
  │ Node Version:   v0.0.0 (bundled with Cypress)                                                  │
  │ Specs:          1 found (base_url_spec.coffee)                                                 │
  │ Searched:       cypress/integration/base_url_spec.coffee                                       │
  └────────────────────────────────────────────────────────────────────────────────────────────────┘

To demo / test this, I made a branch external-node-for-plugins-4432 in https://github.com/cypress-io/cypress-test-tiny/tree/external-node-for-plugins-4432 that runs cy.task that prints Node version. To execute from the repo root run (assuming the test project is in ~/git/cypress-test-tiny/). Example output from the terminal running Node 12

$ npm run dev -- --run-project ~/git/cypress-test-tiny/

> cypress@3.4.0 dev /Users/gleb/git/cypress
> node ./scripts/start.js "--run-project" "/Users/gleb/git/cypress-test-tiny/"

plugins file Node version v12.4.0
plugins file on, Node version v12.4.0

====================================================================================================

  (Run Starting)

  ┌────────────────────────────────────────────────────────────────────────────────────────────────┐
  │ Cypress:    3.4.0                                                                              │
  │ Browser:    Electron 61 (headless)                                                             │
  │ Specs:      1 found (spec.js)                                                                  │
  └────────────────────────────────────────────────────────────────────────────────────────────────┘


────────────────────────────────────────────────────────────────────────────────────────────────────
                                                                                                    
  Running: spec.js...                                                                      (1 of 1) 
file:preprocessor file /Users/gleb/git/cypress-test-tiny/cypress/integration/spec.js Node version v12.4.0
file:preprocessor file /Users/gleb/git/cypress-test-tiny/cypress/support/index.js Node version v12.4.0

New Node Version panel in Desktop GUI

Using node version bundled with Cypress

Screen Shot 2019-10-01 at 1 44 47 PM

Using node version from your system

Screen Shot 2019-10-01 at 1 46 11 PM

Pre-merge Tasks

@bahmutov
Copy link
Contributor Author

hmm, maybe it is better to find Node using https://github.com/npm/node-which

@brian-mann
Copy link
Member

@bahmutov you should use this module: https://github.com/sindresorhus/run-node

If Cypress is run in global "double click" mode then $PATH will not be available. Sindre's package will effectively always find the correct node version correctly - and it's used by things like husky git hooks which work in git gui apps.

@bahmutov
Copy link
Contributor Author

bahmutov commented Jun 12, 2019 via email

@bahmutov
Copy link
Contributor Author

plus run-node is just a bash shell script to run Node - we need to find the path to the node, so at most we can reuse some logic from that shell script. I think I can lift this logic from execa though - load user's $PATH before using node-which is is cross platform

@bahmutov
Copy link
Contributor Author

bahmutov commented Jun 12, 2019

@bahmutov
Copy link
Contributor Author

bahmutov commented Jun 12, 2019

Example (after building and opening Electron binary on Mac)

local mode

$ nvm use 8
Now using node v8.9.4 (npm v6.9.0)
~/git/cypress on external-node-for-plugins-4432*
$ DEBUG=cypress:server:plugins ./build/darwin/Cypress.app/Contents/MacOS/Cypress 
  cypress:server:plugins plugins.init /Users/gleb/git/cypress-test-tiny/cypress/plugins/index.js +0ms
  cypress:server:plugins finding Node with $PATH /Library/Frameworks/Python.framework/Versions/2.7/bin:/opt/local/bin:/opt/local/sbin:/Users/gleb/.nvm/versions/node/v8.9.4/bin:/opt/local/bin:/opt/local/sbin:/opt/local/bin:/opt/local/sbin:/usr/local/bin:/Users/gleb/dev/arcanist/bin:/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin:/opt/X11/bin:/Applications/Postgres.app/Contents/Versions/latest/bin +0ms
  cypress:server:plugins found Node /Users/gleb/.nvm/versions/node/v8.9.4/bin/node +0ms
  cypress:server:plugins using Node /Users/gleb/.nvm/versions/node/v8.9.4/bin/node +1ms
plugins file Node version v8.9.4
plugins file on, Node version v8.9.4

Then use Node 6

$ nvm use 6
Now using node v6.12.2 (npm v3.10.10)
~/git/cypress on external-node-for-plugins-4432*
$ DEBUG=cypress:server:plugins ./build/darwin/Cypress.app/Contents/MacOS/Cypress 
  cypress:server:plugins plugins.init /Users/gleb/git/cypress-test-tiny/cypress/plugins/index.js +0ms
  cypress:server:plugins finding Node with $PATH /Library/Frameworks/Python.framework/Versions/2.7/bin:/opt/local/bin:/opt/local/sbin:/Users/gleb/.nvm/versions/node/v6.12.2/bin:/opt/local/bin:/opt/local/sbin:/opt/local/bin:/opt/local/sbin:/usr/local/bin:/Users/gleb/dev/arcanist/bin:/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin:/opt/X11/bin:/Applications/Postgres.app/Contents/Versions/latest/bin +0ms
  cypress:server:plugins found Node /Users/gleb/.nvm/versions/node/v6.12.2/bin/node +1ms
  cypress:server:plugins using Node /Users/gleb/.nvm/versions/node/v6.12.2/bin/node +2ms
plugins file Node version v6.12.2
plugins file on, Node version v6.12.2

global mode

first open Finder

open build/darwin/Cypress.app/Contents/MacOS/

click on Mac app and start it in "global" mode. Running the project finds the OS Node

Last login: Wed Jun 12 13:53:00 on ttys005
/Users/gleb/git/cypress/build/darwin/Cypress.app/Contents/MacOS/Cypress ; exit;
~ on mac
$ /Users/gleb/git/cypress/build/darwin/Cypress.app/Contents/MacOS/Cypress ; exit;
finding Node with $PATH /Library/Frameworks/Python.framework/Versions/2.7/bin:/opt/local/bin:/opt/local/sbin:/Users/gleb/.nvm/versions/node/v6.12.2/bin:/opt/local/bin:/opt/local/sbin:/opt/local/bin:/opt/local/sbin:/usr/local/bin:/Users/gleb/dev/arcanist/bin:/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin:/opt/X11/bin:/Applications/Postgres.app/Contents/Versions/latest/bin
found Node /Users/gleb/.nvm/versions/node/v6.12.2/bin/node
plugins file Node version v6.12.2
plugins file on, Node version v6.12.2
logout
Saving session...
...copying shared history...
...saving history...truncating history files...
...completed.

[Process completed]

@bahmutov bahmutov requested a review from brian-mann June 12, 2019 18:36
@bahmutov
Copy link
Contributor Author

bahmutov commented Jun 12, 2019

before I go to write some tests, is this ok for this feature? seems to be enough to control the Node version. @brian-mann

@brian-mann
Copy link
Member

Can we rename "node": "find" to something like "node": "*" or like <system> - something that doesn't look like a path that helps communicate that we're going to use the current one enabled in the shell?

Another idea: we could automatically detect if you're using a .node-version in your project (or other common files) and attempt to source that one in too.

Why? Because if you're running on multiple projects you may switch the node version between shells, which would then cause Cypress to pick the "last one". Ideally if there's something in your project that specifies it, that would be the best way for us to know which one to switch to.

Another idea: provide us the actual version like "node": "10.0.0" which will fail if it's not installed via nvm, etc. Then it's super clear.

@brian-mann
Copy link
Member

Beyond the use of plugins - the big win of this is that when we process your spec files, that we use your version of node.

I believe so far you've demonstrated switching node only for plugins but not for spec processing.

@brian-mann
Copy link
Member

Thinking about this a bit more... I actually dont think we should support absolute paths to the node version. Nothing in cypress.json should be user specific - every setting should be OS independent and compatible.

I guess we could still allow for it but maybe display a warning. A better approach is anchoring ourselves to something in the project files, or the user providing a specific node version so we know when it's correct and when it's not correct.

@brian-mann
Copy link
Member

Bleh... I guess this would likely making running in CI more difficult, which is why the * is likely necessary, but not recommended.

Maybe something like how heroku does it - enabling you to pass in a node version like 10 or 10.1 or 10.1.2.

@bahmutov
Copy link
Contributor Author

so I would love to specify Node version (and read it from .node-version if found) like "*" to find any version, and "10.0.0" to use specific, but then we need to figure out how to find it reliably cross-platform. Which is harder than finding current Node and falling back to fix-path + find again if it fails.

Then the second iteration could be "find specific version of Node"

I will look into bundling now

@brian-mann
Copy link
Member

I think the challenge here is that whatever they specify may be problematic when running in CI, etc.

Could we just download that version of node if we can't find it locally and like... cache it in the Cypress cache folder for future use?

I'm also imagining a UI in the config area that gives you a series of radios to help you pick the node version, and whether or not it was found on your system.

Honestly downloading the specific version of node would be a pretty failsafe way of always having it in all environments and users.

@bahmutov
Copy link
Contributor Author

Automatic downloading to me looks almost magical, even if we cache it into ~/.cache automatically. I feel like most people would just love to use whatever system version of Node they have right now - which could even be different on local machine vs CI - as long as it is not the Node version that comes with Cypress. I for example would love to use Node 10 to run plugins (node-sass) locally, because Parcel bundler requires it, and on CI would just use Docker image cypress/base:10

@bahmutov
Copy link
Contributor Author

I have checked the bundling of files - it uses the found Node version, just like processing of plugins file. In the log below, the first Node version if the Cypress Test Runner own code (Node 8) and then the bundling prints its Node version (12)

  cypress:server:preprocessor getFile /Users/gleb/git/cypress-test-tiny/cypress/integration/spec.js +0ms
  cypress:server:plugins plugin event registered? { event: 'file:preprocessor', isRegistered: true } +1s
  cypress:server:plugins execute plugin event 'file:preprocessor' Node 'v8.9.3' with args: EventEmitter { domain: null, _events: { rerun: [Function] }, _eventsCount: 1, _maxListeners: undefined, filePath: '/Users/gleb/git/cypress-test-tiny/cypress/integration/spec.js', shouldWatch: true, outputPath: '/Users/gleb/Library/Application Support/Cypress/cy/development/projects/cypress-test-tiny-a98bbf8a7f1688a4442c8bc398984ac5/bundles/cypress/integration/spec.js' } undefined undefined +0ms
  cypress:server:plugins call event file:preprocessor for invocation id inv2 +2ms
file:preprocessor file /Users/gleb/git/cypress-test-tiny/cypress/integration/spec.js Node version v12.4.0

@bahmutov
Copy link
Contributor Author

Ok, after thinking some more, I believe the great majority of issues related to the built-in Node version can be solved by loading system Node. For this I think we should allow just two options in cypress.json

  • "node": "built-in" runs the bundled Node, which is the only possible behavior today
  • "node": "system" should find the current OS Node and use it to execute the plugins file

I think any more features (like UI drop-down, auto-installing Node, trying to read .node-version and find the specific Node version) are nice, but are a lot more work and not strictly necessary to solve native extensions and JavaScript features in plugins file problems.

@brian-mann
Copy link
Member

You are probably right... but maybe there is some middle ground.

At the very least we need to:

  • add the node version to the output via cypress run and likely need to capture it and display it in the dashboard
  • display the node version in the Settings tab via cypress open so you can at least see which version it's using and where it found that version of node.

The property for this should likely be: nodeVersion and if we did just stick to two options it would probably be...

  • default or bundled
  • system

@bahmutov
Copy link
Contributor Author

yeah, agree that we need to show it in the cypress run output and in the GUI but I would do it in the separate issue. For now I will clean this PR to remove relative user path stuff and update tests

@bahmutov
Copy link
Contributor Author

bahmutov commented Sep 3, 2019

@mpahuja, unfortunately, the change is inside the Cypress compiled binary code, so we cannot even release NPM package fix. For me the work here is done, it is up to @brian-mann to review and merge it (soon)

@mpahuja
Copy link

mpahuja commented Sep 3, 2019

I am sure and also hopeful that he will get to it as soon as he can.

@mpahuja
Copy link

mpahuja commented Sep 3, 2019

@bahmutov what do you think about using this https://github.com/electron/electron-rebuild as a workaround in the meantime?

@drumbeg
Copy link

drumbeg commented Sep 4, 2019

@bahmutov what do you think about using this https://github.com/electron/electron-rebuild as a workaround in the meantime?

Try it! It didn't work consistently for me whilst trying to get the Oracle Node driver working on Windows. Seems to depend on exact set-up.

@bahmutov
Copy link
Contributor Author

bahmutov commented Sep 4, 2019

yeah @mpahuja I think just updating our code by merging this pull request is a lot better way than rebuilding native modules.

@mpahuja
Copy link

mpahuja commented Sep 4, 2019

completely understand Gleb, just curious on the timeline for this to be merged so that I can make an informed decision about whether i need to use the workaround or not.

# Conflicts:
#	packages/desktop-gui/cypress/integration/settings_spec.js
#	packages/desktop-gui/src/projects/projects-api.js
#	packages/server/lib/config.coffee
@jennifer-shehane
Copy link
Member

This test is failing because when run locally - the node version is 8.0.0 then in CI it is 12.0.0, but we are blindly replacing 8.0.0 or 12.0.0 with 0.0.0 so that there is an extra char being lost in CI. I need to update the snapshot replacement to account for and replace the exact number of digits from Node version.

@jennifer-shehane jennifer-shehane dismissed brian-mann’s stale review October 1, 2019 19:23

Changes were addressed as asked

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.

Allow using external Node when running tasks / plugins Native modules used inside plugin fails on Window

6 participants