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.

Sign up for GitHub

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
Merged

test: Addressing launchpad test flake in Windows #22536

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3a7f2a3
test: Bumping timeout on breadcrumb state checks for Windows
tbiethman Jun 27, 2022
36df684
todos project was missing component index but has component testing c…
tbiethman Jun 27, 2022
43a3390
Adding key to links to ensure attrs are cleaned up/reset
tbiethman Jun 27, 2022
c3b0858
Coercing boolean keys to strings to match vue key types
tbiethman Jun 27, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/frontend-shared/src/gql-components/HeaderBarContent.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,9 @@
<li class="inline-block">
<!-- context for use of aria role and disabled here: https://www.scottohara.me/blog/2021/05/28/disabled-links.html -->
<!-- the `href` given here is a fake one provided for the sake of assistive technology. no actual routing is happening. -->
<!-- the `key` is used to ensure the role/href attrs are added and removed appropriately from the element. -->
<a
:key="Boolean(hasLinkToProjects).toString()"
class="font-medium"
:class="hasLinkToProjects ? 'text-indigo-500 hocus-link-default' :
'text-gray-700'"
Expand All @@ -51,7 +53,9 @@
<li class="inline-block">
<!-- context for use of aria role and disabled here: https://www.scottohara.me/blog/2021/05/28/disabled-links.html -->
<!-- the `href` given here is a fake one provided for the sake of assistive technology. no actual routing is happening. -->
<!-- the `key` is used to ensure the role/href attrs are added and removed appropriately from the element. -->
<a
:key="Boolean(hasLinkToCurrentProject).toString()"
Copy link
Contributor Author

@tbiethman tbiethman Jun 27, 2022 •
edited
Loading

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.

class="font-medium"
:role="hasLinkToCurrentProject ? undefined : 'link'"
:href="hasLinkToCurrentProject ? 'choose-testing-type' : undefined"
Expand Down
3 changes: 2 additions & 1 deletion packages/launchpad/cypress/e2e/global-mode.cy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

}

const resetSpies = () => {
Expand Down
12 changes: 12 additions & 0 deletions system-tests/projects/todos/cypress/support/component-index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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.

<html>
<head>
<meta charset="utf-8">
<meta http-equiv="X-UA-Compatible" content="IE=edge">
<meta name="viewport" content="width=device-width,initial-scale=1.0">
<title>Components App</title>
</head>
<body>
<div data-cy-root></div>
</body>
</html>