feat: add new experimental retries configuration #27412

AtofStryker · 2023-07-27T21:49:03Z

Closes N/A

Additional details

Introduces the experimentalStrategy and experimentalOptions keys inside the retries configuration object.

The config validation for this is a bit wonky in order to support the currently supported config of runMode and openMode. If none of the experimental keys are present, runMode and openMode can be the current API. However, if experimentalStrategy is provided, runMode and openMode MUST be booleans. Additionally, if the experimentalStrategy is valid, and experimentalOptions are provided, both keys MUST be present.

To display the experiment correctly, there is some hackiness to get the experimental values to display correctly in the Project Settings page. Since we do not support nested experimental flags inside traditional config, I figured it would be best to provide a one off for retries to check the experimental config options. When we remove the new retry logic out of experimental and into GA, we can remove this work around.

with `experimentalStrategy` and `experimentalOptions` enabled

with only `experimentalStrategy` enabled

with neither enabled

Steps to test

How has the user experience changed?

PR Tasks

Have tests been added/updated?
Has a PR for user-facing changes been opened in cypress-documentation?
Have API changes been updated in the type definitions?

with test burn in

cypress · 2023-07-27T21:53:12Z

38 flaky tests on run #49273 ↗︎

0	27952	1349	0	38

Details:

[run ci]
Project: cypress	Commit: `b8a14e785d`
Status: Passed	Duration: 20:32 💡
Started: Jul 27, 2023 10:22 PM	Ended: Jul 27, 2023 10:42 PM

commands/net_stubbing.cy.ts • 1 flaky test • 5x-driver-electron

View Output Video

Test		Artifacts
network stubbing > intercepting request > can delay and throttle a StaticResponse		`Output` `Video`

e2e/origin/commands/traversal.cy.ts • 1 flaky test • 5x-driver-electron

View Output Video

Test		Artifacts
cy.origin traversal > .children()		`Output` `Video`

e2e/origin/commands/screenshot.cy.ts • 1 flaky test • 5x-driver-electron

View Output Video

Test		Artifacts
cy.origin screenshot > set viewport > captures the fullPage		`Output` `Video`

e2e/origin/commands/navigation.cy.ts • 1 flaky test • 5x-driver-electron

View Output Video

Test		Artifacts
cy.origin navigation > .go()		`Output` `Video`

e2e/origin/commands/querying_shadow.cy.ts • 1 flaky test • 5x-driver-electron

View Output Video

Test		Artifacts
cy.origin shadow dom > .shadow()		`Output` `Video`

The first 5 flaky specs are shown, see all 27 specs in Cypress Cloud.

