feat: CLI `config` not supporting nested properties correctly #20127

BlueWinds · 2022-02-09T20:42:54Z

Closes UNIFY-1028

User facing changelog

Can now pass in testing-type specific config options via --config without nesting JSON.

Additional details

With the addition of more testing type specific config options, and especially with the requirement that specPattern be configured per testing type, using --config via command line was a bit clunky, requiring a user to nest the config value.

How has the user experience changed?

Before

$ cd packages/driver
$ node ../../scripts/cypress run '--config={"e2e": {"specPattern": "**/*.spec.jsx"}}'
# Works!

$ node ../../scripts/cypress run '--config={"specPattern": "**/*.spec.jsx"}'
`specPattern` is not a valid configuration option

https://on.cypress.io/configuration

`specPattern` is not a valid configuration option

After

$ node ../../scripts/cypress run '--config={"e2e": {"specPattern": "**/*.spec.jsx"}}'
# Still works!

$ node ../../scripts/cypress run '--config={"specPattern": "**/*.spec.jsx", "baseUrl": "https://example.com"}'
# Now also works

This example resolves to the the config of

{
  component: {
    specPattern: "**/*.spec.jsx" // specPattern is a valid config for both component and e2e testing, so it is included in both.
  },
  e2e: {
    baseUrl: "https://example.com", // baseUrl is valid only for e2e, so it is included only here.
    specPattern: "**/*.spec.jsx"
  }
}

PR Tasks

Have tests been added/updated?
[N/A] Has the original issue (or this PR, if no issue exists) been tagged with a release in ZenHub? (user-facing changes only)
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?

cypress-bot · 2022-02-09T20:42:56Z

Thanks for taking the time to open a PR!

Create a Draft Pull Request if your PR is not ready for review. Mark the PR as Ready for Review when you're ready for a Cypress team member to review the PR.
Become familiar with the Code Review Checklist for guidelines on coding standards and what needs to be done before a PR can be merged.

cypress · 2022-02-09T20:54:04Z

Test summary

4390 • 0 • 51 • 0 • 0

Run details


Project	cypress
Status	Passed
Commit	`fc5135a`
Started	Feb 24, 2022 9:59 PM
Ended	Feb 24, 2022 10:11 PM
Duration	11:53 💡
OS	Linux Debian - 10.10
Browser	Electron 94

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

…ss-io/cypress into issue-UNIFY-1028-cli-nested-config

BlueWinds · 2022-02-10T19:28:45Z

A number of tests are failing here on this branch, but I'm seeing the same failures in 10.0-release, and even develop when running locally - it feels like something changed with regards to cookies recently, perhaps a browser version upgrade?

Going to put this as ready for review while I look into it, since I suspect the solution is unrelated to the changes in here.

emilyrohrbough · 2022-02-10T20:24:52Z

Without looking at the changes, could I run something like:

--e2e --config= "specPattern":["*/test2.coffee","*/test1.js"]}

BlueWinds · 2022-02-10T20:47:28Z

Without looking at the changes, could I run something like:

Yep! You have a mismatching brace in your example and probably need to quote it so bash doesn't munge the json, but it would work as:

--e2e --config='{"specPattern":["*/test2.coffee","*/test1.js"]}'

...though one sec while I verify.

Edit: Verified. Yes, the above example does work as expected.

lmiller1990

Also, I resolved the conflicts - let's see if CI passes now.

I want to clarify: we use JSON for --config, but the actual config is no longer JSON, but a JS module. Do you know if this has been discussed (stick w/ JSON on config, even if we are not using JSON for the cypress.config.js. What about if they try to pass a key that's a function, like devServer? I wonder if we should error.

I'm guessing this is something @cypress-io/dx has considered previously when thinking about cypress.config.js?

lmiller1990 · 2022-02-17T01:32:56Z

packages/server/test/unit/util/args_spec.js

@@ -595,6 +595,20 @@ describe('lib/util/args', () => {
        config: {},
      })
    })
