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

Merged
merged 10 commits into from
Sep 27, 2023

Conversation

AtofStryker
Copy link
Contributor

@AtofStryker AtofStryker commented Sep 15, 2023

  • Closes N/A

Additional details

adds the implementation to support experimentalStrategy and experimentalOptions for the experimental retries configuration, which is to be used with test burn in. This adds two experimental strategies the user can leverage:

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

Each will have an experimentalOptions that can provide a maxRetries

The first option, detect-flake-and-pass-on-threshold, has a passesRequired option that means the test needs to pass a certain amount of times before marking the test as passed. If this cannot happen, the test is marked as failed

The second option, detect-flake-but-always-fail, has a stopIfAnyPassed option. By default, detect-flake-but-always-fail will run every retry in maxRetries if the test initially fails. This will short circuit if stopIfAnyPassed is true and a retry passes. Regardless of the retries test status (pass or fail), the test will always fail (assuming it fails on the first try)

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

Most of this PR is tests added to verify the behavior works correctly, so there are multiple ways to test this:

  • unit-test that runs as a cypress driver test to verify behavior of calculateTestStatus. This function is subject to change in the future and adding/editing tests in the driver to verify its behavior is likely the easiest way to go to verify changes

  • cy-in-cy tests, retries.experimentalRetries.mochaEvents.cy.ts and runner.experimentalRetries.mochaEvents.cy.ts inside the @packages/app are added for 3 sample projects that change the test status / attempts run. These are

    • detect-flake-and-pass-on-threshold with a maxRetries of 9 (10 attempts total, if applicable) and a passesRequired of 5
    • detect-flake-but-always-fail with a maxRetries of 9 (10 attempts total) and stopIfAnyPassed is false
    • detect-flake-but-always-fail with a maxRetries of 9 (10 attempts total, if applicable) and stopIfAnyPassed is true

    These projects run against tests in runner.experimentalRetries.mochaEvents.cy.ts in order to guarantee (see code comments to explain strategy outcomes inside test file):

    • before|beforeAll hooks that fail skip the test suite.
    • beforeEach hooks that fail on the max attempts allowed by the current test strategy still skip the test suite.
    • afterEach hooks that fail on the max attempts allowed by the current test strategy still skip the test suite.
    • after|afterAll hooks that fail skip the test suite.
    • verify the correct retry logic is applied on failures with only and no retry logic on initial passed test with only
    • passed tests do not retry

    Additionally, and more importantly, these projects run against tests in retries.experimentalRetries.mochaEvents.cy.ts in order to guarantee (see code comments to explain strategy outcomes inside test file):

    • expected number of attempts are executed based on experimentalStrategy and the correct test status is applied in the run.
    • number of attempts with pass/fail status and overall outer status captured in the mocha snapshots. I'd like to find a better way to verify/test this, but this seems like an acceptable current avenue to accomplish this
  • system-tests called experimental_retries to verify the output of the mocha reporter has been added to make sure correct pass/fail is reported for a given test strategy and the correct information is printed to the console. These run in chrome, firefox, and electron currently.

What eventually needs to be tested

  • test outer status sent to the cloud inclusive of all attempts for burn in to make sure the payload looks correct. Right now our source of truth for this is the lib/reporter.js in @package/server
  • specific UI testing to verify attempt pass/fail. This is handled currently in the cy-in-cy tests but mocha snapshots are extremely difficult to comb through and understand what went wrong / why.

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

What is NOT in this PR

TestConfigOverrides

Currently, testConfigOverrides for retries on the test level is not tested or currently supported with the experimental retry options. The goal eventually will be to opt-out users from the experiment if the GA retries configuration is provided in the test, like this:

