Add unit tests for VirtualizedList render quirks #31401

NickGerleman · 2021-04-21T22:50:10Z

Summary

This change adds a series of snapshot tests to validate the render output of VirtualizedList in mixed scenarios. Jest timer mocks are used to measure rendering at different ticks. These test cases mostly center around realization logic, to help prevent regressions when chaning internal state representation.

Changelog

[Internal] [Added] - Add unit tests for VirtualizedList render quirks

Test Plan

Ran the added UTs locally.

This change adds a series of snapshot tests to validate the render output of VirtualizedList in mixed scenarios. Jest timer mocks are used to measure rendering at different ticks. These test cases mostly center around realization logic, to help prevent regressions when chaning internal state representation.

analysis-bot · 2021-04-22T00:45:53Z

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	9,111,276	-9,251
android	hermes	armeabi-v7a	8,639,877	-6,549
android	hermes	x86	9,550,303	-9,427
android	hermes	x86_64	9,516,739	-9,150
android	jsc	arm64-v8a	10,754,586	-8,389
android	jsc	armeabi-v7a	9,674,446	-5,683
android	jsc	x86	10,789,016	-8,560
android	jsc	x86_64	11,396,950	-8,288

Base commit: ebe939b

analysis-bot · 2021-04-22T02:46:37Z

Platform	Engine	Arch	Size (bytes)	Diff
ios	-	universal	n/a	--

Base commit: ebe939b

Libraries/Lists/__tests__/VirtualizedList-test.js

Depends on: facebook#31401 facebook#31420 VirtualizedList currently keeps a [first, last] range as state, tracking the region of cells to render. The render functions uses this as an input, along with a few special cases to render more (sticky headers, initial render region.) This change moves to instead keep state which describes discontiguous render regions. This mask is continually updated as the viewport changes, batch renders expand the region, etc. Special cases are baked into the render mask, with a relatively simple tranformation from the mask to render function. This representation makes it much easier to support keyboarding scenarios, which require keeping distinct regions (e.g. for last focused) realized while out of viewport. MSFT employees can see more here: https://msit.microsoftstream.com/video/fe01a1ff-0400-94b1-d4f1-f1eb924b1809

facebook-github-bot · 2021-04-27T00:27:05Z

@rozele has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

lunaleaps

This is amazing! Thank you for all the test cases here! It's been educational for me to review. I've left some comments/questions mostly about expectations of the test cases and ask if we could better clarify the expectations from the snapshots. For lowest effort, I think even a comment of expectation would be really helpful of what indices we expect to be rendered.

lunaleaps · 2021-04-28T17:25:51Z

Libraries/Lists/__tests__/VirtualizedList-test.js

@@ -104,6 +104,28 @@ describe('VirtualizedList', () => {
    expect(component).toMatchSnapshot();
  });

+  it('renders empty list after batch', () => {


I see we have a test above for rendering empty list, should that just be replaced by this one? Or should all our tests be calling onLayout?

The difference here is initial render vs later render batches. The first virtualizedlist render is special cased to not need layout information, compared to later renders which use layout information and excersise some different code paths.

Gotcha, small nit then, maybe we should rename that test case to initial render renders empty list.

lunaleaps · 2021-04-28T17:32:36Z

Libraries/Lists/__tests__/VirtualizedList-test.js

-      jest.runAllTimers();
+      simulateLayout(component, {
+        viewport: {width: 10, height: 50},
+        content: {width: 10, height: 200},


Why was the content height changed to 200 from 100?

It should be 100 here. Some of the tests are 10 items vs 20, which is why there is a size difference. I tried to do smaller tests where possible to make the snapshots more decipherable, but extra cells made some cases clearer.

lunaleaps · 2021-04-28T17:41:20Z

Libraries/Lists/__tests__/VirtualizedList-test.js

-      .filter(item => item.sticky)
-      .map(item => item.key);
-
+  it('keeps sticky headers above viewport visualized', () => {


Am I understanding correctly that sticky headers should stay rendered once they've been realized and even if they are no longer in the viewport?

Edit: Ah interesting the test case below unmounts sticky headers moved below viewport clarifies more. Hmm, do we have documentation about the virtualization behavior of this? If this is the only thing that addresses it, should we add more comments to explain the behavior?

The prop (exposed to ScrollView) is documented, but I'm not sure virtualization behavior is. I discovered the logic in-code, but its requirement makes sense.

If you have a sticky header, it will stay at top of viewport, even if its original layout position is scrolled out of viewport. That means layout-based logic would not be aware it is on screen, and would consider it eligible to be virtualized away (while on screen). Exempting sticky headers (even just the closest) above the viewport from virtualization prevents them from disappearing.

Oh yes that does make sense! Yea would be great to add this documentation to this test.

lunaleaps · 2021-04-28T17:43:02Z

Libraries/Lists/__tests__/VirtualizedList-test.js

-      });
-      jest.runAllTimers();
+      simulateScroll(component, {x: 0, y: 150});
+      performAllBatches();
    });

    expect(component).toMatchSnapshot();


Could we add more assertions here to be clear what we're looking for in these snapshots? I'm not exactly sure what I'm looking for 😅. Either a comment or a snapshot / inspection assertion would be really helpful! I realize we haven't done so in the past but I think it'll really help clarify expectations.

lunaleaps · 2021-04-28T17:51:05Z

Libraries/Lists/__tests__/VirtualizedList-test.js

+    simulateScroll(component, {x: 0, y: 150});
+    performAllBatches();
+  });
+


