virtual-scroll: add tests for fixed size items #11255

mmalerba · 2018-05-09T23:40:08Z

related to #10180

crisbeto · 2018-05-10T05:47:44Z

src/cdk-experimental/scrolling/virtual-scroll-viewport.spec.ts

+      flush();
+
+      expect((viewport._renderedContentTransform as any).changingThisBreaksApplicationSecurity)
+          .toEqual('translateY(NaNpx)');


You can use toBe here.

crisbeto · 2018-05-10T05:51:53Z

src/cdk-experimental/scrolling/virtual-scroll-viewport.spec.ts

+      finishInit(fixture);
+
+      expect(viewport.measureRangeSize({start: 1, end: 3}))
+          .toBe(testComponent.itemSize * 2, 'combined size of 2 50px items should be 100px');


The expectation could go out of sync if we change the itemSize. What about changing the expectation to use a hardcoded value?

It would also fail if we hard-coded the number and then changed the item size. I would prefer to use values like this over raw numbers when possible, otherwise you have no idea what the numbers are supposed to be when you come back to it in the future. (I had this experience when I refactored the select to work inside form-field. The test file was full of numbers that meant nothing to me).

crisbeto · 2018-05-10T05:55:33Z

src/cdk-experimental/scrolling/virtual-scroll-viewport.spec.ts

+          start: Math.floor(offset / testComponent.itemSize),
+          end: Math.floor(offset / testComponent.itemSize) + 4
+        };
+        expect(viewport.getRenderedRange()).toEqual(expectedRange);


Would it make sense to add some descriptions to the expectations? Since they're in a loop, it might be difficult to debug if things start failing.

crisbeto · 2018-05-10T05:56:00Z

src/cdk-experimental/scrolling/virtual-scroll-viewport.spec.ts

+          start: Math.floor(offset / testComponent.itemSize),
+          end: Math.floor(offset / testComponent.itemSize) + 4
+        };
+        expect(viewport.getRenderedRange()).toEqual(expectedRange);


Also consider adding some descriptions to the expectations here.

crisbeto · 2018-05-10T05:57:27Z

src/cdk-experimental/scrolling/virtual-scroll-viewport.spec.ts