it('test', { retries: 1 }, () => // opt-out of experimental config and use regular test retries)

Refactors to implementation

The implementation of the experimental retries options lives in 8a4526a. There is likely code here that can be cleaned up and simplified because the current implementation is some what invasive as we are

  • patching the mocha package in the driver to support retries on passes, though the patch is small
  • handling exception cases in runner.ts to make sure retries are stopped/queued appropriately
  • adding a method on the test runnable itself, called calculateTestStatus inside src/cypress/mocha which returns an object to the mocha package to determine whether or not to requeue the test
  • a new field, called _cypressTestStatusInfo, which is used by both the cypress reporter in console and in the browser, in order to correctly display the outer test status.

Know bugs

Currently there is an issue with the server console reporter reporting the test status too early before an afterEach hook has had a chance to run and report pass/fail. This likely has to do with us emitting the pass/fail event too early to the server & on the retry itself, meaning we need to emit that later once we have a more full picture.

Here is an example of this bug, where the attempt is logged twice, when it should really be printed once, and printing the status twice, which changes the expected status code in the system test

This issue should be resolved in 072805c

Risk

Likely the biggest thing to address here is possible risk, as the new implementation is applied globally throughout cypress, whether or not it is used. Currently, if no strategy is provided, we treat the config as detect-flake-and-pass-on-threshold up to the retry limit with passesRequired being 1. There is a lot being touched here, but if we are able to get a green build without altering the exterior implementation, then I think we can confidently say this is non breaking (which it should not be breaking)

Reviewing

This is a large change, where the implementation changes are massive but can have a massive downstream impact to the code. To make this easier to review, the commits are broken up as follows:

  • implementation 8a4526a
  • updates to existing snapshots/tests to ensure backwards capabilities 70d0172
  • added cy-in-cy tests to guarantee new functionality 89426e3
  • added unit tests to exercise calculateTestStatus function c69d58b
  • added system tests to verify reporter output 9abc367
  • fixed bugs with aftereach console output into server reporter 072805c
  • make sure hook failures print correct last attempt status 813c9ce

PR Tasks

@AtofStryker AtofStryker changed the title Feat/experimental retries impl feat: implement experimental retries Sep 15, 2023
@cypress
Copy link

cypress bot commented Sep 15, 2023

1 flaky test on run #51314 ↗︎

0 217 21 0 Flakiness 1

Details:

chore: improve types within calculateTestStatus inside mocha.ts
Project: cypress Commit: 6343ae9c30
Status: Passed Duration: 09:32 💡
Started: Sep 27, 2023 6:14 PM Ended: Sep 27, 2023 6:23 PM
Flakiness  cypress/e2e/project-setup.cy.ts • 1 flaky test • launchpad-e2e

View Output Video

Test Artifacts
... > can skip setup CT testing for an E2E project Test Replay Output Screenshots

Review all test suite changes for PR #27826 ↗︎

@AtofStryker AtofStryker force-pushed the feat/experimental_retries_impl branch from b33b83d to 01de7e3 Compare September 18, 2023 20:24
@AtofStryker AtofStryker marked this pull request as ready for review September 18, 2023 20:37
@AtofStryker AtofStryker force-pushed the feat/experimental_retries_impl branch from 01de7e3 to 2ab81bc Compare September 18, 2023 20:37
@AtofStryker AtofStryker force-pushed the feat/experimental_retries_impl branch from 2ab81bc to f76fdb3 Compare September 19, 2023 19:39
@AtofStryker
Copy link
Contributor Author

@chrisbreiding I was able to rebase the PR with comment updates to clarify test vs test attempt. Let me know if something can be clearer or is confusing!

@AtofStryker
Copy link
Contributor Author

AtofStryker commented Sep 20, 2023

@chrisbreiding @cacieprins I think I fixed the afterEach bug mentioned in the PR description in 072805c. If we are in a case where the outer status has failed but the test attempt passed, we want to delay signaling that the test passed because a hook might fail someplace in the suite. snapshots are all updated and look good, though I still think the exit code should be an 8 and not a 9 on the system tests, but this looks to be another issue

@MuazOthman MuazOthman changed the title feat: implement experimental retries feat: implement experimental retries CYCLOUD-1139 Sep 21, 2023
@MuazOthman MuazOthman changed the title feat: implement experimental retries CYCLOUD-1139 feat: implement experimental retries Sep 21, 2023
@AtofStryker AtofStryker force-pushed the feat/experimental_retries_impl branch from 924e2de to ecd339b Compare September 22, 2023 20:09
packages/driver/src/cypress/mocha.ts Outdated Show resolved Hide resolved
packages/driver/src/cypress/mocha.ts Outdated Show resolved Hide resolved
packages/driver/src/cypress/runner.ts Outdated Show resolved Hide resolved
packages/driver/src/cypress/runner.ts Outdated Show resolved Hide resolved
packages/reporter/src/test/test-model.ts Show resolved Hide resolved
@AtofStryker
Copy link
Contributor Author

AtofStryker commented Sep 25, 2023

@chrisbreiding @cacieprins I pushed a fixed up in 89a21a9 that fixes the server reporter to correctly print attempts on hook failures. The system tests take the runner ui cy-in-cy tests and run the m in a system test to verify output. There are some bugs in the system test snapshot, like NaNms in the output, and some of the semantics need to be updated, such as the server reporter assuming the runnable is a test (it could be a hook as well), but think we should hold off on including those updates here.

@AtofStryker AtofStryker force-pushed the feat/experimental_retries_impl branch from 63b6be7 to 89a21a9 Compare September 25, 2023 18:44
…-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.
… 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.
…ests, 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.
…cha is decorated/handled properly in calculateTestStatus
…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.
…opriate spot in runner.ts and not prematurely, which in turn updates the snapshots for cy-in-cy as the fail event comes later."
@AtofStryker AtofStryker force-pushed the feat/experimental_retries_impl branch from 1af7ed3 to 6343ae9 Compare September 27, 2023 16:46
@AtofStryker AtofStryker merged commit a1ad9ca into feature/test-burn-in Sep 27, 2023
2 of 5 checks passed
@AtofStryker AtofStryker deleted the feat/experimental_retries_impl branch September 27, 2023 20:09
@AtofStryker AtofStryker mentioned this pull request Sep 28, 2023
3 tasks
@MuazOthman MuazOthman changed the title feat: implement experimental retries feat: implement experimental retries CYCLOUD-1139 Sep 29, 2023
@MuazOthman MuazOthman changed the title feat: implement experimental retries CYCLOUD-1139 feat: implement experimental retries Sep 29, 2023
cacieprins added a commit that referenced this pull request Oct 26, 2023
* 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 <[email protected]>

* 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 <[email protected]>

* Update cli/CHANGELOG.md

Co-authored-by: Ryan Manuel <[email protected]>

* Update packages/driver/src/cypress.ts

Co-authored-by: Ryan Manuel <[email protected]>

* 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 <[email protected]>
Co-authored-by: mabela416 <[email protected]>
Co-authored-by: Muaz Othman <[email protected]>
Co-authored-by: Muaz Othman <[email protected]>
Co-authored-by: Cacie Prins <[email protected]>
Co-authored-by: Cacie Prins <[email protected]>
Co-authored-by: Chris Breiding <[email protected]>
Co-authored-by: Ryan Manuel <[email protected]>
Co-authored-by: Matthew Schile <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants