-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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(grep): update cypress-grep references & docs & add e2e tests #24992
Conversation
Thanks for taking the time to open a PR!
|
7f1e4ec
to
a672f8f
Compare
47 flaky tests on run #43567 ↗︎
Details:
commands/net_stubbing.cy.ts • 1 flaky test • 5x-driver-firefox
e2e/origin/commands/navigation.cy.ts • 1 flaky test • 5x-driver-firefox
e2e/origin/commands/waiting.cy.ts • 1 flaky test • 5x-driver-firefox
e2e/origin/config_env.cy.ts • 1 flaky test • 5x-driver-firefox
e2e/origin/commands/assertions.cy.ts • 1 flaky test • 5x-driver-firefox
The first 5 flaky specs are shown, see all 25 specs in Cypress Cloud. This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. |
72b8fe3
to
bfd2944
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left one comment about addAxeCode
.
Didn't have the bandwidth to test run this, but otherwise looks straight forward.
e7e0f12
to
71ee31b
Compare
I updated to using |
00d9cc4
to
0aeb3f1
Compare
"name": "grep", | ||
"version": "0.0.0", | ||
"devDependencies": { | ||
"cypress": "latest", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't be installing Cypress but testing against the binary.
npm/grep/test.spec.js
Outdated
it('run Unit Tests', async () => { | ||
await runCommandInProject('npx cypress run --spec cypress/e2e/unit.js --config specPattern="**/unit.js"', projectPath) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there should not be unit tests in system-tests. These should live in npm/grep
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved
npm/grep/test.spec.js
Outdated
await runCommandInProject(`npx cypress-expect --min-passing 5 --pending 0`, projectPath) | ||
}) | ||
|
||
it('Run e2e skip tests without grep 🧪', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would expect these system tests to be modeled like the other system-tests in Cypress for maintainability purposes. If we need to make enhancements to support checking the report output, I think that'd be better than adding dozens of json files with the test results don't live with the test. At a min, this need to be running against the binary and not using npx
in these tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue is that the plugin needs the cypress
module, and it can't resolve without it. It is the same issue we have with @cypress/schematics
. This is why we have to move the system tests to a temp dir to run
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jordanpowell88 What do you mean exactly? Because it's listed as a peerDependency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it needs cypress as a peerDep but it also needs cypress as a devDep
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking how this is setup though, that's because of cypress-expect
right? which I why I am wondering if that's the right tool if we are unable to setup the system-tests to use the just-built binary. It looks like this is just a wrapper around running cypress programmatically to return the test results. We can pull the cypress output from systemTests.it
to determine the tests that ran and how many passed & failed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, we can do this but this would be a bigger lift than we are wanting to do at this moment
3a225d0
to
daa400d
Compare
@jordanpowell88 and you re-ping when this is ready to review? 🙏 |
Are there any updates for this @jordanpowell88 ? |
7e45cf4
to
c502d86
Compare
Hi @jordanpowell88 I hate to be that person, but is there any news here? |
Hi @jordanpowell88 I go the notification that this issue is closed, but the code hasn't been merged. Will the work continue in another PR? |
Let me find out for you. We should decide what's happening with cypress-grep moving forward. |
Thanks @jordanpowell88 What's with the tests, though? Will they be added in a separate PR? |
This will be up the @cypress-io/end-to-end team to create a follow issue around this as they are now responsible for handling the |
User facing changelog
fix references still pointing to
cypress-grep
Additional details
This PR also adds e2e tests as part of this lib
Steps to test
yarn workspace @cypress/grep test
// unit testsyarn workspace @cypress/grep cy:run
// e2e testsHow has the user experience changed?
PR Tasks
cypress-documentation
?type definitions
?