+        fakeAsync(() => {
+          testComponent.bufferSize = 1;
+          finishInit(fixture);
+          triggerScroll(viewport, testComponent.itemSize * 2);


Changing the testComponent.itemSize to the actual value would make this one easier to follow. Currently anybody reading it has to go and look up the value of itemSize.

I disagree. When trying to understand the test, I think what the number represents conceptually is more important than what the actual value is

crisbeto · 2018-05-10T06:01:02Z

src/cdk-experimental/scrolling/virtual-scroll-viewport.spec.ts

+      </div>
+    </cdk-virtual-scroll-viewport>
+  `,
+  styles: [`.cdk-virtual-scroll-content-wrapper { display: flex; flex-direction: column; }`],


This seems like it could potentially be flaky when we run the tests against IE/Edge.

Why is that? IE supports flexbox AFAIK...

crisbeto · 2018-05-10T06:01:28Z

src/cdk-experimental/scrolling/virtual-scroll-viewport.spec.ts

+  @ViewChild(CdkVirtualScrollViewport) viewport: CdkVirtualScrollViewport;
+  @ViewChild(CdkVirtualForOf, {read: ViewContainerRef}) cdkForOfViewContainer: ViewContainerRef;
+
+  @Input() viewportSize = 200;


These aren't really inputs.

They're not inputs to the viewport, but they are inputs to my test component. I will want to create tests that check that the viewport handles re-rendering its content on size change once I add that feature

crisbeto · 2018-05-10T06:07:22Z

src/cdk-experimental/scrolling/virtual-scroll-viewport.spec.ts

+
+/** Finish initializing the virtual scroll component at the beginning of a test. */
+function finishInit(fixture: ComponentFixture<any>) {
+  // One the first cycle we render and measure the viewport.


one the first -> on the first

jelbourn

LGTM aside from @crisbeto's comments

crisbeto

LGTM once the CI is green.

crisbeto · 2018-05-10T16:17:45Z

src/cdk-experimental/scrolling/virtual-scroll-viewport.spec.ts

-      imports: [ScrollingModule],
-      declarations: [BasicViewport],
-    }).compileComponents();
+fdescribe('CdkVirtualScrollViewport', () => {


Forgotten fdescribe.

mmalerba · 2018-05-10T16:41:46Z

Fixed a small bug in the FixedSizeVirtualScrollStrategy found due to the tests

* feat(virtual-scroll): fixed size virtual scroll (#9316) * feat(virtual-scroll): fixed size virtual scroll * address some comments * change VirtualScrollStrategy interface a bit * address some more comments * fix lint & build * chore: move virtual-scroll to cdk-experimental (#9974) * virtual-scroll: simplify scroll listener logic (#10102) * virtual-scroll: only move views that need to be moved (#10099) * virtual-scroll: only move views that need to be moved * address comments * virtual-scroll: switch `throttleTime` to `sampleTime` (#10179) * virtual-scroll: switch throttleTime to sampleTime * add comment * virtual-scroll: allow user to pass `Observable<T[]>` (#10158) * virtual-scroll: rename `Range` to `ListRange` to avoid confusion with native `Range` (#10220) * virtual-scroll: add autosize scroll strategy (#10219) * rename fixed size virtual scroll directive * add autosize virtual scroll strategy * add item size estimator class * add logic for jumping rendered content based on scroll position * address comments * virtual-scroll: add `onContentRendered` hook to `VirtualScrollStrategy` (#10290) * virtual-scroll: add `onContentRendered` hook to `VirtualScrollStrategy` * address comemnts * virtual-scroll: add incremental scroll logic in `AutosizeVirtualScrollStrategy` (#10504) * virtual-scroll: add incremental scroll logic in `AutosizeVirtualScrollStrategy`. This still has a couple issues that need to be ironed out and it doesn't have the code for correcting the error between the predicted and actual scroll position. (See various TODOs for additional things that need work). * fix lint * address comments * address comments * fix bazel * fix devapp * fix lint * cleanup * virtual-scroll: rewrite offset in terms of "to-top" and fix a bug where items were removed too soon (#10986) * rewrite offsets to the end of the rendered content as offsets to the start * add some more autosize demos for testing * make sure not to remove too many items * address comments * virtual-scroll: address amcdnl's feedback (#10988) * rewrite offsets to the end of the rendered content as offsets to the start * add some more autosize demos for testing * make sure not to remove too many items * virtual-scroll: address amcdnl's feedback * virtual-scroll: fix updating when data changes and add trackBy demos (#11085) * virtual-scroll: add logic to correct the scroll position as user move… (#11137) * virtual-scroll: add logic to correct the scroll position as user moves toward the top * address comments * fix(scrolling): adds right to fix pushed content (#11192) * fix(scrolling): adds right to fix pushed content * chore: address feedback * chore: address pr feedback * virtual-scroll: add tests for fixed size items (#11255) * virtual-scroll: add tests for fixed size items * address comments * fix bug in fixed size strategy * fix bazel build * virtual-scroll: add tests for `cdkVirtualFor` logic (#11275) * merge fixed size test components into one * add tests for cdkVirtualFor logic * allow undefined to be explicitly passed as trackBy * fix bazel build * address comments * add some basic tests (#11295) * fix lint * virtual-scroll: add e2e tests for autosize scroll strategy (#11345) * set up virtual scroll page in e2e app * add gulp task for e2e:watch * add e2e tests for autosize * address comments * address comments * fix lint and tests * fix bad rebase * fix(scrolling): scrollbar jump when data change on scroll (#11193) * fix(scrolling): data change not updating size * chore: remove todo * chore: more performant fix for jump

angular-automatic-lock-bot · 2019-09-08T22:03:15Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

mmalerba added the pr: needs review label May 9, 2018

mmalerba assigned amcdnl, jelbourn and crisbeto May 9, 2018

mmalerba requested review from amcdnl, jelbourn and crisbeto May 9, 2018 23:40

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label May 9, 2018

mmalerba force-pushed the vs branch from d2bfe56 to b772184 Compare May 10, 2018 02:50

crisbeto reviewed May 10, 2018

View reviewed changes

virtual-scroll: add tests for fixed size items

4e79c4e

mmalerba force-pushed the vs branch from b772184 to 4e79c4e Compare May 10, 2018 15:52

jelbourn approved these changes May 10, 2018

View reviewed changes

address comments

c779f94

crisbeto approved these changes May 10, 2018

View reviewed changes

crisbeto added pr: lgtm and removed pr: needs review labels May 10, 2018

fix bug in fixed size strategy

615c94d

fix bazel build

3567825

mmalerba merged commit 5ad55da into angular:virtual-scroll May 10, 2018

angular-automatic-lock-bot bot unassigned amcdnl Sep 8, 2019

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 8, 2019

virtual-scroll: add tests for fixed size items #11255

virtual-scroll: add tests for fixed size items #11255

Uh oh!

Conversation

mmalerba commented May 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jelbourn left a comment

Choose a reason for hiding this comment

Uh oh!

crisbeto left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mmalerba commented May 10, 2018

Uh oh!

angular-automatic-lock-bot bot commented Sep 8, 2019

Uh oh!

Uh oh!