feat: Defaulting nodeVersion to system (#18732)

* Defaulting nodeVersion to system

* try to fix system test

* Rename arg parameters, fix system test in a much better way.

* remove invalid comment

* Add deprecation warning for the nodeVersion config.

* Remove default value to avoid warning regardless of the presence of `nodeVersion`

* More tests fixes 😅

* Updates to deprecation message

* update node version in deprecation notice.

* flex config file name that we tell consumers to update

* simplify validateNoBreakingConfig options

cli/lib/exec/spawn.js

@@ -81,7 +81,7 @@ module.exports = {
       args = [args]
     }
 
-    args = [...args, '--cwd', process.cwd()]
+    args = [...args, '--cwd', process.cwd(), '--userNodePath', process.execPath, '--userNodeVersion', process.versions.node]
 
     _.defaults(options, {
       dev: false,

cli/test/lib/exec/spawn_spec.js

@@ -18,6 +18,8 @@ const expect = require('chai').expect
 const snapshot = require('../../support/snapshot')
 
 const cwd = process.cwd()
+const execPath = process.execPath
+const nodeVersion = process.versions.node
 
 const defaultBinaryDir = '/default/binary/dir'
 
@@ -98,6 +100,10 @@ describe('lib/exec/spawn', function () {
           '--foo',
           '--cwd',
           cwd,
+          '--userNodePath',
+          execPath,
+          '--userNodeVersion',
+          nodeVersion,
         ], {
           detached: false,
           stdio: ['inherit', 'inherit', 'pipe'],
@@ -122,6 +128,10 @@ describe('lib/exec/spawn', function () {
           '--foo',
           '--cwd',
           cwd,
+          '--userNodePath',
+          execPath,
+          '--userNodeVersion',
+          nodeVersion,
         ]
 
         expect(args).to.deep.equal(['/path/to/cypress', expectedCliArgs])
@@ -142,6 +152,10 @@ describe('lib/exec/spawn', function () {
           '--foo',
           '--cwd',
           cwd,
+          '--userNodePath',
+          execPath,
+          '--userNodeVersion',
+          nodeVersion,
         ], {
           detached: false,
           stdio: ['inherit', 'inherit', 'pipe'],
@@ -163,6 +177,10 @@ describe('lib/exec/spawn', function () {
           '--foo',
           '--cwd',
           cwd,
+          '--userNodePath',
+          execPath,
+          '--userNodeVersion',
+          nodeVersion,
         ], {
           detached: false,
           stdio: ['inherit', 'inherit', 'pipe'],

packages/server/lib/config.ts

@@ -11,7 +11,6 @@ import origin from './util/origin'
 import * as settings from './util/settings'
 import Debug from 'debug'
 import pathHelpers from './util/path_helpers'
-import findSystemNode from './util/find_system_node'
 
 const debug = Debug('cypress:server:config')
 
@@ -63,14 +62,29 @@ const convertRelativeToAbsolutePaths = (projectRoot, obj, defaults = {}) => {
   , {})
 }
 
-const validateNoBreakingConfig = (cfg) => {
-  return _.each(breakingOptions, ({ name, errorKey, newName, isWarning }) => {
-    if (_.has(cfg, name)) {
+const validateNoBreakingConfig = (config) => {
+  breakingOptions.forEach(({ name, errorKey, newName, isWarning, value }) => {
+    if (config.hasOwnProperty(name)) {
+      if (value && config[name] !== value) {
+        // Bail if a value is specified but the config does not have that value.
+        return
+      }
+
       if (isWarning) {
-        return errors.warning(errorKey, name, newName)
+        return errors.warning(errorKey, {
+          name,
+          newName,
+          value,
+          configFile: config.configFile,
+        })
       }
 
-      return errors.throw(errorKey, name, newName)
+      return errors.throw(errorKey, {
+        name,
+        newName,
+        value,
+        configFile: config.configFile,
+      })
     }
   })
 }
@@ -293,6 +307,8 @@ export function mergeDefaults (config: Record<string, any> = {}, options: Record
 
   config = setParentTestsPaths(config)
 
+  config = setNodeBinary(config, options.args?.userNodePath, options.args?.userNodeVersion)
+
   // validate config again here so that we catch
   // configuration errors coming from the CLI overrides
   // or env var overrides
@@ -305,7 +321,6 @@ export function mergeDefaults (config: Record<string, any> = {}, options: Record
   return setSupportFileAndFolder(config)
   .then(setPluginsFile)
   .then(setScaffoldPaths)
-  .then(_.partialRight(setNodeBinary, options.onWarning))
 }
 
 export function setResolvedConfigValues (config, defaults, resolved, options) {
@@ -453,22 +468,19 @@ export function resolveConfigValues (config, defaults, resolved = {}, options =
 }
 
 // instead of the built-in Node process, specify a path to 3rd party Node
-export const setNodeBinary = Promise.method((obj, onWarning) => {
-  if (obj.nodeVersion !== 'system') {
-    obj.resolvedNodeVersion = process.versions.node
+export const setNodeBinary = (obj, userNodePath, userNodeVersion) => {
+  // if execPath isn't found we weren't executed from the CLI and should used the bundled node version.
+  if (userNodePath && userNodeVersion && obj.nodeVersion !== 'bundled') {
+    obj.resolvedNodePath = userNodePath
+    obj.resolvedNodeVersion = userNodeVersion
 
     return obj
   }
 
-  return findSystemNode.findNodePathAndVersion()
-  .then(({ path, version }) => {
-    obj.resolvedNodePath = path
-    obj.resolvedNodeVersion = version
-  }).catch((err) => {
-    onWarning(errors.get('COULD_NOT_FIND_SYSTEM_NODE', process.versions.node))
-    obj.resolvedNodeVersion = process.versions.node
-  }).return(obj)
-})
+  obj.resolvedNodeVersion = process.versions.node
+
+  return obj
+}
 
 export function setScaffoldPaths (obj) {
   obj = _.clone(obj)

packages/server/lib/config_options.ts

@@ -151,8 +151,7 @@ export const options = [
     isInternal: true,
   }, {
     name: 'nodeVersion',
-    defaultValue: 'default',
-    validation: v.isOneOf('default', 'bundled', 'system'),
+    validation: v.isOneOf('bundled', 'system'),
   }, {
     name: 'numTestsKeptInMemory',
     defaultValue: 50,
@@ -333,5 +332,15 @@ export const breakingOptions = [
     name: 'firefoxGcInterval',
     errorKey: 'FIREFOX_GC_INTERVAL_REMOVED',
     isWarning: true,
+  }, {
+    name: 'nodeVersion',
+    value: 'system',
+    errorKey: 'NODE_VERSION_DEPRECATION_SYSTEM',
+    isWarning: true,
+  }, {
+    name: 'nodeVersion',
+    value: 'bundled',
+    errorKey: 'NODE_VERSION_DEPRECATION_BUNDLED',
+    isWarning: true,
   },
 ]

packages/server/lib/errors.js

@@ -633,7 +633,7 @@ const getMsgByType = function (type, arg1 = {}, arg2, arg3) {
         We found an invalid value in the file: ${chalk.blue(filePath)}
 
         ${chalk.yellow(arg2)}`
-    // happens when there is an invalid config value returnes from the
+    // happens when there is an invalid config value returned from the
     // project's plugins file like "cypress/plugins.index.js"
     case 'PLUGINS_CONFIG_VALIDATION_ERROR':
       filePath = `\`${arg1}\``
@@ -650,9 +650,9 @@ const getMsgByType = function (type, arg1 = {}, arg2, arg3) {
         ${chalk.yellow(arg1)}`
     case 'RENAMED_CONFIG_OPTION':
       return stripIndent`\
-        The ${chalk.yellow(arg1)} configuration option you have supplied has been renamed.
+        The ${chalk.yellow(arg1.name)} configuration option you have supplied has been renamed.
 
-        Please rename ${chalk.yellow(arg1)} to ${chalk.blue(arg2)}`
+        Please rename ${chalk.yellow(arg1.name)} to ${chalk.blue(arg1.newName)}`
     case 'CANNOT_CONNECT_BASE_URL':
       return stripIndent`\
         Cypress failed to verify that your server is running.
@@ -932,7 +932,7 @@ const getMsgByType = function (type, arg1 = {}, arg2, arg3) {
         You can safely remove this option from your config.`
     case 'EXPERIMENTAL_COMPONENT_TESTING_REMOVED':
       return stripIndent`\
-        The ${chalk.yellow(`\`experimentalComponentTesting\``)} configuration option was removed in Cypress version \`7.0.0\`. Please remove this flag from \`cypress.json\`.
+        The ${chalk.yellow(`\`experimentalComponentTesting\``)} configuration option was removed in Cypress version \`7.0.0\`. Please remove this flag from ${chalk.yellow(`\`${arg1.configFile}\``)}.
 
         Cypress Component Testing is now a standalone command. You can now run your component tests with:
 
@@ -1010,6 +1010,16 @@ const getMsgByType = function (type, arg1 = {}, arg2, arg3) {
         ${arg1 ? 'Try installing Node.js 64-bit and reinstalling Cypress to use the 64-bit build.'
         : 'Consider upgrading to a 64-bit OS to continue using Cypress.'}
         `
+    case 'NODE_VERSION_DEPRECATION_SYSTEM':
+      return stripIndent`\
+      Deprecation Warning: ${chalk.yellow(`\`${arg1.name}\``)} is currently set to ${chalk.yellow(`\`${arg1.value}\``)} in the ${chalk.yellow(`\`${arg1.configFile}\``)} configuration file. As of Cypress version \`9.0.0\` the default behavior of ${chalk.yellow(`\`${arg1.name}\``)} has changed to always use the version of Node used to start cypress via the cli.
+      Please remove the ${chalk.yellow(`\`${arg1.name}\``)} configuration option from ${chalk.yellow(`\`${arg1.configFile}\``)}.
+      `
+    case 'NODE_VERSION_DEPRECATION_BUNDLED':
+      return stripIndent`\
+      Deprecation Warning: ${chalk.yellow(`\`${arg1.name}\``)} is currently set to ${chalk.yellow(`\`${arg1.value}\``)} in the ${chalk.yellow(`\`${arg1.configFile}\``)} configuration file. As of Cypress version \`9.0.0\` the default behavior of ${chalk.yellow(`\`${arg1.name}\``)} has changed to always use the version of Node used to start cypress via the cli. When ${chalk.yellow(`\`${arg1.name}\``)} is set to ${chalk.yellow(`\`${arg1.value}\``)}, Cypress will use the version of Node bundled with electron. This can cause problems running certain plugins or integrations. 
+      As the ${chalk.yellow(`\`${arg1.name}\``)} configuration option will be removed in a future release, it is recommended to remove the ${chalk.yellow(`\`${arg1.name}\``)} configuration option from ${chalk.yellow(`\`${arg1.configFile}\``)}.
+      `
     default:
   }
 }

packages/server/lib/open_project.ts

@@ -42,6 +42,8 @@ export interface LaunchArgs {
   testingType: Cypress.TestingType
   invokedFromCli: boolean
   os: PlatformName
+  userNodePath?: string
+  userNodeVersion?: string
 
   onFocusTests?: () => any
   /**

packages/server/lib/util/args.js

@@ -13,7 +13,48 @@ const nestedObjectsInCurlyBracesRe = /\{(.+?)\}/g
 const nestedArraysInSquareBracketsRe = /\[(.+?)\]/g
 const everythingAfterFirstEqualRe = /=(.*)/
 
-const allowList = 'appPath apiKey browser ci ciBuildId clearLogs config configFile cwd env execPath exit exitWithCode group headed inspectBrk key logs mode outputPath parallel ping port project proxySource quiet record reporter reporterOptions returnPkg runMode runProject smokeTest spec tag updating version testingType'.split(' ')
+const allowList = [
+  'apiKey',
+  'appPath',
+  'browser',
+  'ci',
+  'ciBuildId',
+  'clearLogs',
+  'userNodePath',
+  'userNodeVersion',
+  'config',
+  'configFile',
+  'cwd',
+  'env',
+  'execPath',
+  'exit',
+  'exitWithCode',
+  'group',
+  'headed',
+  'inspectBrk',
+  'key',
+  'logs',
+  'mode',
+  'outputPath',
+  'parallel',
+  'ping',
+  'port',
+  'project',
+  'proxySource',
+  'quiet',
+  'record',
+  'reporter',
+  'reporterOptions',
+  'returnPkg',
+  'runMode',
+  'runProject',
+  'smokeTest',
+  'spec',
+  'tag',
+  'testingType',
+  'updating',
+  'version',
+]
 // returns true if the given string has double quote character "
 // only at the last position.
 const hasStrayEndQuote = (s) => {

packages/server/lib/util/find_system_node.js

@@ -1,11 +1,8 @@
 const debug = require('debug')('cypress:server:find_system_node')
-const execa = require('execa')
 const fixPath = require('fix-path')
 const Promise = require('bluebird')
 const which = require('which')
 
-const NODE_VERSION_RE = /^v(\d+\.\d+\.\d+)/m
-
 /*
  * Find the full path to a `node` binary on the current PATH.
  * Note about fix-path:
@@ -41,44 +38,6 @@ function findNodeInFullPath () {
   })
 }
 
-function findNodeVersionFromPath (path) {
-  if (!path) {
-    return Promise.resolve(null)
-  }
-
-  return execa.stdout(path, ['-v'])
-  .then((stdout) => {
-    debug('node -v returned %o', { stdout })
-    const matches = NODE_VERSION_RE.exec(stdout)
-
-    if (matches && matches.length === 2) {
-      const version = matches[1]
-
-      debug('found Node version', { version })
-
-      return version
-    }
-  })
-  .catch((err) => {
-    debug('could not resolve Node version %o', { err })
-
-    throw err
-  })
-}
-
-function findNodePathAndVersion () {
-  return findNodeInFullPath()
-  .then((path) => {
-    return findNodeVersionFromPath(path)
-    .then((version) => {
-      return {
-        path, version,
-      }
-    })
-  })
-}
-
 module.exports = {
   findNodeInFullPath,
-  findNodePathAndVersion,
 }