-
Notifications
You must be signed in to change notification settings - Fork 196
chore: Updates test pages and integ tests for compatibility with React 18 (1) #3832
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
Conversation
const [refreshCounter, setRefreshCounter] = useState(0); | ||
window.refreshItems = () => setRefreshCounter(prev => prev + 1); | ||
|
||
useEffectOnUpdate(() => { |
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 auto-refresh feature was not used in integration tests, but it was causing issues: in React 18 with strict mode the useEffectOnUpdate callback is triggered even when the dependencies did not change. As result, the table's state changed, causing some tests to fail.
<> | ||
<h1>Syncing horizontal scrolls</h1> | ||
<div | ||
onScroll={() => { |
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 onScroll is not reached in React 18 + strict-mode. The nested onScroll do get triggered, so I moved the related code to those handlers instead.
const page = new BasePageObject(browser); | ||
await page.setWindowSize({ width: 900, height: 800 }); | ||
await browser.url('#/light/table/tall-rows'); | ||
await page.waitForVisible(tableWrapper.findRows().toSelector()); |
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 is what we normally do in tests. Without this line the scroll on line 30 (page.windowScrollTo({ top: 200 })
) caused no effect when run in React 18, thus making the test assertion fail.
setupTest({ actionsMode: 'inline' }, async page => { | ||
await page.click('button[aria-label="Update item"]'); | ||
await expect(page.isFocused('button[aria-label="Update item"]')).resolves.toBe(true); | ||
await page.click('button[aria-label="Update item"]:not([disabled])'); |
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.
In the newly generated data for table with grid navigation the first active "Update item" action moved to the 2nd row, so I updated the selector to ignore the prev row's disabled action.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3832 +/- ##
=======================================
Coverage 97.10% 97.10%
=======================================
Files 839 839
Lines 24419 24419
Branches 8624 8625 +1
=======================================
Hits 23711 23711
+ Misses 701 658 -43
- Partials 7 50 +43 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
await page.keys(['ArrowRight', 'ArrowDown', 'ArrowRight', 'ArrowRight']); | ||
await expect(page.isFocused('tr[aria-rowindex="2"] button[aria-label="Update item"]')).resolves.toBe(true); | ||
await page.keys(['ArrowRight', 'ArrowDown', 'ArrowDown', 'ArrowRight', 'ArrowRight']); | ||
await expect(page.isFocused('tr[aria-rowindex="3"] button[aria-label="Update item"]')).resolves.toBe(true); |
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.
Same as above: small changes were required to account for the newly generated data.
Pull request was converted to draft
91402e0
to
92ea2dc
Compare
92ea2dc
to
c8685a6
Compare
|
||
import { id, Instance } from './generate-data'; | ||
|
||
export const allInstances50: Instance[] = [ |
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 is generated with generateItems(50). Having pre-generated items in test pages is better because we have tests that depends on specific item placements. These tests require adjustments every time we make changes to the order in which items are generated on the page.
Description
Some of our integration tests fail when run against React 18 (in strict-mode). This PR fixes some of those.
To run tests with React 18 use this setup: #3829
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
checkSafeUrl
function.Testing
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.