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(driver): fix integration test retry configuration #18643

Merged
merged 24 commits into from
Nov 9, 2021

Conversation

emilyrohrbough
Copy link
Member

Test flakiness in packages/driver has been observed and tests have not been retried as expected. This change enables the retry configuration in the integration tests via the cypress configuration.

@emilyrohrbough emilyrohrbough requested a review from a team as a code owner October 26, 2021 14:38
@emilyrohrbough emilyrohrbough requested review from jennifer-shehane and removed request for a team October 26, 2021 14:38
@cypress-bot
Copy link
Contributor

cypress-bot bot commented Oct 26, 2021

Thanks for taking the time to open a PR!

@cypress
Copy link

cypress bot commented Oct 26, 2021



Test summary

452 0 6 0Flakiness 0


Run details

Project cypress
Status Passed
Commit 11f5c14
Started Nov 9, 2021 9:31 PM
Ended Nov 9, 2021 9:40 PM
Duration 09:49 💡
OS Linux Debian - 10.9
Browser Electron 93

View run in Cypress Dashboard ➡️


This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

@emilyrohrbough emilyrohrbough marked this pull request as draft October 26, 2021 15:03
@jennifer-shehane jennifer-shehane removed their request for review October 26, 2021 15:09
@emilyrohrbough emilyrohrbough marked this pull request as ready for review October 28, 2021 15:31
mjhenkes
mjhenkes previously approved these changes Oct 29, 2021
Copy link
Contributor

@flotwig flotwig left a comment

Choose a reason for hiding this comment

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

I can't observe this actually retrying like it's supposed to. I see flaky driver tests still in CI. Locally, I tried to validate these changes by adding this spec to packages/driver and using yarn cypress:run:

let i = 0; let j = 0; let k = 0
const N = 2

it('fails twice then succeeds', () => {
  if (i++ < N) throw new Error(`failed ${i}`)
})

it('fails twice then succeeds', () => {
  if (j++ < N) throw new Error(`failed ${j}`)
})

it('fails twice then succeeds', () => {
  if (k++ < N) throw new Error(`failed ${k}`)
})

It does retry the first test one time with these changes, but it does not retry twice like it's supposed to:
image

Also, none of the subsequent test failures are retried.

Changing N to 1 makes the first test pass (since it retries once) but the next 2 still fail and are not retried: image

Comment on lines 3 to 11
let isActuallyInteractive

