-
Notifications
You must be signed in to change notification settings - Fork 54
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 a whole bunch of frontend test issues #3028
Conversation
@@ -41,6 +42,12 @@ const UsersSideNav: React.FC<Props> = ({ | |||
"padding-105 padding-right-2", | |||
activeUserId === user.id && "usa-current" | |||
)} | |||
aria-selected={activeUserId === user.id} | |||
aria-label={displayFullName( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just added these for use in tests
@@ -208,7 +208,7 @@ const AoEForm: React.FC<Props> = ({ | |||
</p> | |||
{emails.map((email) => ( | |||
<span | |||
key={"test-result-delivery-preference-email"} | |||
key={`test-result-delivery-preference-email-${email}`} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this ensures unique key
props to suppress console error
"warn", | ||
{ | ||
"vars": "all", | ||
"varsIgnorePattern": "^_", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooh, does this mean I can prefix unused variables with an underscore and the linter won't yell at me for it? 😁
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes!! 🎉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The patterns introduced here result in faster, less-fragile runs with no console output spam. This is a big win! 🎉 🚀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you so much for all your hard work on this one Nick!!
Kudos, SonarCloud Quality Gate passed! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! thank you for the hard work that went into this
Related Issue or Background Info
Changes Proposed
(Here is a link to view changes with whitespace and deleted files filtered out)
waitFor
andact
in our tests. The plugin suggests to never useact
in your tests because Testing Library's methods actually haveact
built in, and the warning appearing in your tests actually means that there was a side effect in your test that was not explicitly anticipated by your expectations. A lot of times this can be resolved by usingwaitForElementToBeRemoved
to explicitly wait for a loading screen to pass, or for a button to be re-enabled, for example. More info here: https://kentcdodds.com/blog/common-mistakes-with-react-testing-library#wrapping-things-in-act-unnecessarilyeslint --fix
is ran, meaning no more lint commits of shame!cleanup
in testsreact-test-renderer
to usetesting-library
insteadDOMException
fromQueueItem.test.tsx
Screenshots / Demos
Checklist for Author and Reviewer
UI
Testing
Changes are Backwards Compatible
(including re-granting permission to the no-PHI user if need be)
./gradlew liquibaseRollbackSQL
orliquibaseRollback
Security
Cloud
test
,dev
, orpentest
environment for verification