Should we verify here that the sticky indices in question are rendered?

The beginning part of this should be I think the equivalent of the above keeps sticky headers above viewport visualized

lunaleaps · 2021-04-28T17:53:06Z

Libraries/Lists/__tests__/VirtualizedList-test.js

+    performAllBatches();
+  });
+
+  expect(component).toMatchSnapshot();


When I look at the snapshot of this, I see all 20 MockCellItems rendered. Is that expected?

Good catch. A past iteration of this IIRC had windowSize={1} to force that we virtualize part of the list. It looks like this was dropped.

I will do a look through of all of the snapshots when adding comments describing expectations, to catch if there are other cases like this.

It looks like windowSize was replaced with window in a few places. Need to figure out what happened there.

Confirmed the snapshot is now correct, with the correct prop being passed from the test.

lunaleaps · 2021-04-28T17:54:51Z

Libraries/Lists/__tests__/VirtualizedList-test.js

+  expect(component).toMatchSnapshot();
+});
+
+it('renders offset cells in initial render when initialScrollIndex set', () => {


Could you clarify what the offset cells are here? And provide more assertions on the test?

lunaleaps · 2021-04-28T17:55:26Z

Libraries/Lists/__tests__/VirtualizedList-test.js

+  expect(component).toMatchSnapshot();
+});
+
+it('does not over-render when there is less than initialNumToRender cells', () => {


What does over-render mean?

Need to think about the language for this.

The intent is to test the case where we start at a non-zero initial scrollIndex, and need to render less than initialNumToRender.

Ah, so we want to ensure we don't render anything before the initialScrollIndex? Gotcha, again just a comment here might be helpful enough to get that context across.

NickGerleman · 2021-04-29T07:11:30Z

Will add some comments explaining expected behavior to everything

analysis-bot

Code analysis results:

eslint found some issues. Run yarn lint --fix to automatically fix problems.

…o add-virt-test-coverage

This merges in the changes from facebook/react-native#31401 into our repo, since they have not been merged upstream yet. These files can be set to be immutable copies of upstream code again once the changes are merged upstream.

lunaleaps · 2021-07-06T22:13:40Z

Libraries/Lists/__tests__/__snapshots__/VirtualizedList-test.js.snap

@@ -728,7 +862,7 @@ exports[`VirtualizedList keeps sticky headers realized after scrolled out of vie
    <View
      style={null}
    >
-      <item
+      <MockCellItem


This snapshot was described as:

// Scroll to the bottom 50 dip (last five items) of the content. Expect the
// last five items to be rendered, along with every sticky header above,
// even though they are out of the viewport window in layout coordinates.
// This is because they will remain rendered even once scrolled-past in
// layout space.

I see more than the last 5 items rendered here -- am I misunderstanding?

You're right. The description here isn't quite accurate. I can update it.

We always keep the initially rendered items present rendered, for the scroll to top optimization. Since initialNumToRender is set to 1, we keep the top cell rendered. windowSize is set to 1, instead of the default 21, which should theoretically mean "just keep the stuff in viewport rendered". This would mean five items, and I've observed that is what we do end up seeing when viewport is at the top, or some other positions. I've noticed we sometimes render more than that though, which I think may be related to either rounding, or directional render logic, that I have not explored. That is I think why we see more than 5 items at the end, even though the contract would be that windowSize of 1 would mean just the items in viewport.

Now that I am looking closer at this, we have a 50-height spacer where some sticky headers should be. We do specifically keep MockCellItem 6 rendered, even though there are two spacers after, but that implies we are only keeping the most recent sticky header above viewport realized, instead of all of them.

Updated the description to better match the current behavior

lunaleaps

Cool thanks @NickGerleman for digging deep into this! I think @rozele may need to re-import this

facebook-github-bot · 2021-07-20T15:50:28Z

@rozele has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2021-07-20T17:11:13Z

@rozele merged this pull request in cd78558.

Builds upon the `CellRenderMask` data structure added with facebook#31420, and VirtualizedList coverage added with facebook#31401. VirtualizedList currently keeps a [first, last] range as state, tracking the region of cells to render. The render functions uses this as an input, along with a few special cases to render more (sticky headers, initial render region.) This change moves to instead keep state which describes discontiguous render regions. This mask is continually updated as the viewport changes, batch renders expand the region, etc. Special cases are baked into the render mask, with a relatively simple tranformation from the mask to render function. This representation makes it much easier to support keyboarding scenarios, which require keeping distinct regions (e.g. for last focused) realized while out of viewport. MS/FB folks have a video discussion about VirtualizedList here: https://msit.microsoftstream.com/video/fe01a1ff-0400-94b1-d4f1-f1eb924b1809 facebook#31401 added quite a few snapshot tests, centering around the logic this change is touching. I manually validated RNTester FlatList examples (and their should be some upstream UI testing for them).

facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Microsoft Partner: Microsoft Partner labels Apr 21, 2021

NickGerleman commented Apr 23, 2021

View reviewed changes

Libraries/Lists/__tests__/VirtualizedList-test.js Show resolved Hide resolved

NickGerleman added 3 commits April 23, 2021 15:14

Fix a couple of tests

23fe28d

Add some more tests

7482c26

Fix faulty layout mocking

b343837

NickGerleman commented Apr 24, 2021

View reviewed changes

Libraries/Lists/__tests__/VirtualizedList-test.js Show resolved Hide resolved

renaming

9a8c55a

NickGerleman mentioned this pull request Apr 24, 2021

Shift VirtualizedList State Representation to CellRenderMask #31422

Closed

lunaleaps requested changes Apr 28, 2021

View reviewed changes

NickGerleman added 3 commits June 8, 2021 22:40

window->windowSize

9a49044

Update snapshots for window->windowSize

d5fc544

Add descriptions

9499dca

analysis-bot suggested changes Jul 1, 2021

View reviewed changes

NickGerleman added 2 commits July 1, 2021 08:08

Merge branch 'master' of https://github.com/facebook/react-native int…

bf9f98d

…o add-virt-test-coverage

run prettier

80a249e

NickGerleman mentioned this pull request Jul 2, 2021

Add new VirtualizedList Tests from RN Core PR microsoft/react-native-windows#8149

Merged

lunaleaps reviewed Jul 6, 2021

View reviewed changes

update description

68ef8e8

lunaleaps approved these changes Jul 7, 2021

View reviewed changes

halaei mentioned this pull request Jul 7, 2021

Optimize VirtualizedList #31327

Closed

facebook-github-bot closed this in cd78558 Jul 20, 2021

facebook-github-bot added the Merged This PR has been merged. label Jul 20, 2021

This was referenced Nov 21, 2021

Enable Generic Discontiguous Regions within VirtualizedList #32638

Closed

Do not virtualize items around the last focused item #32646

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add unit tests for VirtualizedList render quirks #31401

Add unit tests for VirtualizedList render quirks #31401

NickGerleman commented Apr 21, 2021

analysis-bot commented Apr 22, 2021 •

edited

Loading

analysis-bot commented Apr 22, 2021 •

edited

Loading

facebook-github-bot commented Apr 27, 2021

lunaleaps left a comment

lunaleaps Apr 28, 2021

NickGerleman Apr 28, 2021

lunaleaps Apr 29, 2021

lunaleaps Apr 28, 2021

NickGerleman Apr 28, 2021

lunaleaps Apr 28, 2021

NickGerleman Apr 29, 2021

lunaleaps Apr 29, 2021

lunaleaps Apr 28, 2021

lunaleaps Apr 28, 2021

NickGerleman Apr 29, 2021

lunaleaps Apr 28, 2021

NickGerleman Apr 29, 2021

NickGerleman Apr 29, 2021

NickGerleman Jul 1, 2021

lunaleaps Apr 28, 2021

lunaleaps Apr 28, 2021

NickGerleman Apr 29, 2021

lunaleaps Apr 29, 2021

NickGerleman commented Apr 29, 2021

analysis-bot left a comment

lunaleaps Jul 6, 2021

NickGerleman Jul 6, 2021

NickGerleman Jul 6, 2021

lunaleaps left a comment

facebook-github-bot commented Jul 20, 2021

facebook-github-bot commented Jul 20, 2021

Add unit tests for VirtualizedList render quirks #31401

Add unit tests for VirtualizedList render quirks #31401

Conversation

NickGerleman commented Apr 21, 2021

Summary

Changelog

Test Plan

analysis-bot commented Apr 22, 2021 • edited Loading

analysis-bot commented Apr 22, 2021 • edited Loading

facebook-github-bot commented Apr 27, 2021

lunaleaps left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickGerleman commented Apr 29, 2021

analysis-bot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lunaleaps left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Jul 20, 2021

facebook-github-bot commented Jul 20, 2021

analysis-bot commented Apr 22, 2021 •

edited

Loading

analysis-bot commented Apr 22, 2021 •

edited

Loading