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

Fix navigation_spec "page load" flaky test. #6030

Closed
sainthkh opened this issue Dec 23, 2019 · 8 comments · Fixed by #6138
Closed

Fix navigation_spec "page load" flaky test. #6030

sainthkh opened this issue Dec 23, 2019 · 8 comments · Fixed by #6138
Labels
process: tests Related to our internal tests type: chore Work is required w/ no deliverable to end user

Comments

@sainthkh
Copy link
Contributor

Current behavior:

navigation_spec does not log "page load" events is really flaky.

Desired behavior:

It should always pass.

Steps to reproduce: (app code and test code)

It fails a lot on PR.

Versions

develop.

@jennifer-shehane
Copy link
Member

jennifer-shehane commented Dec 23, 2019

Is this test specifically https://github.com/cypress-io/cypress/blob/develop/packages/driver/test/cypress/integration/commands/navigation_spec.coffee#L391:L391

stack trac3

 1) src/cy/commands/navigation #go .log does not log 'Page Load' events:
     AssertionError: expected true to be false
      at getRet (cypress:///../driver/src/cy/commands/connectors.coffee:111:16)
      at tryCatcher (cypress:///./node_modules/bluebird/js/release/util.js:16:23)
      at Function.Promise.attempt.Promise.try (cypress:///./node_modules/bluebird/js/release/method.js:39:29)
      at Context.thenFn (cypress:///../driver/src/cy/commands/connectors.coffee:125:23)
      at Context.then (cypress:///../driver/src/cy/commands/connectors.coffee:492:21)
      at Context.eval (cypress:///../driver/src/cypress/cy.js:890:21)
      at eval (cypress:///../driver/src/cypress/cy.js:368:33)
      at tryCatcher (cypress:///./node_modules/bluebird/js/release/util.js:16:23)
      at Promise._settlePromiseFromHandler (cypress:///./node_modules/bluebird/js/release/promise.js:512:31)
      at Promise._settlePromise (cypress:///./node_modules/bluebird/js/release/promise.js:569:18)
      at Promise._settlePromiseCtx (cypress:///./node_modules/bluebird/js/release/promise.js:606:10)
      at Async._drainQueue (cypress:///./node_modules/bluebird/js/release/async.js:138:12)
      at Async._drainQueues (cypress:///./node_modules/bluebird/js/release/async.js:143:10)
      at Async.drainQueues (cypress:///./node_modules/bluebird/js/release/async.js:17:14)

@jennifer-shehane jennifer-shehane added the process: tests Related to our internal tests label Dec 23, 2019
@cypress-bot cypress-bot bot added the stage: ready for work The issue is reproducible and in scope label Dec 23, 2019
@jennifer-shehane jennifer-shehane added the type: chore Work is required w/ no deliverable to end user label Dec 23, 2019
@sainthkh
Copy link
Contributor Author

I learned that it's related to the execution order of events and promises. But needs to research more about how to fix it.

@sainthkh
Copy link
Contributor Author

sainthkh commented Jan 6, 2020

@jennifer-shehane

To pass this flaky test, 4 things should be done in the exact order below:

  1. go() content
  2. win.history.go()
  3. navigationChanged()
  4. then() content

These are made possible by Promise.delay(100). In that delay, step 2 and 3 are done so that step 4 should clear up things.

When the flaky test fails, the steps 2 and 3 aren't done in 100ms for some reason (it might be because of the Chrome scheduler's scheduling.).

I thought about using popstate event to check if go is done. But it cannot be a solution, because we need to show a command message when go process slows down (if you're moving from hash to hash, it can be done under 100ms, but when you need to load a new page, it's usually impossible).

So, I came to the conclusion that it's impossible to fix this flakiness completely. It's more of a feature than a bug.

However, I think there are some ways to suppress this problem.

  1. Increase the delay to 150 or 200. It might slow down the test a little, but it'll be less flaky because browser has more time to finish steps 2 and 3.
  2. Rerun the flaky test several times until it fails. Maybe, we need to wait for feat: support test retries #3968 to be merged.
  3. Maybe, use both 1 and 2.

For some reason, "page load" isn't flaky these days. Should we wait for #3968 and revisit later?

@kuceb
Copy link
Contributor

kuceb commented Jan 6, 2020

@sainthkh I can't reproduce the flake locally. Can you link to a CI run that exhibits this behavior?

@sainthkh
Copy link
Contributor Author

sainthkh commented Jan 7, 2020

Here it is: https://circleci.com/gh/cypress-io/cypress/221683#tests/containers/4.
And one more here: https://circleci.com/gh/cypress-io/cypress/220811#tests/containers/2

To forcefully reproduce it, change Promise.delay(100) to Promise.delay(1).

It'll always fail.

@kuceb
Copy link
Contributor

kuceb commented Jan 10, 2020

ok, I believe this will fix it #6138

@cypress-bot cypress-bot bot added stage: work in progress stage: needs review The PR code is done & tested, needs review and removed stage: ready for work The issue is reproducible and in scope stage: work in progress labels Jan 10, 2020
@cypress-bot cypress-bot bot added stage: pending release and removed stage: needs review The PR code is done & tested, needs review labels Jan 13, 2020
@cypress-bot
Copy link
Contributor

cypress-bot bot commented Jan 13, 2020

The code for this is done in cypress-io/cypress#6138, but has yet to be released.
We'll update this issue and reference the changelog when it's released.

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Jan 24, 2020

Released in 3.8.3.

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

@cypress-bot cypress-bot bot locked as resolved and limited conversation to collaborators Jan 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
process: tests Related to our internal tests type: chore Work is required w/ no deliverable to end user
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants