-
Notifications
You must be signed in to change notification settings - Fork 204
chore: Fixes some integ tests for react 18 #3947
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
base: main
Are you sure you want to change the base?
Conversation
7238690 to
6d0e92d
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3947 +/- ##
==========================================
+ Coverage 97.17% 97.20% +0.03%
==========================================
Files 854 858 +4
Lines 24983 25359 +376
Branches 8801 9014 +213
==========================================
+ Hits 24276 24650 +374
+ Misses 701 660 -41
- Partials 6 49 +43 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
40cf2ed to
92ffa9f
Compare
92ffa9f to
2727ad3
Compare
2727ad3 to
54d9251
Compare
| const [readyItems, setReadyItems] = useState(memoItems); | ||
| const [delayedItems, setReadyItems] = useState(memoItems); | ||
| const readyItems = settings.manualServerMock ? memoItems : delayedItems; | ||
| useEffect(() => { |
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 useEffect is firing an extra time during integ tests when run with React 18 + strict-mode, thus enforcing the loading state after the items are already resolved. The updated code avoids that effect as unnecessary as its only purpose is to imitate a server-side delay for manual tests.
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.
Would it help to merge variables, e.g, loading might not be necessary if it can be inferred from the presence of the ready items, and error and items could be merged in one single object?
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.
I din't quite understand that, could you please share a code example?
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.
For example:
const [response, setResponse] = useState({});
if (!settings.manualServerMock) {
setResponse({ isLoading: true, isError: false });
return getServerResponse(() => {
setResponse({
isLoading: false,
isError: settings.emulateServerError,
items: memoItems
});
});
}
in case setting three state variables independently was causing some problem.
54d9251 to
d155d92
Compare
| ); | ||
|
|
||
| (process.env.REACT_VERSION !== '18' ? test : test.skip)( | ||
| test( |
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 test is now working thanks to the changes to the test page.
| return marks.filter(m => m.detail?.source === 'awsui'); | ||
| let awsuiMarks: PerformanceMark[] = []; | ||
| await page.waitForAssertion(async () => { | ||
| await new Promise(r => setTimeout(r, 200)); |
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 timeout is likely causing issues. Increasing it three-fold reduces flakiness, but the waitForAssertion seems to do a better job.
| ); | ||
|
|
||
| (process.env.REACT_VERSION !== '18' ? test : test.skip)( | ||
| test( |
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 test was flaky, but I wasn't able to reproduce the failures when running it 100 times in a loop. I tried running it with visualRefresh=true and visualRefresh=false.
| expect(role).toEqual('region'); | ||
| expect(ariaLabelledby).not.toBeFalsy(); | ||
| await expect(page.getElementsText(`#${ariaLabelledby}`)).resolves.toEqual([tableHeading]); | ||
| await expect(page.getElementsText(`#${CSS.escape(ariaLabelledby)}`)).resolves.toEqual([tableHeading]); |
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.
When React 18+ is used, we use the React's useId() function to generate unique IDs (used for ARIA labelled-by). However, that function produces ids with colons (":") that cause an invalid selector error unless escaped.
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.
But if the element with the aria-labelledby attribute uses the ID without escaping, do they still match?
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, the matching works - only querying does not
| return marks.filter(m => m.detail?.source === 'awsui'); | ||
| let awsuiMarks: PerformanceMark[] = []; | ||
| await page.waitForAssertion(async () => { | ||
| await new Promise(r => setTimeout(r, 200)); |
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.
Do we need the timeout at all if we wrap the assertion in waitForAssertion?
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.
Good point - I will experiment with that
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.
I will move the timeout to the outside of the block, as the initial delay is expected
| await new Promise(r => setTimeout(r, 200)); | ||
| const marks = await browser.execute(() => performance.getEntriesByType('mark') as PerformanceMark[]); | ||
| awsuiMarks = marks.filter(m => m.detail?.source === 'awsui'); | ||
| if (awsuiMarks.length === 0) { |
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.
Minor: why not use expect instead?
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.
Good point - will updated 👍
| expect(role).toEqual('region'); | ||
| expect(ariaLabelledby).not.toBeFalsy(); | ||
| await expect(page.getElementsText(`#${ariaLabelledby}`)).resolves.toEqual([tableHeading]); | ||
| await expect(page.getElementsText(`#${CSS.escape(ariaLabelledby)}`)).resolves.toEqual([tableHeading]); |
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.
But if the element with the aria-labelledby attribute uses the ID without escaping, do they still match?
| // Imitate server-side delay when items update. | ||
| const memoItems = useEqualsMemo(collectionResult.items); | ||
| const [readyItems, setReadyItems] = useState(memoItems); | ||
| const [delayedItems, setReadyItems] = useState(memoItems); |
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.
Is it intentional that the name of these two variables don't match?
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.
No - will update 👍
| const [readyItems, setReadyItems] = useState(memoItems); | ||
| const [delayedItems, setReadyItems] = useState(memoItems); | ||
| const readyItems = settings.manualServerMock ? memoItems : delayedItems; | ||
| useEffect(() => { |
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.
Would it help to merge variables, e.g, loading might not be necessary if it can be inferred from the presence of the ready items, and error and items could be merged in one single object?
d155d92 to
d7b7a05
Compare
| const memoItems = useEqualsMemo(collectionResult.items); | ||
| const [readyItems, setReadyItems] = useState(memoItems); | ||
| const [delayedItems, setDelayedItems] = useState(memoItems); | ||
| const readyItems = settings.manualServerMock ? memoItems : delayedItems; |
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.
What is the difference between settings.manualServerMock and settings.useServerMock?
In the dev page, there only seems to be one related option ("Use server mock")
Description
The PR fixes some of the tests that were muted for React 18.
How has this been tested?
Review checklist
The following items are to be evaluated by the author(s) and the reviewer(s).
Correctness
CONTRIBUTING.md.CONTRIBUTING.md.Security
checkSafeUrlfunction.Testing
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.