fix: resolves correctly specPattern #21057

estrada9166 · 2022-04-13T03:16:45Z

Closes UNIFY-1461

User facing changelog

The default specPattern was being resolved as it was coming from the user config file which was incorrect; the reason why it was being resolved from the config file was that when trying to search for the default value, it was never found because it was nested in the testing type - and we are merging the testing type into the top-level config, creating a mismatch between the defaults values and the config itself

Before

After

Additional details

How has the user experience changed?

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)
Has a PR for user-facing changes been opened in cypress-documentation?
Have API changes been updated in the type definitions?
Have new configuration options been added to the cypress.schema.json?

cypress-bot · 2022-04-13T03:16:47Z

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-04-13T03:32:03Z

Test summary

17872 • 0 • 217 • 0 • 2

Run details


Project	cypress
Status	Passed
Commit	`dd5a2d1`
Started	Apr 14, 2022 6:20 PM
Ended	Apr 14, 2022 6:34 PM
Duration	13:34 💡
OS	Linux Debian - 10.10
Browser	Multiple

View run in Cypress Dashboard ➡️

Flakiness

	commands/xhr.cy.js	1
1	... > no status when request isnt forced 404
	cypress/proxy-logging.cy.ts	1
1	Proxy Logging > request logging > xhr log has response body/status code when xhr response is logged second

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

lmiller1990

One recommendation and one general question

lmiller1990 · 2022-04-13T05:40:39Z

packages/app/cypress/e2e/settings.cy.ts

