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: experimental retries #27930

Merged
merged 58 commits into from
Oct 26, 2023
Merged

feat: experimental retries #27930

merged 58 commits into from
Oct 26, 2023

Conversation

AtofStryker
Copy link
Contributor

@AtofStryker AtofStryker commented Sep 28, 2023

  • Closes N/A

Description

This feature branch his based of the feature/test-burn-in branch with the following commits reverted:

  • c428443 which introduces the experimentalBurnIn config option (PR). experimentalBurnIn is not ready for release yet and we have decided to omit the config options from the release of experimentalRetries
  • ae3df1a (PR) which removes the cloud burnInAction as it isn't needed for experimentalRetries release.
  • 83842a6 Removes the update the the experimentalBurnIn snapshot which was updated with the v13 release

ALL COMMITS ON THIS BRANCH HAVE BEEN PREVIOUSLY REVIEWED (with the exception of f010aef)!

Goals / Risk

The goal of this feature branch is to release experimentalRetries ahead of experimentalBurnIn, which will allow users to try the new configuration options for themselves and report any bugs if found.

The secondary goal of releasing experimentalRetries ahead of burn in is to get users to experiment with the experimental feature and identify bugs that might occur with experimentalRetries. However, the primary goal is to detect any issues that might have occurred adapting the existing GA retries to the new retries engine implemented under the hood of the Cypress App.

It's important to understand that in order to support experimentalRetries, we needed to patch mocha in order to rerun passed test attempts. To accomplish this, we needed to change how retries fundamentally work, which means signficant changes to the retries mechanism. In short, this means that the logic within the new calculateTestStatus function backports the retries functionality into the new engine. An easy way to think about this would be:

  • A GA config of retries: 2 would map to a strategy of detect-flake-and-pass-on-threshold with a maxRetries of 2 and a passesRequired of 1

In addition, the new field, called _cypressTestStatusInfo, which is used by both the cypress reporter in console and in the browser, is still used in GA implementation to interpret the test outerStatus, and will fallback to the status of the last attempt.

It's important to note that there isn't currently anything signaling that the implementation changes are an issue, but we want to ship this out to a wider audience to reduce the scope of changes and have the impact be deterministic. If issues occur with the release of this feature, we can determine those issues with as little noise as possible, and if needed, rollback easily and assess what went wrong.

For more information on this, please see the following PRs that go into the implementation details more specifically:

The Feature

Experimental retries will allow users to leverage what we are calling a retry strategy. These strategies include

  • detect-flake-and-pass-on-threshold
  • detect-flake-but-always-fail

detect-flake-and-pass-on-threshold is similar to how retries work today. Users will be able to specify a retry limit and specify a required number of passes needed in order for that to be marked as passed. Today, that limit is 1, but users can set the limit to whatever they'd like. This can be useful if a test is retrying and is known to be flaky, but the test engineers wishes to make sure that the test can at least pass a certain threshold in order to be marked as passed, which might help in confidence that the test is just flaky and the failure is not legitimate. It's important to note that if the passesRequired can not be acheived by the remaining retries of the test, the test will stop retrying and the test will be marked as a failure.

