-
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: actionability not retrying in after hooks where the test failed #30831
base: develop
Are you sure you want to change the base?
Conversation
cypress Run #60090
Run Properties:
|
Project |
cypress
|
Branch Review |
after-hook-action-fix
|
Run status |
Passed #60090
|
Run duration | 17m 35s |
Commit |
3908fb0d5e: merge develop
|
Committer | Jennifer Shehane |
View all properties for this run ↗︎ |
Test results | |
---|---|
Failures |
0
|
Flaky |
5
|
Pending |
1099
|
Skipped |
0
|
Passing |
26538
|
View all changes introduced in this branch ↗︎ |
UI Coverage
45.56%
|
|
---|---|
Untested elements |
191
|
Tested elements |
164
|
Accessibility
92.55%
|
|
---|---|
Failed rules |
3 critical
8 serious
2 moderate
2 minor
|
Failed elements |
888
|
it('clicks element in hook', (done) => { | ||
cy.on('fail', (err) => { | ||
expect(err.message).contain('expected true to be false') | ||
done() |
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 don't think you want to use done()
in these tests, otherwise the test is going to pass even if the assertions in the after
fail.
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.
@mschile If you remove the code for the fix, this test does fail in the after hook.
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, but the test will also pass if you modify any of the assertions in the after
hook since you've already informed mocha that the test is done.
cli/CHANGELOG.md
Outdated
@@ -1,10 +1,11 @@ | |||
<!-- See the ../guides/writing-the-cypress-changelog.md for details on writing the changelog. --> | |||
## 14.0.1 | |||
|
|||
_Released 1/28/2025 (PENDING)_ | |||
_Released 1/28/2024 (PENDING)_ |
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.
_Released 1/28/2024 (PENDING)_ | |
_Released 1/28/2025 (PENDING)_ |
@@ -116,7 +115,7 @@ export const create = (Cypress: ICypress, state: StateFunc, timeout: $Cy['timeou | |||
// bug in bluebird with not propagating cancellations | |||
// fast enough in a series of promises | |||
// https://github.com/petkaantonov/bluebird/issues/1424 | |||
return state('canceled') || state('error') || runnableHasChanged() | |||
return state('canceled') || runnableHasChanged() |
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.
Any idea if this code have any impact on before/beforeEach blocks? We have quite a few tests for this I believe in https://github.com/cypress-io/cypress/blob/after-hook-action-fix/packages/app/cypress/e2e/runner/runner.mochaEvents.cy.ts so it appears to not. Usually if the before
/beforeEach
fails I believe we skip the suite but I don't think that marks the runnable as canceled, but has the same effect?
I don't think we are going to know the true impact of this until we release it to the general public which might introduce the use/test case we are missing here... or it may not introduce anything at all except a fix!
Additional details
When an action (such as click or type) is performed in an
after
hook where the test has failed, the actionability check is not retried and the click/type/other action? does not execute as expected. This is because Cypress is seeing that state('error') is set (since the test failed) and exiting because it believes the current runnable is in an error state so should stop retrying the check.cypress/packages/driver/src/cy/actionability.ts
Line 621 in c50a138
In actuality, we're in a different runnable - where the state has not errored. So we should continue retrying in the new runnable where the error is not present.
To fix the issue, I removed the check for
state('error')
altogether in the retries logic. This seems like it was put there for a purpose - and that tests would fail if you remove it, but all of our tests pass. I'm assuming that logic changed at some point where the condition is met properly regardless of this check now, but it'd be great to have some deeper verification.Steps to test
Added tests to click.ts and type.ts although I feel like this isn't quite the right place for this logic since it's addressing an issue in retries, but no unit tests are there.
How has the user experience changed?
Click and type in after hooks now execute properly.
PR Tasks
cypress-documentation
?type definitions
?