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

feat: support specPattern, deprecate integrationFolder and componentFolder #19168

Closed
wants to merge 91 commits into from

Conversation

lmiller1990
Copy link
Contributor

@lmiller1990 lmiller1990 commented Dec 1, 2021

Documented here (Cypress team only): https://www.notion.so/cypressdx/testFiles-specPattern-glob-patterns-295d545a0af942ebb5b71eda2a6d422b

This is a gigantic change that touches every inch of the system, and has several breaking changes. Those are:

  • deprecate testFiles, integrationFolder, component
  • introduce specPattern, configurable on component and e2e
  • ignoreTestFiles is now ignoreSpecPattern

Before

{
  integrationFolder: 'cypress/integration',
  testFiles: '**/*'
  ignoreTestFiles: '**.hot'
}

After

{
  e2e: {
    specPattern: 'cypress/integration/**/*',
    ignoreSpecPattern: '**.hot'
  }
}

Scaffolding cypress/integration is gone

Our scaffolding logic is very closely tied to cypress/integration existing. Since we no longer scaffold by default on first use, instead using a visual wizard to let the user choose which specs they'd like to scaffold, I deleted all this code and relevant tests.

util/spec.ts is gone

Now using the findSpecs method in data-context, which does the same thing with much less indirection, not relying on integration or component folders any more, instead just using the new specPattern property.

Breaking change for @cypress/code-coverage

No integrationFolder is a breaking change for code coverage. I patched it locally to get CI passing for now: https://github.com/cypress-io/cypress/pull/19168/files#diff-f7d0849fee5886e2b2efea84707ab2d2b8046975155572be66a486e5a7de1354. We will need to figure out the migration strategy here. Made an issue here.

Default is still cypress/integration/**/* instead of cypress/e2e/**/*.cy.js

As outlined in notion, we are changing the default directory for e2e from cypress/integration to cypress/e2e. Changing the default means changing all our projects in system-tests/projects (all 89 of them) and all the ones in npm. It's easy but adds a lot of noise to the PR. For this reason, I'll do those in a separate PR.

@cypress
Copy link

cypress bot commented Dec 2, 2021



Test summary

18168 0 202 0Flakiness 3


Run details

Project cypress
Status Passed
Commit ced245e
Started Dec 9, 2021 6:10 AM
Ended Dec 9, 2021 6:22 AM
Duration 11:52 💡
OS Linux Debian - 10.10
Browser Multiple

View run in Cypress Dashboard ➡️


Flakiness

commands/net_stubbing_spec.ts Flakiness
1 network stubbing > waiting and aliasing > can timeout waiting on a single request using "alias.request"
2 network stubbing > waiting and aliasing > can timeout waiting on a single request using "alias.request"
cypress/proxy-logging-spec.ts Flakiness
1 Proxy Logging > request logging > xhr log has response body/status code

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 lmiller1990 changed the title feat: specPattern feat: support specPattern, deprecate integrationFolder and componentFolder Dec 7, 2021
@@ -110,7 +105,9 @@ const resolvedOptions: Array<ResolvedConfigOption> = [
}, {
name: 'e2e',
// e2e runner overrides
defaultValue: {},
defaultValue: {
specPattern: 'cypress/integration/**/*',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should be cypress/e2e/**/*.cy.js but changing this means changing all our example projects, making this PR ungodly huge, so I will come back for this soon.

Also @tgriesser is doing work around config and this code will change a bunch soon (again) anyway.

return []
}

const pattern = await ctx.project.specPatternForTestingType(source.projectRoot, ctx.appData.currentTestingType)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

should cache these instead of hitting filesystem each time but outside scope of this PR

@@ -1137,7 +944,7 @@ describe('Routes', () => {
// .expect("Content-Type", /application\/json/)
// .expect(JSON.parse(json))

context('GET /__cypress/iframes/*', () => {
xcontext('GET /__cypress/iframes/*', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I deleted the code this is associated with, but going to leave this in case I missed something.

const { experimental } = require(`${root}../lib/experiments`)

describe('lib/modes/run', () => {
// todo(lachlan): put these back in when we've updated run.js
xdescribe('lib/modes/run', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This spec is basically impossible to get passing without a HUGE amount of extra stubbing, since we now use the data context in run.js... the unit tests here are really not very useful anyway, but I'm going to leave them here and hopefully keep some of them when we finish run mode for the unified runner.

@@ -39,7 +40,8 @@ exports['e2e new project passes 1'] = `
(Video)

- Started processing: Compressing to 32 CRF
- Finished processing: /XXX/XXX/XXX/cypress/videos/app_spec.js.mp4 (X second)
- Finished processing: /XXX/XXX/XXX/cypress/videos/cypress/integration/app_spec.js (X second)
Copy link
Member

Choose a reason for hiding this comment

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

I know this probably correct, but it somehow feels off... cypress/videos/cypress/integration vs cypress/videos/integration

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cypress/videos + <spec relative to projectRoot>

Previously we stripped of integrationFolder. Yes this feels weird, I agree.

https://cypressio.slack.com/archives/CNM0FKJFJ/p1638886882133300?thread_ts=1638886612.133100&cid=CNM0FKJFJ

Comment on lines -1863 to -1880
# testing scaffolding examples and running them
# against example.cypress.io
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to record this as an action item to follow up on / add back in one way or another?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm yes probably, we want to make sure all the scaffolded specs still function and work, just without all the complexity of the existing scaffolding setup.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lmiller1990
Copy link
Contributor Author

lmiller1990 commented Dec 9, 2021

gone in favor of #19319

@lmiller1990 lmiller1990 closed this Dec 9, 2021
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.

None yet

3 participants