feat: Scope slowTestThreshold to specific testing types #20569

BlueWinds · 2022-03-10T22:49:15Z

Closes https://cypress-io.atlassian.net/browse/UNIFY-1302

User facing changelog

slowTestThreshold is no longer a valid global configuration option, and must be defined per testing type. This will be done as part of the automatic config migration where possible.

Additional details

If a user in 9.x has defined a custom slowTestThreshold at the global level:

{
  slowTestThreshold: 500
}

After this change, migrate to 10.x will update their config to this instead:

{
  e2e: {
    slowTestThreshold: 500
  },
  component: {
    slowTestThreshold: 500
  }
}

If they only have one testing type set up (eg, most users with only e2e tests), they will get the shorter version with slowTestThreshold moved inside their testing type:

{
  e2e: {
    slowTestThreshold: 500
  }
}

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? TODO
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-03-10T22:49:17Z

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-03-10T23:22:58Z

Test summary

17787 • 0 • 217 • 0 • 2

Run details


Project	cypress
Status	Passed
Commit	`7dc27ad`
Started	Mar 15, 2022 10:49 PM
Ended	Mar 15, 2022 11:01 PM
Duration	12:11 💡
OS	Linux Debian - 10.10
Browser	Multiple

View run in Cypress Dashboard ➡️

Flakiness

	net_stubbing.cy.ts	1
1	network stubbing > waiting and aliasing > yields the expected interception when two requests are raced
	navigation.cy.js	1
1	src/cy/commands/navigation > #visit > window immediately resolves and doesn't reload when visiting the same URL with hashes

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

elevatebart

A few changes that I don't understand.

Also, can you add a test for migration that actually moves the value from the root of the json file to the e2e tests?

packages/data-context/test/unit/sources/migration/codegen.spec.ts

lmiller1990

Code seems 💯 but why are we scoping this in the first place?

I also don't see this documented anywhere (like here: https://www.notion.so/cypressdx/Changes-for-10-0-0-1dca213837b84d6ea297c2d12c615f3a#d80e7d92e76c41b9b56673acf48f93cf).

Sorry to drop this here - just wanting to clarify whether

DX decided this and forgot to document it
Someone else decided this without it going through the correct process (usually DX should approve config changes)

cc @cowboy perhaps, master of the config changes document.

packages/config/lib/options.ts

jennifer-shehane

@BlueWinds Can you open a PR against the unification docs? The slowTestThreshold needs to be moved into the Testing-Type Specific Options for config: https://deploy-preview-4186--cypress-docs.netlify.app/guides/references/configuration#Testing-Type-Specific-Options

…owTestThreshold-by-testing-type

BlueWinds · 2022-03-14T18:59:02Z

@BlueWinds Can you open a PR against the unification docs? The slowTestThreshold needs to be moved into the Testing-Type Specific Options for config: https://deploy-preview-4186--cypress-docs.netlify.app/guides/references/configuration#Testing-Type-Specific-Options

Done in cypress-io/cypress-documentation#4388. Was on my TODO list, is why I didn't check it off in the PR Tasks checklist. :)

…e specific" This reverts commit c002011.

lmiller1990

One comment! We don't want to add component to the migrated config if you haven't previously used component testing; we should have a test where hasComponentTesting: false, which means "this user has not used component testing before".

packages/data-context/test/unit/sources/migration/codegen.spec.ts

Dismissing my previous review

BlueWinds · 2022-03-15T18:06:20Z

packages/data-context/test/unit/sources/migration/codegen.spec.ts

