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

chore: better validation messages for experimental retries #28214

Merged
merged 8 commits into from
Dec 5, 2023

Conversation

MuazOthman
Copy link
Contributor

@MuazOthman MuazOthman commented Nov 1, 2023

PR Tasks

Copy link
Member

@emilyrohrbough emilyrohrbough left a comment

Choose a reason for hiding this comment

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

Could you add screenshots of these error combinations?

Copy link
Contributor

@mabela416 mabela416 left a comment

Choose a reason for hiding this comment

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

Not passing passedRequired for detect-flake-and-pass-on-threshold and stopIfAnyPassed for detect-flake-but-always-fail is NOT erroring but it's supposed to return an error.
image
image

@mabela416
Copy link
Contributor

For this legacy config, I'm also NOT getting an error when starting the run but I do after the run is done

  retries: {
    openMode: true,
    runMode: true,
  },

image

BUT in develop, I do get an error when trying to start my run

image

@mabela416
Copy link
Contributor

this config should also error out but instead it lets me do a run which ends with this error

  retries: {
    runMode: true,
    openMode: true
  },

image

@MuazOthman
Copy link
Contributor Author

@mabela416 I fixed the validation for the last 2 comments. Validating fields within experimentalOptions are beyond the scope of this PR since adding that would be a change of behavior. cc: @ryanpei for feedback on that.

@MuazOthman MuazOthman requested a review from mabela416 November 15, 2023 17:50
Copy link

cypress bot commented Nov 15, 2023

5 flaky tests on run #52558 ↗︎

0 5467 80 0 Flakiness 5

Details:

Merge branch 'develop' into muaz/CYCLOUD-1765-better-retries-validation
Project: cypress Commit: 899ea4364d
Status: Passed Duration: 17:06 💡
Started: Nov 28, 2023 6:50 PM Ended: Nov 28, 2023 7:07 PM
Flakiness  commands/storage.cy.ts • 1 flaky test • 5x-driver-chrome:beta

View Output

Test Artifacts
src/cy/commands/storage > #getAllLocalStorage > gets local storage from all origins Test Replay
Flakiness  e2e/origin/commands/storage.cy.ts • 1 flaky test • 5x-driver-chrome:beta

View Output

Test Artifacts
cy.origin storage > .getAllLocalStorage Test Replay
Flakiness  cypress/cypress.cy.js • 3 flaky tests • 5x-driver-chrome:beta

View Output

Test Artifacts
... > correctly returns currentRetry Test Replay
... > correctly returns currentRetry Test Replay
... > correctly returns currentRetry Test Replay

Review all test suite changes for PR #28214 ↗︎

@mabela416
Copy link
Contributor

@mabela416 I fixed the validation for the last 2 comments. Validating fields within experimentalOptions are beyond the scope of this PR since adding that would be a change of behavior. cc: @ryanpei for feedback on that.

@MuazOthman Validating fields within experimentalOptions is already happening in develop and it's being removed in this PR which should not be happening

@mabela416
Copy link
Contributor

This is the error i get in develop when I don't pass passesRequired or stopIfAnyPassed

image

image

Copy link
Contributor

@mabela416 mabela416 left a comment

Choose a reason for hiding this comment

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

One small nitpick:
When I pass a config without the runMode and openMode, I get this message but I think it should say the value passed was undefined
image

image

@MuazOthman
Copy link
Contributor Author

@mabela416 it shows 0 in that error message because this is the default value, we can probably make this less confusing once strategy-based retries becomes GA.

@MuazOthman MuazOthman requested a review from mabela416 November 22, 2023 21:11
@MuazOthman
Copy link
Contributor Author

@emilyrohrbough validation error messages can be seen under packages/config/__snapshots__/validation.spec.ts.js

@MuazOthman MuazOthman merged commit 522835d into develop Dec 5, 2023
82 of 84 checks passed
@MuazOthman MuazOthman deleted the muaz/CYCLOUD-1765-better-retries-validation branch December 5, 2023 15:31
@cypress-bot
Copy link
Contributor

cypress-bot bot commented Dec 5, 2023

Released in 13.6.1.

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

@cypress-bot cypress-bot bot locked as resolved and limited conversation to collaborators Dec 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants