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

test: Addressing launchpad test flake in Windows #22536

Merged
merged 4 commits into from
Jun 27, 2022

Conversation

tbiethman
Copy link
Contributor

@tbiethman tbiethman commented Jun 27, 2022

This PR addresses the consistently failing spec for the launchpad's global mode tests in Windows CI. See comments for details.

Steps to test

  • Open up the launchpad, navigate to a project, and select a testing type
  • Navigate back to the testing type selection
  • Select a testing type again (the same one or different, doesn't matter)
  • Inspect the project name link in the nav breadcrumbs and make sure there is no role attribute set on it (we want the implicit role on the a tag to take priority in this case)

How has the user experience changed?

PR Tasks

  • Have tests been added/updated?
  • [n/a] Has the original issue (or this PR, if no issue exists) been tagged with a release in ZenHub? (user-facing changes only)
  • [n/a] Has a PR for user-facing changes been opened in cypress-documentation?
  • [n/a] Have API changes been updated in the type definitions?

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Jun 27, 2022

Thanks for taking the time to open a PR!

@@ -0,0 +1,12 @@
<!DOCTYPE html>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I noticed a bunch of compilation errors around the failing spec due to this file missing from the todos project. todos is configured for component testing but did not include this file. I don't think this was causing a problem in the tests outside of the chatty log, but now it's set up correctly and the log is chatty no more.

@@ -152,7 +152,8 @@ describe('Launchpad: Global Mode', () => {

it('updates breadcrumb when selecting a project and navigating back', () => {
const getBreadcrumbLink = (name: string, options: { disabled: boolean } = { disabled: false }) => {
return cy.findByRole('link', { name }).should('have.attr', 'aria-disabled', options.disabled ? 'true' : 'false')
// The timeout is increased to account for variability in configuration load times in CI.
return cy.findByRole('link', { name, timeout: 10000 }).should('have.attr', 'aria-disabled', options.disabled ? 'true' : 'false')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In addition to the role lookup failing, the test would occasionally have to retry due to the config loading taking a bit longer than expected. So I just bumped the timeout here to give us plenty of room.

@cypress
Copy link

cypress bot commented Jun 27, 2022



Test summary

4377 0 49 0Flakiness 0


Run details

Project cypress
Status Passed
Commit c3b0858
Started Jun 27, 2022 6:58 PM
Ended Jun 27, 2022 7:10 PM
Duration 12:07 💡
OS Linux Debian - 10.11
Browser Electron 100

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

<a
:key="Boolean(hasLinkToCurrentProject).toString()"
Copy link
Contributor Author

@tbiethman tbiethman Jun 27, 2022

Choose a reason for hiding this comment

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

I noticed that the failure here was that findByRole couldn't find the breadcrumb links appropriately: https://dashboard.cypress.io/projects/sehy69/runs/12235/overview/83c7515d-deb1-414a-953c-b74c08865063

I could see in the recording that the links are enabled. And findByRole did find the 'Projects' and 'todos' elements, but it reported their role as...nothing?

  listitem:

  Name "":

  Name "":

  Name "":

  Name "":

  Name "":

---

  :                                 // <-- Wat

  Name "Projects":

  Name "todos":

---

  link:

  Name "v10.2.0":

I found this odd, so I took a look at those elements as rendered in the test and noticed that as we transitioned through the lifecycle, we would end up in a state where href/role attrs have gone from unset, to set, to unset again. However, once set, they would not be removed from the rendered element. So we would end up in a state where we have an a tag, with an href attribute, and a blank role set on it explicitly.

So long story short, I added a key to make sure these attributes we don't want to set actually do get removed. I tried using null rather than undefined in these cases to try and get these attributes to be completely removed from the same element instance, with no success.

And what do you know, this change fixed the test. As it turns out, our windows and linux builds are running these tests on different versions of Chrome. Windows is using Chrome 103 (the most recent build that it downloads onto machine at test time) vs. the linux build's Chrome 100 it uses from the docker image. This spec is actually failing in develop with Chrome 103, regardless of platform.

So even longer story short, correcting the issues with the blank role fixes this test and leaves us with a more correct DOM.

Copy link
Contributor

Choose a reason for hiding this comment

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

Excellent! And ugh. What a strange combination of events. I'm also real surprised that explicitly setting undefined which is also the natural value for the role makes a difference, as it didn't seem to make any difference to Chrome's accessibility tree. But hurray for getting these passing.

As a side note, I'm suggesting we just not have this link at all any more. If anybody has thoughts one way or the other.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like previous versions of chrome would leave the "link" value for the attribute, even when we unset it. This would allow the test to succeed, but it's not what we were wanting/expecting DOM-wise.

Chrome 103 gets a little closer and removes the role value from the element, but leaves the attribute just hanging there, like a boolean attribute: <div role>Wat</div>. This....is worse functionally but more indicative of what we're wanting, which is to unset the role.

I too would have figured that setting undefined would have cleared it, but I imagine this is some virtual dom weirdness where it's hard to interpret "no value" and "no attribute at all" through a single API.

@tbiethman tbiethman marked this pull request as ready for review June 27, 2022 18:40
@tbiethman tbiethman requested review from a team as code owners June 27, 2022 18:40
@tbiethman tbiethman requested review from emilyrohrbough and removed request for a team June 27, 2022 18:40
@rachelruderman
Copy link
Contributor

What a rockstar, thank you!! Do you mind filling out Steps to test? Given the comments I imagine in this case it's as simple as "Run packages/launchpad/cypress/e2e/global-mode.cy.ts on Chrome 103 and confirm it passes`, but would be great to have in the PR description for posterity

@tbiethman
Copy link
Contributor Author

@rachelruderman Steps to test have been added!

@tgriesser
Copy link
Member

Amazing

@tgriesser tgriesser self-requested a review June 27, 2022 19:16
Copy link
Member

@tgriesser tgriesser left a comment

Choose a reason for hiding this comment

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

🎉

@tbiethman tbiethman merged commit 24052eb into develop Jun 27, 2022
@tbiethman tbiethman deleted the tbiethman/windows-flake-6-27 branch June 27, 2022 19:24
tgriesser added a commit that referenced this pull request Jun 27, 2022
…esser/CLOUD-577-spec-list-display-latest-runs-batching

* muaz/CLOUD-577-spec-list-display-latest-runs:
  test: Addressing launchpad test flake in Windows (#22536)
  address comments from @marktnoonan
  Address code review comments
  followup on other comments
  re: @lmiller1990 PR comments
  chore(deps): update dependency semantic-release to v19 [security] (#22238)
  chore: Address skipped specs in server package (#22356)
  Address code review findings
  Address code review findings
  Empty-Commit to generate new percy nonce
  fix: handle case of implicit plugins/index.js files during migration (#22501)
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.

6 participants