_{This comment has been generated by cypress-bot as a result of this
project's GitHub integration settings.}

AtofStryker · 2023-07-27T21:57:52Z

packages/config/src/validation.ts

+
+      // if options aren't present (either undefined or null) or are configured correctly, return true
+      if (isValidStrategy && isValidRunAndOpenModeConfigWithStrategy && (
+        experimentalConfigOptions.experimentalOptions == null ||


I think we should bounce on null here since it isn't a valid option. its either undefined or what is expected for the given strategy.

Suggested change

experimentalConfigOptions.experimentalOptions == null ||

experimentalConfigOptions.experimentalOptions === undefined ||

AtofStryker · 2023-07-27T22:09:16Z

cli/types/cypress.d.ts

@@ -2838,6 +2838,30 @@ declare namespace Cypress {
    certs: PEMCert[] | PFXCert[]
  }

+  type RetryStrategyWithModeSpecs = RetryStrategy & {


@ryanpei did you want retries to be false for the experimental release? At least for experimental, it feels like it has the same behavior as configuring nothing and having runMode and openMode both set to 0

@AtofStryker yes, I agree it should be default false.

@ryanpei I think what I am saying is setting it to false would break the API for experimental, but it essentially has the same effect as the default for the current API so it should be fine to leave it the way it is for experimental release

@AtofStryker I see, so if they set retries false, they won't see any errors or anything, right?

@ryanpei what I am saying is setting retries to false wouldn't be an option for experimental since it changes the API. To tweak retries, the user would need to provide openMode and runMode with booleans and provide an experimental strategy.

I see so this is a breaking change for anyone who has set retries: false? There's no way to fallback to the old API, in this case?

We can we would need to code for it though, which for an experimental release I am not sure of the value since it would be the same thing as the current default.

AtofStryker · 2023-07-27T22:09:49Z

packages/app/src/settings/project/Experiments.vue

+
+  // get experimental retry properties on the 'retries' config object. Mutate the experimentalConfigurations array as to not have side effects with props.gql.config
+  // TODO: remove this once experimentalRetries becomes GA. This is to be treated as a one off as supported nested experiments inside config is rare.
+  const { value: { experimentalStrategy, experimentalOptions, from } } = props.gql?.config.find((item) => item.field === 'retries')


this is the hack I was mentioning in the PR description. Figured its OK for a one off, but something we want to remove once the experiment is done.

AtofStryker · 2023-07-27T22:10:18Z

packages/config/src/validation.ts

+    }
+
+    // if the strategy is 'detect-flake-but-always-fail', stopIfAnyPassed must be provided and must be a boolean
+    if (strategy === 'detect-flake-but-always-fail') {


should I update these comments to include given a valid 'maxRetries' or is that implied from above?

AtofStryker · 2023-07-27T22:10:53Z

packages/config/src/validation.ts

  }

-  return errMsg(key, value, 'a positive number or null or an object with keys "openMode" and "runMode" with values of numbers or nulls')
+  return errMsg(key, value, 'a positive number or null or an object with keys "openMode" and "runMode" with values of numbers, booleans, or nulls, or experimental configuration with key "experimentalStrategy" with value "detect-flake-but-always-fail" or "detect-flake-and-pass-on-threshold" and key "experimentalOptions" to provide a valid configuration for your selected strategy')


@ryanpei I think I might need some suggestions on the messaging here, as this is really confusing 😅

Hm, yea I don't know how I would simplify this in this current format. I guess our options would be either:

Link to a doc instead of explaining this all in-line

Break up the error message into conditional parts, so that it isn't just one error message to cover all cases.

I think I lean towards the latter.

At least for now I'm kinda leaning on keeping the long error message and we can decide what we'd like to do as we iterate on this, but I think it's something that for sure needs to improve.

* chore: set up feature/test-burn-in feature branch * feat: add burnIn Configuration option (currently a no-op) (#27377) * 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: add new experimental retries configuration (#27412) * feat: implement the experimental retries configuration options to pair with test burn in * [run ci] * fix cache invalidation [run ci] * fix snapshot added in v13 for module api to include test burn in experimentalflag * chore: fix merge conflict * chore: add burnInTestAction capability (#27768) * add burnInTestAction capability * feat: add burn in capability for cloud * chore: fix snapshot for record_spec * feat: implement experimental retries (#27826) * 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 * Revert "feat: add burnIn Configuration option (currently a no-op) (#27377)" This reverts commit c428443. * Revert "chore: add burnInTestAction capability (#27768)" This reverts commit ae3df1a. * chore: run snapshot and binary jobs against experimental retries feature branch * chore: add changelog entry (wip) * Revert "fix snapshot added in v13 for module api to include test burn in experimentalflag" This reverts commit bb5046c. * Fix system tests * Clear CircleCI cache * Normalize retries config for test execution * Fixed some unit tests * update snapshots for newer test metadata * Fix cy-in-cy snapshots * update snapshots * bump cache version * chore: ensure legacy retry overrides work; reject exp. retries overrides (#28045) * update changelog * flip if statement in experimental retries option validation * refactor invalid experimental retry override for more useful error msg * revert testConfigOverrides snapshot * update snapshots for test override sys test * Update packages/config/src/validation.ts Co-authored-by: Chris Breiding <chrisbreiding@users.noreply.github.com> * succinct changelog entry; links to docs for details * testConfigOverride system test snapshots * Update .github/workflows/update_v8_snapshot_cache.yml Co-authored-by: Ryan Manuel <ryanm@cypress.io> * Update cli/CHANGELOG.md Co-authored-by: Ryan Manuel <ryanm@cypress.io> * Update packages/driver/src/cypress.ts Co-authored-by: Ryan Manuel <ryanm@cypress.io> * updating cache-version * improve typescript usage when appending experimental retry options to experiments in Experimenets.vue * Revert "improve typescript usage when appending experimental retry options to experiments in Experimenets.vue" This reverts commit b459aba. * refactor test config override validation for experimental retry subkeys * account for error throw differences in browsers in system tests * bump circle cache * bump circle cache again --------- Co-authored-by: astone123 <adams@cypress.io> Co-authored-by: mabela416 <mabel@cypress.io> Co-authored-by: Muaz Othman <muaz@cypress.io> Co-authored-by: Muaz Othman <muazweb@gmail.com> Co-authored-by: Cacie Prins <cacie@cypress.io> Co-authored-by: Cacie Prins <cacieprins@users.noreply.github.com> Co-authored-by: Chris Breiding <chrisbreiding@users.noreply.github.com> Co-authored-by: Ryan Manuel <ryanm@cypress.io> Co-authored-by: Matthew Schile <mschile@cypress.io>

AtofStryker added 2 commits July 27, 2023 15:04

feat: implement the experimental retries configuration options to pair

8afc296

with test burn in

[run ci]

b8a14e7

AtofStryker changed the title ~~Feat/add retries config~~ feat: add new experimental retries configuration Jul 27, 2023

AtofStryker marked this pull request as ready for review July 27, 2023 22:11

AtofStryker commented Jul 28, 2023

View reviewed changes

AtofStryker requested review from ryanpei and MuazOthman July 28, 2023 19:07

ryanpei approved these changes Aug 1, 2023

View reviewed changes

mabela416 approved these changes Aug 1, 2023

View reviewed changes

AtofStryker merged commit ef84f03 into feature/test-burn-in Aug 1, 2023

AtofStryker deleted the feat/add_retries_config branch August 1, 2023 21:23

mabela416 mentioned this pull request Sep 15, 2023

Set 250 as an upper limit for retries #27797

Closed

1 task

AtofStryker mentioned this pull request Sep 15, 2023

feat: implement experimental retries #27826

Merged

3 tasks

AtofStryker mentioned this pull request Sep 28, 2023

feat: experimental retries #27930

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add new experimental retries configuration #27412

feat: add new experimental retries configuration #27412

AtofStryker commented Jul 27, 2023 •

edited

Loading

cypress bot commented Jul 27, 2023 •

edited

Loading

AtofStryker Jul 27, 2023

AtofStryker Jul 27, 2023

ryanpei Jul 28, 2023

AtofStryker Jul 31, 2023

ryanpei Jul 31, 2023

AtofStryker Aug 1, 2023

ryanpei Aug 1, 2023

AtofStryker Aug 1, 2023

AtofStryker Jul 27, 2023

AtofStryker Jul 27, 2023

AtofStryker Jul 27, 2023

ryanpei Jul 28, 2023

AtofStryker Jul 31, 2023

	experimentalConfigOptions.experimentalOptions == null \|\|
	experimentalConfigOptions.experimentalOptions === undefined \|\|

feat: add new experimental retries configuration #27412

feat: add new experimental retries configuration #27412

Conversation

AtofStryker commented Jul 27, 2023 • edited Loading

Additional details

with experimentalStrategy and experimentalOptions enabled

with only experimentalStrategy enabled

with neither enabled

Steps to test

How has the user experience changed?

PR Tasks

cypress bot commented Jul 27, 2023 • edited Loading

38 flaky tests on run #49273 ↗︎

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AtofStryker commented Jul 27, 2023 •

edited

Loading

with `experimentalStrategy` and `experimentalOptions` enabled

with only `experimentalStrategy` enabled

cypress bot commented Jul 27, 2023 •

edited

Loading