+    snapshot(generatedConfig)
+  })
+
+  it('should create only a component entry when no e2e specs are detected', async () => {


@lmiller1990 - I added this test and left the one below it, testing the case where only one testing type is detected. The snapshots for these two test display only component and only e2e blocks respectively.

emilyrohrbough · 2022-03-15T19:42:49Z

packages/config/lib/options.ts

+    errorKey: 'CONFIG_FILE_INVALID_ROOT_CONFIG',
+    isWarning: false,
+    testingTypes: ['component', 'e2e'],
+  },


On this PR, @estrada9166 added/updated typescript tests for baseUrl, which is another invalid root-level configuration option. Might be a good idea to add in tests for this option. Thoughts?

https://github.com/cypress-io/cypress/pull/20589/files#diff-facdf706a5238380b5e97c040bc1a5f5cb89e3116f878754dd7fb4e693787715L35

Looking at those tests, it doesn't look like we test every config value that way, just a few examples selected to show Cypress.config() reflects the typings of values that can be retrieved from it. We already have Cypress.config('taskTimeout') // $ExpectType number demonstrating that numbers are done correctly, and don't think slowTestThreshold needs to be added there.

Interesting though, I'd never seen that file before.

It looks like it can be added to verify the types are working as expected and/or throws an error when used incorrectly. i.e.

// cypress.config.js { slowTestThreshold: 200 // expect error as unsupported type }```

Cypress.config('slowTestThreshold', 1000) is not an error, nor should it be - in the resolved config, slowTestThreshold can be at the top level, and it can be changed on a per-test basis (unlike baseUrl). It's only in the config file that it can't be top-level.

emilyrohrbough · 2022-03-15T19:44:17Z

packages/data-context/src/sources/migration/codegen.ts

+        const ext = '**/*.cy.{js,jsx,ts,tsx}'
+        const isDefaultE2E = key === 'e2e' && specPattern === `cypress/e2e/${ext}`
+        const isDefaultCT = key === 'component' && specPattern === ext


not a change for this PR, just in general..would love to see these default values pulled from the config packages instead of hard-coded here

packages/data-context/src/sources/migration/codegen.ts

emilyrohrbough · 2022-03-15T19:53:47Z

packages/data-context/src/sources/migration/codegen.ts

+      case 'supportFile':
+        return {
+          ...acc,
+          e2e: { ...acc.e2e, supportFile: val },


hmm sorry @BlueWinds...sanity check though, currently in 9x the support file is shared between e2e and component right? Shouldn't this map to both?

Hm, it does sound like it should to me. @lmiller1990, thoughts?

I asked exactly the same question at the time - Brian said nope, we just map to e2e.supportFile. During CT setup we will create a new file called cypress/support/component.js.

(Yes - this means that CT users will have some small inconvenience during migration from 9.x -> 10.x. But CT users are in the minority).

You can ping in the migration channel if you feel this is weird and should be revisited.

@lmiller1990 I did ask there as well. No response yet.

packages/data-context/src/sources/migration/codegen.ts

…owTestThreshold-by-testing-type

lmiller1990

@emilyrohrbough pointed out lots of potential small improvements we could make, I don't see those as blockers to this PR, since those were not changes introduced here. Definitely good to capture those in Jira tickets.

As for the actual code and PR, my comments have been addressed. ✅

cypress-bot · 2022-06-01T15:48:26Z

Released in 10.0.0.

This comment thread has been locked. If you are still experiencing this issue after upgrading to
Cypress v10.0.0, please open a new issue.

Move slowTestThreshold into testing type specific configuration

95b9fc7

Attempt TS type fix

87f8159

BlueWinds marked this pull request as ready for review March 10, 2022 23:14

ryanthemanuel requested a review from emilyrohrbough March 11, 2022 16:45

ryanthemanuel assigned BlueWinds Mar 11, 2022

ryanthemanuel requested review from marktnoonan and elevatebart and removed request for marktnoonan March 11, 2022 16:46

elevatebart suggested changes Mar 11, 2022

View reviewed changes

packages/data-context/test/unit/sources/migration/codegen.spec.ts Show resolved Hide resolved

lmiller1990 reviewed Mar 14, 2022

View reviewed changes

packages/config/lib/options.ts Outdated Show resolved Hide resolved

jennifer-shehane previously requested changes Mar 14, 2022

View reviewed changes

BlueWinds added 2 commits March 14, 2022 11:13

Merge remote-tracking branch 'origin/10.0-release' into UNIFY-1302-sl…

0114a59

…owTestThreshold-by-testing-type

Fix types

1d0df41

BlueWinds mentioned this pull request Mar 14, 2022

Update docs to scope slowTestThreshold by testing type cypress-io/cypress-documentation#4388

Merged

BlueWinds added 3 commits March 14, 2022 13:20

Move default slowtestthreshold config value to be testing type specific

c002011

Revert "Move default slowtestthreshold config value to be testing typ…

2206852

…e specific" This reverts commit c002011.

Fix system test

538c4ae

BlueWinds requested review from elevatebart, jennifer-shehane and lmiller1990 March 15, 2022 01:01

lmiller1990 reviewed Mar 15, 2022

View reviewed changes

packages/data-context/test/unit/sources/migration/codegen.spec.ts Show resolved Hide resolved

flotwig requested a review from lmiller1990 March 15, 2022 15:33

BlueWinds commented Mar 15, 2022

View reviewed changes

emilyrohrbough reviewed Mar 15, 2022

View reviewed changes

packages/data-context/src/sources/migration/codegen.ts Show resolved Hide resolved

emilyrohrbough reviewed Mar 15, 2022

View reviewed changes

packages/data-context/src/sources/migration/codegen.ts Show resolved Hide resolved

Merge remote-tracking branch 'origin/10.0-release' into UNIFY-1302-sl…

7dc27ad

…owTestThreshold-by-testing-type

lmiller1990 approved these changes Mar 16, 2022

View reviewed changes

emilyrohrbough approved these changes Mar 16, 2022

View reviewed changes

BlueWinds merged commit 99181ba into 10.0-release Mar 16, 2022

BlueWinds deleted the UNIFY-1302-slowTestThreshold-by-testing-type branch March 16, 2022 16:12

cypress-bot bot locked as resolved and limited conversation to collaborators Jun 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Scope slowTestThreshold to specific testing types #20569

feat: Scope slowTestThreshold to specific testing types #20569

BlueWinds commented Mar 10, 2022 •

edited

Loading

cypress-bot bot commented Mar 10, 2022

cypress bot commented Mar 10, 2022 •

edited

Loading

elevatebart left a comment

lmiller1990 left a comment

jennifer-shehane left a comment

BlueWinds commented Mar 14, 2022

lmiller1990 left a comment

BlueWinds Mar 15, 2022

lmiller1990 Mar 16, 2022

emilyrohrbough Mar 15, 2022

BlueWinds Mar 15, 2022

emilyrohrbough Mar 16, 2022

BlueWinds Mar 16, 2022

emilyrohrbough Mar 15, 2022

emilyrohrbough Mar 15, 2022

BlueWinds Mar 15, 2022

lmiller1990 Mar 16, 2022

emilyrohrbough Mar 16, 2022

lmiller1990 left a comment

cypress-bot bot commented Jun 1, 2022

feat: Scope slowTestThreshold to specific testing types #20569

feat: Scope slowTestThreshold to specific testing types #20569

Conversation

BlueWinds commented Mar 10, 2022 • edited Loading

User facing changelog

Additional details

How has the user experience changed?

PR Tasks

cypress-bot bot commented Mar 10, 2022

cypress bot commented Mar 10, 2022 • edited Loading

Test summary

Run details

Flakiness

elevatebart left a comment

Choose a reason for hiding this comment

lmiller1990 left a comment

Choose a reason for hiding this comment

jennifer-shehane left a comment

Choose a reason for hiding this comment

BlueWinds commented Mar 14, 2022

lmiller1990 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmiller1990 left a comment

Choose a reason for hiding this comment

cypress-bot bot commented Jun 1, 2022

BlueWinds commented Mar 10, 2022 •

edited

Loading

cypress bot commented Mar 10, 2022 •

edited

Loading