+      })
+
+      cy.get('[data-cy="config-code"]').within(() => {
+        cy.get('.bg-gray-50').contains('cypress/e2e/**/*.cy.{js,jsx,ts,tsx}')


I wonder if these should be data selectors, like [data-cy-config="resolved"]. I do not like using CSS much for test selectors, it's super hard to read and tends to be brittle. What do you think?

lmiller1990 · 2022-04-13T05:41:56Z

packages/server/lib/config.ts

+
+  // As we normalize the config based on the selected testing type, we need
+  // to do the same with the default values to resolve those correctly
+  defaultsForRuntime = { ...defaultsForRuntime, ...defaultsForRuntime[options.testingType] }


Interesting - kind of surprised we need to do this, I thought this was a UI only bug - so was the bug more deeper in the system, too?

The config stuff is so spread out all over the place, probably why we have some many bugs around it :(

+1 all configuration things should prob be happening in the config package to keep this logic in one place and so we can make it easily testable.

emilyrohrbough · 2022-04-13T14:12:39Z

packages/server/lib/config.ts

+
+  // As we normalize the config based on the selected testing type, we need
+  // to do the same with the default values to resolve those correctly
+  defaultsForRuntime = { ...defaultsForRuntime, ...defaultsForRuntime[options.testingType] }


Suggested change

defaultsForRuntime = { ...defaultsForRuntime, ...defaultsForRuntime[options.testingType] }

defaultsForRuntime = {

...defaultsForRuntime,

...defaultsForRuntime[options.testingType]

}

~~Below we spread these values with the ignore type. Was the config overriding the default resolved values?~~

Are we able to add a unit test for this?

EDIT: talked offline, looks like it was stomping on itself.

_.defaultsDeep(config, defaultsForRuntime) is updating the config with the default, including the nested types which are totally correct, but the issue comes here config = setResolvedConfigValues(config, defaultsForRuntime, resolved) because, at that moment config has the nested type merged with the top level, but the nested fields as specPattern do not exist on top-level, so defaultsForRuntime does not include it unless we merge the testing type with the top-level ones, and when we try to search for the resolved one, it does not exist on the top-level (the default) so we assume it was coming from users config

…ult-specPattern

lmiller1990

Thanks for the quick follow up... ✔️

…ult-specPattern

flotwig · 2022-04-14T16:24:57Z

packages/server/lib/config.ts

+  // As we normalize the config based on the selected testing type, we need
+  // to do the same with the default values to resolve those correctly
+  defaultsForRuntime = {
+    ...defaultsForRuntime,
+    ...defaultsForRuntime[options.testingType],
+  }


imo, this logic would be more suitably moved to packages/config/src/index.ts where getDefaultValues is defined. this is the only usage of that function, so IMO it would make sense to confine this logic to the config package. what do you think @estrada9166 ?

Sounds good! yeah, I'm gonna update it!

…ult-specPattern

…e-config * 10.0-release: fix: make error on integration folder point to e2e (#20853) fix(unify): Update Cypress Dashboard Service Link in Login Modal (#21084) chore: fix windows node_modules install step (#21098) fix: webpack integration tests for app w webpack-dev-server-fresh (#21093) fix: move non spec files on migration (#21054) Bumping electron version in root chore(deps): Bumping electron dependency to 15.5.1 (#21072) fix: resolves correctly specPattern (#21057) feat: replace reconfigure button on settings page with link to config doc (#21077) feat(launchpad): update CT setup and config scaffolding (#20893) fix: cy.type('{enter}') on <input> elements submits the form correctly after Firefox 98. (#21042) chore: making the npm deps for vue, react, and vue2 use 0.0.0-dev (#21081) fix(cli): show additional mitigation steps for max path length error (#21047) fix: Plugin error when sending on disconnected IPC channel (#21011) chore: add internal types for cy.session command (#21028) chore: Update Chrome (stable) to 100.0.4896.88 (#21043)

fix: resolves correctly specPattern

ccf5a76

estrada9166 requested a review from a team as a code owner April 13, 2022 03:16

estrada9166 requested review from jennifer-shehane and removed request for a team April 13, 2022 03:16

estrada9166 requested review from emilyrohrbough and lmiller1990 and removed request for jennifer-shehane April 13, 2022 03:17

lmiller1990 reviewed Apr 13, 2022

View reviewed changes

emilyrohrbough reviewed Apr 13, 2022

View reviewed changes

estrada9166 and others added 2 commits April 13, 2022 09:28

Merge branch '10.0-release' into alejandro/fix/resolve-correctly-defa…

270ae07

…ult-specPattern

Update test

f42fd00

estrada9166 requested review from emilyrohrbough and lmiller1990 April 13, 2022 15:42

estrada9166 added 2 commits April 13, 2022 11:40

Merge branch '10.0-release' into alejandro/fix/resolve-correctly-defa…

1c822f8

…ult-specPattern

Merge branch '10.0-release' into alejandro/fix/resolve-correctly-defa…

fc87c0d

…ult-specPattern

estrada9166 requested a review from tbiethman April 14, 2022 00:07

lmiller1990 approved these changes Apr 14, 2022

View reviewed changes

Merge branch '10.0-release' into alejandro/fix/resolve-correctly-defa…

faa00e7

…ult-specPattern

flotwig requested review from flotwig and removed request for emilyrohrbough and tbiethman April 14, 2022 15:36

flotwig suggested changes Apr 14, 2022

View reviewed changes

estrada9166 and others added 2 commits April 14, 2022 12:20

Update with feedback

3f403f5

Merge branch '10.0-release' into alejandro/fix/resolve-correctly-defa…

e1db09d

…ult-specPattern

estrada9166 requested a review from flotwig April 14, 2022 17:28

Merge branch '10.0-release' into alejandro/fix/resolve-correctly-defa…

260d0d9

…ult-specPattern

flotwig approved these changes Apr 14, 2022

View reviewed changes

Merge branch '10.0-release' into alejandro/fix/resolve-correctly-defa…

dd5a2d1

…ult-specPattern

estrada9166 merged commit 7f69f94 into 10.0-release Apr 14, 2022

estrada9166 deleted the alejandro/fix/resolve-correctly-default-specPattern branch April 14, 2022 18:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: resolves correctly specPattern #21057

fix: resolves correctly specPattern #21057

estrada9166 commented Apr 13, 2022

cypress-bot bot commented Apr 13, 2022

cypress bot commented Apr 13, 2022 •

edited

Loading

lmiller1990 left a comment

lmiller1990 Apr 13, 2022

lmiller1990 Apr 13, 2022

emilyrohrbough Apr 13, 2022

emilyrohrbough Apr 13, 2022

emilyrohrbough Apr 13, 2022 •

edited

Loading

estrada9166 Apr 13, 2022

lmiller1990 left a comment

flotwig Apr 14, 2022

estrada9166 Apr 14, 2022

fix: resolves correctly specPattern #21057

fix: resolves correctly specPattern #21057

Conversation

estrada9166 commented Apr 13, 2022

User facing changelog

Before

After

Additional details

How has the user experience changed?

PR Tasks

cypress-bot bot commented Apr 13, 2022

cypress bot commented Apr 13, 2022 • edited Loading

Test summary

Run details

Flakiness

lmiller1990 left a comment

Choose a reason for hiding this comment

lmiller1990 Apr 13, 2022

Choose a reason for hiding this comment

lmiller1990 Apr 13, 2022

Choose a reason for hiding this comment

emilyrohrbough Apr 13, 2022

Choose a reason for hiding this comment

emilyrohrbough Apr 13, 2022

Choose a reason for hiding this comment

emilyrohrbough Apr 13, 2022 • edited Loading

Choose a reason for hiding this comment

estrada9166 Apr 13, 2022

Choose a reason for hiding this comment

lmiller1990 left a comment

Choose a reason for hiding this comment

flotwig Apr 14, 2022

Choose a reason for hiding this comment

estrada9166 Apr 14, 2022

Choose a reason for hiding this comment

cypress bot commented Apr 13, 2022 •

edited

Loading

emilyrohrbough Apr 13, 2022 •

edited

Loading