+
+    it('moves testing-type specific config options', function () {


Cool, looks good. Do we have a pre-existing pattern for if they pass invalid config, eg --config={blablah=false}, or I think we just ignore these? It looks like we will inherit the same behavior with the hoisted keys, just wanted to clarify we do the same thing for the hoisted keys.

I haven't touched the handling of invalid keys. The hoisting occurs via whitelist - we only pull keys from the breakingRootOptions down into testing-type specific options, so if it's not whitelisted, we don't touch it here (and the same behavior as before will apply).

I haven't looked into exactly what that 'same behavior' is, but whatever it is, it'll still work! :D

cowboy · 2022-02-17T17:07:40Z

I want to clarify: we use JSON for --config, but the actual config is no longer JSON, but a JS module. Do you know if this has been discussed (stick w/ JSON on config, even if we are not using JSON for the cypress.config.js. What about if they try to pass a key that's a function, like devServer? I wonder if we should error.

I'm guessing this is something @cypress-io/dx has considered previously when thinking about cypress.config.js?

We did discuss the --config option, and what I remember from that conversation (I couldn't find any documentation) was that all options specified via --config override the resolved config value, so that the user never needs to specify the e2e or component objects. It was my understanding that this is the 9.x behavior, and we were going to keep it in 10.0.

Also, I believe we decided that function options would not be able to be specified via --config.

Does this make sense? Are there any issues with this?

cowboy · 2022-02-17T17:13:31Z

--e2e --config='{"specPattern":["*/test2.coffee","*/test1.js"]}'

Not sure if this is relevant, but Windows cmd shell only understands " as a string delimiter, in case we're testing this there.

BlueWinds · 2022-02-17T17:46:44Z

We did discuss the --config option, and what I remember from that conversation (I couldn't find any documentation) was that all options specified via --config override the resolved config value, so that the user never needs to specify the e2e or component objects. It was my understanding that this is the 9.x behavior, and we were going to keep it in 10.0.

Also, I believe we decided that function options would not be able to be specified via --config.

Does this make sense? Are there any issues with this?

Makes sense to me, and matches what was implemented in the PR, thanks for explaining. Just wanted to double check to make sure this was desired and intended.

--e2e --config='{"specPattern":["*/test2.coffee","*/test1.js"]}'
Not sure if this is relevant, but Windows cmd shell only understands " as a string delimiter, in case we're testing this there.

No tests / docs using single quotes, provided here only as an example for discussion in the PR.

-cli-nested-config

BlueWinds · 2022-02-17T20:48:34Z

packages/data-context/src/data/ProjectLifecycleManager.ts


      // TODO: pass in options.config overrides separately, so they are reflected in the UI
-      configFileContents = { ...configFileContents, ...testingTypeOverrides }
+      configFileContents = { ...configFileContents, ...testingTypeOverrides, ...optionsOverrides }


Basically: The cli arg parsing lifts any testing-type specific args into e2e, and then buildBaseFullConfig puts them back in.

I went through probably half a dozen iterations on "why is baseUrl not getting set on the root config" before settling on this as the right place to put it, next to other code that already has the same purpose.

lmiller1990

thanks for clarification @cowboy, seems we are all good. Just need one more 👀 and ✔️ .

brian-mann

This was something we discussed during the design of the CLI changes as per 10.0

In fact there is not supposed to be support for passing nested --config values for e2e + component. They are supposed to be passed flattened for the testing type that you want applied.

So passing --config specPattern='...' is only applied to the testing type you eventually go into and is applied to both testing types. You don't pass different configuration to different testing types.

It this is something we'd like to revisit I'm all ears - but it was intentionally designed this way. It's possible the existing implementation was not working correctly, but the aforementioned behavior was the desired behavior.

Edit

Sorry I just reread the comments throughout this PR and clearly others have chimed in with the same information. @BlueWinds what's not clear from this PR is what specific behavior this is fixing. When I first read this I saw the PR description regarding the config not supporting nesting and assumed it was attempting to pass specific testing type config. What I'm looking for clarity on is what specific existing behavior is and what this PR is changing to the desired behavior. A simple before/after.

lmiller1990 · 2022-02-21T00:14:31Z

I tested this branch a bit more. This one isn't working for me unless I explicitly include --e2e or --component. I'm not sure if that's related, and it only seems to be for nested only config options (like specPattern).

# error - `specPattern` is not a valid configuration option
cd packages/driver
yarn cypress:run --config specPattern="**/*.spec.jsx"

# ok
cd packages/driver
yarn cypress:run --config specPattern="**/*.spec.jsx" --e2e

As for the problem this PR solves @brian-mann, it's related to when you pass a JSON object to --config. I think we want to do the same flattening logic there, too.

Before

$ node ../../scripts/cypress run --e2e '--config={"specPattern": "**/*.spec.jsx"}'
`specPattern` is not a valid configuration option

https://on.cypress.io/configuration

`specPattern` is not a valid configuration option

After

$ node ../../scripts/cypress run --e2e '--config={"specPattern": "**/*.spec.jsx"}'
# works!

-cli-nested-config

BlueWinds · 2022-02-21T15:55:43Z

Sorry I just reread the comments throughout this PR and clearly others have chimed in with the same information. @BlueWinds what's not clear from this PR is what specific behavior this is fixing. When I first read this I saw the PR description regarding the config not supporting nesting and assumed it was attempting to pass specific testing type config. What I'm looking for clarity on is what specific existing behavior is and what this PR is changing to the desired behavior. A simple before/after.

Latchaln put this fairly clearly, so I'll refer you to his comment for a before / after.

Currently in 10.0, most config values already work as you describe, getting included in whichever config type the user selects. The only ones that don't are those explicitly not available as root config values - eg, those in breakingRootOptions: supportFile, specPattern, excludeSpecPattern and baseUrl. These are not valid as 'root' config options, and so the CLI rejects them.

This PR aims to allow those breakingRootOptions to be applied to the appropriate testing type(s) when passed in via --config. I'll update the PR description once I address @lmiller1990's findings above.

BlueWinds · 2022-02-21T16:17:34Z

I tested this branch a bit more. This one isn't working for me unless I explicitly include --e2e or --component. I'm not sure if that's related, and it only seems to be for nested only config options (like specPattern).

It was checking to see what testing type the user had actually passed in, which, upon reflection, is not the correct action - we know what testingType baseUrl applies to. I've updated the PR so that it nests invalid root configs directly to the testing types they apply to - it will also now work in open mode when you switch between testing types, applying CLI args to the appropriate one.

This also means that if they run cypress open --component --config baseUrl="foo", the baseUrl will be ignored for component testing (as it's not valid there), but apply if they switch to e2e testing within the runner.

BlueWinds · 2022-02-21T17:05:48Z

Updated PR with clear before / after examples.

lmiller1990

Cool, seems good to me.

flotwig · 2022-02-24T16:22:36Z

Seems like a legit failure in server-unit-tests: https://app.circleci.com/pipelines/github/cypress-io/cypress/32422/workflows/962760ee-0761-4d37-b915-8babc3bf8e5d/jobs/1284475

-cli-nested-config

BlueWinds · 2022-02-24T23:02:11Z

Seems like a legit failure in server-unit-tests: https://app.circleci.com/pipelines/github/cypress-io/cypress/32422/workflows/962760ee-0761-4d37-b915-8babc3bf8e5d/jobs/1284475

Fixed.

* 10.0-release: chore: fix linting (#20388) feat(unify): rearrange settings (#20332) fix: make the glob groups work on windows fix: stop deleting files on install chore: fixing eslint duplicate imports fix(launchpad): scaffold correct config file (#20372) chore: Refactor Command Log UI (#20202) chore: fix launchpad's failing windows test (#20371) chore: UNIFY-860 remove all references to Cypress Cloud (#20369) chore: fix minor UI issues noticed in design review (#20251) feat: Sort browsers list alphabetically (#20248) fix: Improvements to Spec Pattern Rename "change" Modal (#20320) feat: CLI `config` not supporting nested properties correctly (#20127) fix: Relaunching app when Focus is pressed after app has closed (#20352)

Support setting testingType specific config at root via cli args

cde89f8

Blue F and others added 3 commits February 9, 2022 13:32

Merge branch '10.0-release' into issue-UNIFY-1028-cli-nested-config

55a8bbb

Properly merge CLI testing-type specific arguments

e96acd9

Merge branch 'issue-UNIFY-1028-cli-nested-config' of github.com:cypre…

e2581f8

…ss-io/cypress into issue-UNIFY-1028-cli-nested-config

BlueWinds marked this pull request as draft February 10, 2022 17:44

BlueWinds requested a review from emilyrohrbough February 10, 2022 19:27

BlueWinds marked this pull request as ready for review February 10, 2022 19:27

merge in origin/10.0-release

477f10d

lmiller1990 previously approved these changes Feb 17, 2022

View reviewed changes

BlueWinds added 2 commits February 17, 2022 11:30

Merge remote-tracking branch 'origin/10.0-release' into issue-UNIFY-1028

bdd95c4

-cli-nested-config

Include CLI overrides when refreshing project config in dataContext

ae0aac8

BlueWinds dismissed lmiller1990’s stale review via ae0aac8 February 17, 2022 19:58

Make TS happy

f5c236f

BlueWinds requested a review from lmiller1990 February 17, 2022 20:45

BlueWinds commented Feb 17, 2022

View reviewed changes

lmiller1990 previously approved these changes Feb 18, 2022

View reviewed changes

davidmunechika requested a review from flotwig February 18, 2022 16:52

brian-mann requested changes Feb 20, 2022

View reviewed changes

Merge remote-tracking branch 'origin/10.0-release' into issue-UNIFY-1028

ab9fc12

-cli-nested-config

Update config nesting to work without explicitly passed in testing type

6c56619

BlueWinds dismissed lmiller1990’s stale review via 6c56619 February 21, 2022 16:18

BlueWinds requested review from lmiller1990 and brian-mann February 21, 2022 17:05

BlueWinds added 2 commits February 21, 2022 09:53

Fix types

b66811d

Fix types gooder

41b489b

lmiller1990 previously approved these changes Feb 21, 2022

View reviewed changes

BlueWinds added 2 commits February 24, 2022 09:04

Fix server unit test for config nesting

c8e6611

Merge remote-tracking branch 'origin/10.0-release' into issue-UNIFY-1028

fc5135a

-cli-nested-config

BlueWinds dismissed lmiller1990’s stale review via fc5135a February 24, 2022 17:04

lmiller1990 merged commit e79fd64 into 10.0-release Feb 25, 2022

lmiller1990 deleted the issue-UNIFY-1028-cli-nested-config branch February 25, 2022 02:24

feat: CLI config not supporting nested properties correctly #20127

feat: CLI config not supporting nested properties correctly #20127

Uh oh!

Conversation

BlueWinds commented Feb 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

User facing changelog

Additional details

How has the user experience changed?

Before

After

PR Tasks

Uh oh!

cypress-bot bot commented Feb 9, 2022

Uh oh!

cypress bot commented Feb 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test summary

Run details

Uh oh!

BlueWinds commented Feb 10, 2022

Uh oh!

emilyrohrbough commented Feb 10, 2022

Uh oh!

BlueWinds commented Feb 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lmiller1990 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lmiller1990 Feb 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BlueWinds Feb 17, 2022

Choose a reason for hiding this comment

Uh oh!

cowboy commented Feb 17, 2022

Uh oh!

cowboy commented Feb 17, 2022

Uh oh!

BlueWinds commented Feb 17, 2022

Uh oh!

BlueWinds Feb 17, 2022

Choose a reason for hiding this comment

Uh oh!

lmiller1990 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brian-mann left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Edit

Uh oh!

lmiller1990 commented Feb 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Before

After

Uh oh!

BlueWinds commented Feb 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BlueWinds commented Feb 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BlueWinds commented Feb 21, 2022

Uh oh!

lmiller1990 left a comment

Choose a reason for hiding this comment

Uh oh!

flotwig commented Feb 24, 2022

Uh oh!

BlueWinds commented Feb 24, 2022

Uh oh!

Uh oh!

feat: CLI `config` not supporting nested properties correctly #20127

feat: CLI `config` not supporting nested properties correctly #20127

BlueWinds commented Feb 9, 2022 •

edited

Loading

cypress bot commented Feb 9, 2022 •

edited

Loading

BlueWinds commented Feb 10, 2022 •

edited

Loading

lmiller1990 left a comment •

edited

Loading

lmiller1990 Feb 17, 2022 •

edited

Loading

lmiller1990 left a comment •

edited

Loading

brian-mann left a comment •

edited

Loading

lmiller1990 commented Feb 21, 2022 •

edited

Loading

BlueWinds commented Feb 21, 2022 •

edited

Loading

BlueWinds commented Feb 21, 2022 •

edited

Loading