Skip to content
This repository was archived by the owner on Oct 11, 2022. It is now read-only.

Conversation

@mxstbr
Copy link
Contributor

@mxstbr mxstbr commented May 29, 2018

Status

  • WIP
  • Ready for review
  • Needs testing

This pattern of "button-is-disabled then button-is-no-longer-disabled" broke pretty often in CI due to the race condition where the network would be faster than Cypress and the button would only be disabled for a split second.

Removing that pattern entirely should fix the e2e test flakiness we've been seeing.

Shoutout to @tadasant who discovered this in #3168, this is just another step further removing all instances of that pattern.

/cc @brianlovin for awareness

/cc @bahmutov maybe this would be worth adding to the docs as an anti-pattern?

This pattern of button-is-disabled button-is-no-longer-disabled broke
pretty often in CI due to the race condition where the network would be
faster than Cypress. Simply removing those fixes the e2e test flakiness.
@bahmutov
Copy link

Good point, this is something we should add to the "conditional testing" section https://docs.cypress.io/guides/core-concepts/conditional-testing.html#The-DOM-is-unstable

@brianlovin
Copy link
Contributor

Hm, thanks for the heads up - I can see why this would break. It's too bad, because this is helpful for us to test to make sure that we're properly guarding the button when mutations are in flight.

@brianlovin brianlovin merged commit 672a5ae into alpha May 29, 2018
@brianlovin brianlovin deleted the unflaky-e2e-tests branch May 29, 2018 15:42
@bahmutov
Copy link

bahmutov commented May 29, 2018

@brianlovin for this case when the button quickly appears based on XHR for example, you could set up separate test with delayed XHR request (using cy.route) and only test that the button appears, then disappears. But not test it in every test while testing some other feature.

We really discuss this case (with loaders that might sometimes quickly go disappear) when talking about flaky DOM, we really should describe this in our documentation

@brianlovin
Copy link
Contributor

Thanks for the note @bahmutov - makes sense, and hopefully this can make it in the documentation to help others! Appreciate it :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants