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.

Sign up for GitHub

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

fix: use util.getEnv to handle environment variables set with npm #19560

Merged
merged 6 commits into from
Jan 12, 2022
Merged

fix: use util.getEnv to handle environment variables set with npm #19560

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
969a198
use util.getEnv to handle environment variables set with npm
legap Jan 5, 2022
ef9c092
Merge branch 'develop' into issue-19559-set-verify-timeout-from-npmrc
legap Jan 9, 2022
79f2a5f
Merge branch 'develop' into issue-19559-set-verify-timeout-from-npmrc
legap Jan 11, 2022
cdcc719
Merge branch 'develop' into issue-19559-set-verify-timeout-from-npmrc
mjhenkes Jan 11, 2022
d15ffce
Merge branch 'develop' into issue-19559-set-verify-timeout-from-npmrc
mjhenkes Jan 11, 2022
ff38f4b
Merge branch 'develop' into issue-19559-set-verify-timeout-from-npmrc
emilyrohrbough Jan 11, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cli/lib/tasks/verify.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ const logger = require('../logger')
const xvfb = require('../exec/xvfb')
const state = require('./state')

const VERIFY_TEST_RUNNER_TIMEOUT_MS = +process.env.CYPRESS_VERIFY_TIMEOUT || 30000
const VERIFY_TEST_RUNNER_TIMEOUT_MS = +util.getEnv('CYPRESS_VERIFY_TIMEOUT') || 30000
Copy link
Member

Choose a reason for hiding this comment

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

Testing this locally, I'm noticing the capitalization makes a difference on the resolved value when testing with a simple dummy script.

"scripts": {
   "try": "node ./try.js"
}
// try.js
const util = require('./cli/lib/util')

console.log(process.env)
console.log(' process.env -.CYPRESS_VERIFY_TIMEOUT', process.env.CYPRESS_VERIFY_TIMEOUT)
console.log('process.env.cypress_verify_timeout -', process.env.cypress_verify_timeout)
console.log('util.getEnv('CYPRESS_VERIFY_TIMEOUT') - ', util.getEnv('CYPRESS_VERIFY_TIMEOUT'))
console.log('util.getEnv('cypress_verify_timeout') -', util.getEnv('cypress_verify_timeout'))

.npmrc

CYPRESS_VERIFY_TIMEOUT=yes

Terminal

> CYPRESS_VERIFY_TIMEOUT='no' npm run try
process.env - {
   ...
  npm_config_cypress_verify_timeout: 'yes',
  CYPRESS_VERIFY_TIMEOUT: 'no',
  ...
}
process.env.CYPRESS_VERIFY_TIMEOUT - no
process.env.cypress_verify_timeout - undefined
util.getEnv('CYPRESS_VERIFY_TIMEOUT') -  no
util.getEnv('cypress_verify_timeout') - yes

Copy link
Contributor Author

@legap legap Jan 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the review and the testing @emilyrohrbough.

I had to dig a little deeper to understand how npm handles environment variables. The results are available in this README in a small test project.

As a conclusion we can see, that npm itself handles the key of an environment variable different, depending on the operating system (I didn't have an OSX available but I would guess it behaves the same as linux).

I also looked up all calls to the getEnv method in the cypress codebase and had a look at the tests in util_spec.js and they are always with uppercase keys.

So in my opinion getEnv should work on all operating systems when using the key in uppercase and the benefit would be that we can also use the definition for CYPRESS_VERIFY_TIMEOUT in package.json or .npmrc.

The only exception is setting the variable in .npmrc where the result will vary depending on the operating system. But I guess this should be addressed with another issue for the getEnv method.

Copy link
Member

Choose a reason for hiding this comment

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

@legap WOW! You led a very thorough investigation into these differences. I truly appreciate it & this is great information.

You conclusions regarding Mac OS seems correct (I have a Mac). With your findings it appears this accomplishes the intent of how CYPRESS_VERIFY_TIMEOUT will be used and accessed for this timeout.


const checkExecutable = (binaryDir) => {
const executable = state.getPathToExecutable(binaryDir)
Expand Down
8 changes: 8 additions & 0 deletions cli/test/lib/tasks/verify_spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,14 @@ context('lib/tasks/verify', () => {
expect(newVerifyInstance.VERIFY_TEST_RUNNER_TIMEOUT_MS).to.eql(500000)
})

it('accepts custom verify task timeout from npm', () => {
process.env.npm_config_CYPRESS_VERIFY_TIMEOUT = '500000'
delete require.cache[require.resolve(`${lib}/tasks/verify`)]
const newVerifyInstance = require(`${lib}/tasks/verify`)

expect(newVerifyInstance.VERIFY_TEST_RUNNER_TIMEOUT_MS).to.eql(500000)
})

it('falls back to default verify task timeout if custom value is invalid', () => {
process.env.CYPRESS_VERIFY_TIMEOUT = 'foobar'
delete require.cache[require.resolve(`${lib}/tasks/verify`)]
Expand Down