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: [M3-7725] - Unit tests button enabled assertions #10142

Merged
merged 3 commits into from
Feb 6, 2024

Conversation

abailly-akamai
Copy link
Contributor

@abailly-akamai abailly-akamai commented Feb 5, 2024

Description πŸ“

Since we changed our buttons to feature an aria tag VS using a disabled attribute, both toBeEnabled & toBeDisabled can lead to false positives.

The reason is, the jest-dom assertion for toBeEnabled will look for a disabled attribute which isn't going to be present since we've altered our Button to feature an aria-disabled tag instead, leading to a false positive.

We've applied the fix for our e2e but the unit suite configuration needed to be handled as well.

Changes πŸ”„

  • Add an interceptor for toBeEnabled & toBeDisabled assertions (for buttons only) that checks for aria-disabled tags as well

How to test πŸ§ͺ

Reproduction steps

  • An easy to reproduce the initial issue is to use an existing assertion such as the CloseAccountSetting component locally (on develop branch)
    • hardcode this value to true
    • run yarn test CloseAccountSetting.test.tsx - this assertion will still pass, which is a false positive

Verification steps

  • Pull code locally and reproduce the steps above, make sure the assertion now fails
  • run yarn test and Make sure the whole suite runs

As an Author I have considered πŸ€”

Check all that apply

  • πŸ‘€ Doing a self review
  • ❔ Our contribution guidelines
  • 🀏 Splitting feature into small PRs
  • βž• Adding a changeset
  • πŸ§ͺ Providing/Improving test coverage
  • πŸ” Removing all sensitive information from the code and PR description
  • 🚩 Using a feature flag to protect the release
  • πŸ‘£ Providing comprehensive reproduction steps
  • πŸ“‘ Providing or updating our documentation
  • πŸ•› Scheduling a pair reviewing session
  • πŸ“± Providing mobile support
  • β™Ώ Providing accessibility support

@abailly-akamai abailly-akamai self-assigned this Feb 5, 2024
@abailly-akamai abailly-akamai marked this pull request as ready for review February 5, 2024 18:46
@abailly-akamai abailly-akamai requested a review from a team as a code owner February 5, 2024 18:46
@abailly-akamai abailly-akamai requested review from jdamore-linode and hana-linode and removed request for a team February 5, 2024 18:46
Copy link

github-actions bot commented Feb 5, 2024

Coverage Report: βœ…
Base Coverage: 81.15%
Current Coverage: 81.15%

@jdamore-linode
Copy link
Contributor

The PR that originally changed the button disable behavior (#10028) also included a bunch of unit test changes to check explicitly for aria-disabled -- should we reevaluate those unit test changes in light of this improvement?

@abailly-akamai
Copy link
Contributor Author

@jdamore-linode I don't think it's too important? checking for aria-disabled is still a proper/valid assertion so i don't have any qualms leaving them there. I also don't think there's a performance impact.

This PR really just aims to future-proof new disabled assertions.

Copy link
Contributor

@jdamore-linode jdamore-linode left a comment

Choose a reason for hiding this comment

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

This is great, thanks @abailly-akamai!

Copy link
Contributor

@hana-linode hana-linode left a comment

Choose a reason for hiding this comment

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

The ImageAndPassword unit test failed when I ran yarn test but passed when I ran it by itself locally. Think it might have been a false alarm

Screenshot 2024-02-05 at 4 02 09 PM

image

@abailly-akamai abailly-akamai merged commit 460012d into linode:develop Feb 6, 2024
18 checks passed
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.

None yet

3 participants