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

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

Conversation

legap
Copy link
Contributor

@legap legap commented Jan 5, 2022

User facing changelog

  • CYPRESS_VERIFY_TIMEOUT can also be set in package.json and .npmrc (depending on os)

Additional details

This change allows the CYPRESS_VERIFY_TIMEOUT environment variable to be set via .npmrc file or as config in package.json. The test cases were also updated to verify this case.

How has the user experience changed?

Users can set CYPRESS_VERIFY_TIMEOUT the same way as the other environment variables (config in package.json and .npmrc with some restrictions).

PR Tasks

  • Have tests been added/updated?
  • Has the original issue (or this PR, if no issue exists) been tagged with a release in ZenHub? (user-facing changes only)
  • [n/a] Has a PR for user-facing changes been opened in cypress-documentation?
  • [n/a] Have API changes been updated in the type definitions?
  • [n/a] Have new configuration options been added to the cypress.schema.json?

@legap legap requested a review from a team as a code owner January 5, 2022 11:14
@legap legap requested review from jennifer-shehane and removed request for a team January 5, 2022 11:14
@cypress-bot
Copy link
Contributor

cypress-bot bot commented Jan 5, 2022

Thanks for taking the time to open a PR!

@@ -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

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.

@emilyrohrbough emilyrohrbough merged commit 576519e into cypress-io:develop Jan 12, 2022
@jennifer-shehane jennifer-shehane removed their request for review January 12, 2022 15:53
mschile added a commit that referenced this pull request Jan 18, 2022
commit d8fa85d
Author: Chris Breiding <chrisbreiding@users.noreply.github.com>
Date:   Fri Jan 14 09:48:43 2022 -0500

    chore: Fix a couple multi-domain bugs (#19698)

commit 2e5fbad
Author: Chris Breiding <chrisbreiding@gmail.com>
Date:   Thu Jan 13 11:44:35 2022 -0500

    fix types issue

commit cc08d12
Author: Chris Breiding <chrisbreiding@gmail.com>
Date:   Thu Jan 13 09:56:31 2022 -0500

    fix issues after merge

commit 8e0770f
Merge: 2ee9893 d87711e
Author: Chris Breiding <chrisbreiding@gmail.com>
Date:   Thu Jan 13 09:31:25 2022 -0500

    Merge branch 'develop' into feature-multidomain

commit d87711e
Merge: 576519e f22e3ca
Author: Brian Barrow <briancbarrow@gmail.com>
Date:   Wed Jan 12 16:41:35 2022 +0000

    Merge branch 'master' into develop

commit f22e3ca
Author: Brian Barrow <briancbarrow@gmail.com>
Date:   Wed Jan 12 09:40:48 2022 -0700

    Fixed Vue README links in Global Components section (#19550)

commit 576519e
Author: Pascal Gafner <gafner.pascal@gmail.com>
Date:   Wed Jan 12 15:52:26 2022 +0100

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

    Co-authored-by: Matt Henkes <mjhenkes@gmail.com>
    Co-authored-by: Emily Rohrbough <emilyrohrbough@users.noreply.github.com>

commit 0382768
Author: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Date:   Tue Jan 11 21:35:43 2022 +0000

    chore(deps): update dependency electron to v15.3.4 🌟 (#19657)

    Co-authored-by: Renovate Bot <bot@renovateapp.com>

commit 1305cca
Author: Lachlan Miller <lachlan.miller.1990@outlook.com>
Date:   Wed Jan 12 07:10:14 2022 +1000

    fix: rename specs to correctly match convention (#19641)

    * fix: rename specs to correctly match convention

    * Remove underscore from TESTFILES glob pattern

    Co-authored-by: Zach Bloomquist <github@chary.us>

commit c45a240
Author: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Date:   Tue Jan 11 12:59:14 2022 -0800

    fix(deps): update dependency node-forge to v1 [security] (#19635)

    Co-authored-by: Renovate Bot <bot@renovateapp.com>

commit ea531b7
Author: Kukhyeon Heo <sainthkh@naver.com>
Date:   Wed Jan 12 00:37:05 2022 +0900

    chore: remove pkg/driver //@ts-nocheck part 2 (#19483)

    * listeners.ts

    * chainer.ts

    * command.ts

    * actionability.ts

    * inspect.ts

    * agents.ts

    * aliasing.ts

    * angular.ts

    * asserting.ts

    * clock.ts files

    * commands.ts

    * debugging.ts

    * fix comment.

    * roll back change.

    * Fix.

    * fix

    * Casted to cast.

    * Feedback changes.

    * fix any.

commit 513074e
Author: Josh Wooding <12938082+joshwooding@users.noreply.github.com>
Date:   Tue Jan 11 15:34:01 2022 +0000

    fix: overflow clip to prevent selector header from disapearing (#18649) (#19646)

    Co-authored-by: Tim Griesser <tgriesser10@gmail.com>

commit b8ccf12
Merge: 2071575 d227420
Author: Ryan Manuel <ryanm@cypress.io>
Date:   Mon Jan 10 15:38:23 2022 -0600

    Merge branch 'develop'

commit d227420
Author: Ryan Manuel <ryanm@cypress.io>
Date:   Mon Jan 10 15:34:34 2022 -0600

    release 9.2.1 [skip ci]

commit 5d1dce6
Author: Ryan Manuel <ryanm@cypress.io>
Date:   Mon Jan 10 13:01:12 2022 -0600

    Merge master to dev

commit 4818a21
Author: Juan Julián Merelo Guervós <jjmerelo@gmail.com>
Date:   Mon Jan 10 19:52:32 2022 +0100

    fix: update cli-table dependency to fix broken colors.js (#19622)

    Co-authored-by: Emily Rohrbough <emilyrohrbough@users.noreply.github.com>
    Co-authored-by: Ryan Manuel <ryanm@cypress.io>

commit 2071575
Author: semantic-release-bot <semantic-release-bot@martynus.net>
Date:   Mon Jan 10 11:23:17 2022 -0500

    chore: release @cypress/react-v5.12.1

    [skip ci]

commit 3f85a04
Merge: 642ec41 6304fd7
Author: Zachary Williams <ZachJW34@gmail.com>
Date:   Mon Jan 10 16:02:22 2022 +0000

    Merge branch 'master' into develop

commit 6304fd7
Author: Zachary Williams <ZachJW34@gmail.com>
Date:   Mon Jan 10 10:01:27 2022 -0600

    fix: check resolvedNodePath for Next.js 12 guard (#19604)

commit 10e3e0a
Author: semantic-release-bot <semantic-release-bot@martynus.net>
Date:   Tue Dec 21 14:35:12 2021 -0500

    chore: release @cypress/react-v5.12.0

    [skip ci]
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.

CYPRESS_VERIFY_TIMEOUT ignored when passed via .npmrc
3 participants