isActuallyInteractive = Cypress.config('isInteractive')
if (!isActuallyInteractive) {
// we want to only enable retries in runMode
// and because we set `isInteractive` above
// we have to set retries here
Cypress.config('retries', 2)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This was here as a workaround to the fact that we set isInteractive = true on line 19.

Cypress's getTestRetries switches on isInteractive to decide how many retries to use:

this.getTestRetries = function () {
const testRetries = this.config('retries')
if (_.isNumber(testRetries)) {
return testRetries
}
if (_.isObject(testRetries)) {
return testRetries[this.config('isInteractive') ? 'openMode' : 'runMode']
}
return null
}

Locally, it was returning 0 for me in this PR, which makes sense since this workaround was removed.

Comment on lines 11 to 14
},
"retries": {
"runMode": 2,
"openMode": 0
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this will work because of isInteractive being forced to true

@@ -163,6 +170,8 @@ describe('src/cy/commands/navigation', () => {
})

it('throws passing 2 invalid arguments', { defaultCommandTimeout: 200, retries: 1 }, (done) => {
expect(Cypress.config('retries')).to.eq(1)
Copy link
Contributor

Choose a reason for hiding this comment

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

testing the value of Cypress.getTestRetries() may be good to add somewhere

@emilyrohrbough emilyrohrbough marked this pull request as ready for review November 8, 2021 21:36
@emilyrohrbough
Copy link
Member Author

Looking at the failures / flakey tests listed in the last Cypress comment, retries have been enabled.

@@ -1,8 +1,7 @@
const { $ } = Cypress

let isActuallyInteractive
const isActuallyInteractive = Cypress.config('isInteractive')
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the change to enable retries

@@ -66,6 +66,12 @@ export const shouldBeCalledWithCount = (num) => (stub) => wrapped(stub).should('

export const shouldNotBeCalled = (stub) => wrapped(stub).should('not.be.called')

export const assertLogLength = (logs, expectedLength) => {
Copy link
Member Author

Choose a reason for hiding this comment

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

Helper to log the list of logs received to help debug when log length is different than expected.

Copy link
Contributor

@flotwig flotwig left a comment

Choose a reason for hiding this comment

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

My supplied test case from earlier works, but some stuff is still not retrying: https://app.circleci.com/pipelines/github/cypress-io/cypress/26345/workflows/825f5705-1c96-4ff5-b0d4-f43234e78612/jobs/994369/parallel-runs/1

There's no evidence of retrying on that failure in the logs.

You can observe this locally running yarn cypress:run --headed --no-exit, some tests will fail and not be retried, and some will be retried. Not sure what's happening, there's no per-test config in this file...

@emilyrohrbough
Copy link
Member Author

@flotwig You linked to the CircleCI logs. If you look in the Cypress Dashboard results you will see retries are working.

I had a similar concern about the logs not showing a retry & when testing locally with the following example, I was able to observe retries when running yarn cypress:run --headed --not-exit & mocha did not log information to indicate a retry had occurred.

Screen Shot 2021-11-09 at 11 44 35 AM

You can observe this locally running yarn cypress:run --headed --no-exit, some tests will fail and not be retried, and some will be retried. Not sure what's happening, there's no per-test config in this file...

I am currently running yarn workspace @packages/driver cypress:run --headed and have yet to observed this behavior , but there are a lot of tests here & haven't made it through the cypress/integration/commands/actions yet. It'll take while to get through them locally. Can you share which specs (or the video recording) where you observed this so I can dig in deeper?

@flotwig
Copy link
Contributor

flotwig commented Nov 9, 2021

@flotwig You linked to the CircleCI logs. If you look in the Cypress Dashboard results you will see retries are working.

Some tests are retrying, some tests aren't retrying. That test code does work now, but some real specs in assertions_spec are still not retrying.

You can observe this locally running yarn cypress:run --headed --no-exit, some tests will fail and not be retried, and some will be retried. Not sure what's happening, there's no per-test config in this file...

I am currently running yarn workspace @packages/driver cypress:run --headed and have yet to observed this behavior , but there are a lot of tests here & haven't made it through the cypress/integration/commands/actions yet. It'll take while to get through them locally. Can you share which specs (or the video recording) where you observed this so I can dig in deeper?

Observe the results of yarn cypress:run --headed --no-exit --spec **/assertions_spec.js (to run the spec with the test that's not retrying in CI):

Peek.2021-11-09.13-33.mp4

Earlier tests in the file do retry, so I'm not sure what causes this yet. But, it's not retrying.

@emilyrohrbough
Copy link
Member Author

@flotwig Thanks for the example. I'll take a look.

// user does an asynchronous `done`, so double-check
// that the test has not already passed before timing out
return
}
Copy link
Member Author

Choose a reason for hiding this comment

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

For anyone curious to reproduce this issue, you can comment this out and runt his test to consistently see the error shown in the video Zach linked:

      it('should not be true', { defaultCommandTimeout: 1 }, (done) => {
        cy.on('fail', (err) => {
          expect(err.message).to.eq('expected false to be true')

          done()
        })

        cy.noop(false).should('be.true')
      })

@chrisbreiding chrisbreiding changed the title test(driver): fix integration test retry configuration chore(driver): fix integration test retry configuration Nov 9, 2021
@chrisbreiding chrisbreiding merged commit a5cf5c0 into develop Nov 9, 2021
@chrisbreiding chrisbreiding deleted the fix-driver-retries branch November 9, 2021 21:16
tgriesser added a commit that referenced this pull request Nov 18, 2021
* develop: (329 commits)
  chore: Update Chrome (stable) to 96.0.4664.45 (#18931)
  fix: Loading of specs with % in the filename (#18877)
  chore: refactor `create` into class `$Cy` (#18715)
  chore: Update Chrome (beta) to 96.0.4664.45 (#18891)
  fix: flaky `system-tests-firefox` job (#18848)
  release 9.0.0
  feat: ensure major release
  have conduit app wait on localhost:3000
  fix install-required-node
  use --legacy-peer-deps
  feat: ensure major release
  fix darwin node install
  chore(driver): fix integration test retry configuration (#18643)
  feat(deps): update dependency electron to v15 🌟 (#18317)
  chore: Bind this correctly when setting response headers with cy.route() (#18859)
  feat: create config package for config validation (#18589)
  chore: patch `winston` to suppress `padLevels` warning (#18824)
  chore: test out major release build
  fix: remove outdated npm registry links (#18727)
  fix: Adding an existing command with `Cypress.Commands.add()` will throw an error (#18587)
  ...
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.

4 participants