The other, detect-flake-but-always-fail, will always mark a test that enters retries as failed, regardless of how many retried attempts pass. There is an option here, called stopIfAnyPassed, that will stop the test retries if any attempt passes. Without this option, a user might want to see what the probability that their test might pass (similar to detect-flake-and-pass-on-threshold, but mark the test as failed as the test engineer might not want to mark any detected flaky test as a passed test.

with experimentalStrategy and experimentalOptions enabled

with only experimentalStrategy enabled

with neither enabled

For reference, here are some configuration examples as to hopw the experimental options compare to what exists today, and how their outputs differ.

current implementation (GA)

retries: 2

Experimental options

Detect Flake and Pass on Threshold
experimentalStrategy: 'detect-flake-and-pass-on-threshold'
experimentalOptions: {
  maxRetries: 9,
  passesRequired: 5
}

Detect Flake but Always Fail
experimentalStrategy: 'detect-flake-but-always-fail'
experimentalOptions: {
  maxRetries: 5,
  stopIfAnyPassed: false
}

Detect Flake but Always Fail (stopIfAnyPassed=true)
experimentalStrategy: 'detect-flake-but-always-fail'
experimentalOptions: {
  maxRetries: 10,
  stopIfAnyPassed: true
}

Steps to test

For how this experiment is tested, please see the description of #27826, which uses several testing strategies and mechanisms to guarantee functionality

How has the user experience changed?

Users will now be able to provide experimentalStrategy and experimentalOptions in their global config and watch the new form of retries take hold in their test

PR Tasks

AtofStryker and others added 19 commits July 24, 2023 11:19
…st-burnin

chore: merge develop into test burnin
* feat: add the burnIn Configuration to the config package. Option
currently is a no-op

* chore: make burn in experimental

* chore: set experimentalBurnIn to false by default
* feat: implement the experimental retries configuration options to pair
with test burn in

* [run ci]
chore: merge develop into feature/test-burn-in
* add burnInTestAction capability

* feat: add burn in capability for cloud

* chore: fix snapshot for record_spec
* chore: format the retries/runner snapshot files to make diff easier

* feat: implement experimentalRetries strategies 'detect-flake-and-pass-on-threshold' and 'detect-flake-but-always-fail'. This should not be a breaking change, though it does modify mocha and the test object even when the experiment is not configured. This is to exercise the system and make sure things still work as expected even when we go GA. Test updates will follow in following commits.

* chore: update snapshots from system tests and cy-in-cy tests that now have the cypress test metadata property _cypressTestStatusInfo. tests have been added in the fail-with-[before|after]each specs to visually see the suite being skipped when developing.

* chore: add cy-in-cy tests to verify reporter behavior for pass/fail tests, as well as new mocha snapshots to verify attempts. New tests were needed for this as the 'retries' option in testConfigOverrides currently is and will be invalid for experiment and will function as an override. tests run in the cy-in-cy tests are using globally configured experimentalRetries for the given tested project, which showcases the different behavior between attempts/retries and pass/fail status.

* chore: add unit test like driver test to verify the test object in mocha is decorated/handled properly in calculateTestStatus

* chore: add sanity system tests to verify console reporter output for experimental retries logic. Currently there is a bug in the reporter where the logged status doesnt wait for the aftereach to complete, which impacts the total exitCode and printed status.

* fix: aftereach console output. make sure to fail the test in the appropriate spot in runner.ts and not prematurely, which in turn updates the snapshots for cy-in-cy as the fail event comes later."

* chore: address comments from code review

* fix: make sure hook failures print outer status + attempts when the error is the hook itself.

* chore: improve types within calculateTestStatus inside mocha.ts
@AtofStryker AtofStryker changed the title Feature/experimental retries feat: experimental retries Sep 28, 2023
@@ -14,6 +14,7 @@ exports['module api and after:run results'] = `
"arch": "x64",
"baseUrl": null,
"blockHosts": null,
"experimentalBurnIn": false,
Copy link
Member

Choose a reason for hiding this comment

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

nit: Is it posssible to alphabetize this? I feel like Chris might have said it's not automatic. 😅

Regardless, approval given for this change as the codeowner of this file.

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 don't think we can but I cant remember. Either way this should not be there, so I am going to revert the commit it was introduced which was bb5046c

Copy link
Contributor Author

Choose a reason for hiding this comment

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

reverted in 83842a6

cacieprins and others added 2 commits October 20, 2023 16:51
Co-authored-by: Chris Breiding <chrisbreiding@users.noreply.github.com>
@@ -52,6 +52,22 @@ function setConfig (testConfig: ResolvedTestConfigOverride, config, localConfigO

try {
testConfig.applied = overrideLevel
// this is unique validation, not applied to the general cy config.
Copy link
Member

@emilyrohrbough emilyrohrbough Oct 23, 2023

Choose a reason for hiding this comment

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

Should this logic be pushed up the validateOverridableAtRunTime method to ensure the stack & messaging is consistent with the existing override error messaging (found here). That is the place that ensure these levels are appropriate for the config passed in.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can't declare invalid override levels for these keys because these are subkeys, and not defined as a primary config key. I can move the subkey detection to validateOverridableAtRunTime, but this logic will be removed in the next few weeks when we add support for setting experimental retry config at test & suite level overrides.

// If experimentalRetries are configured, a experimentalStrategy is present, and the retries configured is a boolean
// then we need to set the mocha '_retries' to 'maxRetries' present in the 'experimentalOptions' configuration.
if (testRetries['experimentalStrategy'] && _.isBoolean(retriesAsNumberOrBoolean) && retriesAsNumberOrBoolean) {
return testRetries['experimentalOptions'].maxRetries
Copy link
Member

Choose a reason for hiding this comment

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

If maxRetries are not set, does/should this fallback to the retriesAsNumberOrBoolean value? Or should this condition be verifying maxRetries is set?

Copy link
Contributor

Choose a reason for hiding this comment

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

This logic was previously reviewed (see PR description)

let shouldAttemptsContinue: boolean = true
let outerTestStatus: 'passed' | 'failed' | undefined = undefined

const passedTests = _.filter(test.prevAttempts, (o) => o.state === 'passed')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could probably do this and the next line in one loop and it'd be a shade more efficient. However, guessing that prevAttempts is likely not ever that large so ultimately doesn't matter. Up to you.

cli/CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@ryanthemanuel ryanthemanuel left a comment

Choose a reason for hiding this comment

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

Looks good! Couple of very minor things.

@@ -103,7 +103,9 @@ export const validateConfig = (state: State, config: Record<string, any>, skipCo
validateOverridableAtRunTime(config, isSuiteOverride, (validationResult) => {
let errKey = 'config.cypress_config_api.read_only'

if (validationResult.supportedOverrideLevel === 'suite') {
if (validationResult.supportedOverrideLevel === 'global_only') {
Copy link
Member

Choose a reason for hiding this comment

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

Can def fix this later, but read_only & global_only should be the same thing. can't be updated at test-time so must be set in the configuration file.

Copy link
Member

Choose a reason for hiding this comment

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

at least that is how I intended it to be if it's not working that way 🙃

@cacieprins cacieprins merged commit 201e9f3 into develop Oct 26, 2023
125 of 128 checks passed
@cacieprins cacieprins deleted the feature/experimental-retries branch October 26, 2023 18:06
@cypress-bot
Copy link
Contributor

cypress-bot bot commented Oct 31, 2023

Released in 13.4.0.

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

@cypress-bot cypress-bot bot locked as resolved and limited conversation to collaborators Oct 31, 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.