test: datagrid scrolling [INFENG-687] #9379

JComins000 · 2024-05-15T23:48:38Z

Ticket

Description

Turns out I needed to scroll the datagrid in order to view it's contents. Check the example in the test case before looking through the datagrid changes.

    await expect((await row.getCellByColumnName('Checkpoints')).pwLocator).toHaveText('0');

Test Plan

Checklist

Changes have been manually QA'd
User-facing API changes need the "User-facing API Change" label.
Release notes should be added as a separate file under docs/release-notes/.
See Release Note for details.
Licenses should be included for new code which was copied and/or modified from any external code.

netlify · 2024-05-15T23:48:58Z

✅ Deploy Preview for determined-ui ready!

Name	Link
🔨 Latest commit	`0cf9ed2`
🔍 Latest deploy log	https://app.netlify.com/sites/determined-ui/deploys/664d2551fcbf8e0008cea34a
😎 Deploy Preview	https://deploy-preview-9379--determined-ui.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

JComins000 · 2024-05-15T23:50:51Z

webui/react/src/e2e/tests/experimentList.spec.ts

@@ -52,6 +52,7 @@ test.describe('Experiement List', () => {
 const row = await projectDetailsPage.f_experiemntList.dataGrid.getRowByColumnValue('ID', '1');
 await row.clickColumn('Select');
 expect(await row.isSelected()).toBeTruthy();
+ await expect((await row.getCellByColumnName('Checkpoints')).pwLocator).toHaveText('0');


this would fail if the table isn't scrolled to the row. in the DOM, the column wouldn't exist.

codecov · 2024-05-15T23:51:31Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 42.88%. Comparing base (047580c) to head (0cf9ed2).
Report is 4 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9379      +/-   ##
==========================================
- Coverage   48.12%   42.88%   -5.24%     
==========================================
  Files        1219      909     -310     
  Lines      157884   118784   -39100     
  Branches     2731     2731              
==========================================
- Hits        75975    50946   -25029     
+ Misses      81734    67663   -14071     
  Partials      175      175

Flag	Coverage Δ
harness	`?`
web	`43.50% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

see 310 files with indirect coverage changes

djanicekpach

This does seem like a pain. I think it would be helpful if somewhere in the datagrid there was a summary of the algorithm we intend to use for scrolling.

djanicekpach · 2024-05-16T15:35:50Z

webui/react/src/e2e/models/components/F_ExperiementList.ts

@@ -26,7 +26,7 @@ export class F_ExperiementList extends BaseReactFragment {
 readonly pagination = new Pagination({ parent: this });
 }

-class ExperimentHeadRow extends HeadRow {}
+class ExperimentHeadRow extends HeadRow<ExperimentRow, ExperimentHeadRow> {}


can class ExperimentHeadRow extends HeadRow<ExperimentRow, ExperimentHeadRow> change to class ExperimentHeadRow extends HeadRow<ExperimentRow>? Having a class reference itself as a generic seems like a pretty strong code smell. There should be no reason for it.
Since the compiler already has the class type from the parent class here, we shouldn't need to specify it again. I think just removing the second generic class works but I can't pull to try it right now for unknown reasons.

i made it a little better. check again

djanicekpach · 2024-05-16T15:47:43Z

webui/react/src/e2e/models/hew/DataGrid.ts

+ 0,
+ );
+ // scrolling isn't waited on, and in this instance it's not guaranteed either. let's just wait a bit.
+ await page.waitForTimeout(3_000);


If at all possible we should find something else to wait on here. Maybe we could take a locator to expect to see as a parameter and we could wait visible? Maybe that checkColumnInView function could be passed as a parameter so we could parameterize the wait function.

well typically we'd want to wait on the columns to change, but the columns wont change if the last column is already in view

yeah the problem is this is too strict checkColumnInView

the best we could do is "wait for columns to change" but even that wouldn't be guaranteed. All we can do is wait for a change to "maybe" happen, and then check with

if (JSON.stringify(indexes) === JSON.stringify(prevIndexes)) { return false; }

im just gonna leave this here since i considered it, but at this moment i can't figure out what else I'd wait on anyway.

/** * Scolls the table * @param {object} obj * @param {number} [obj.xAbsolute] - The x coordinate. If not provided, the table will incrementally scroll to the right. * @param {Function} [obj.waitMethod] - The method we use to wait for the table to scroll. Default is to wait 3 seconds. */ private async scrollTable({ xAbsolute, waitMethod = async () => await this.root._page.waitForTimeout(3000) }: { xAbsolute?: number, waitMethod?: () => Promise<void> } = {}): Promise<void> { const box = await this.pwLocator.boundingBox(); if (box === null) { throw new Error('Expected to see a bounding box for the table.'); } const page = this.root._page; // move mouse to the center of the table await page.mouse.move((box.x + box.width) / 2, (box.y + box.height) / 2); // scroll the table await page.mouse.wheel( xAbsolute ? xAbsolute : box.width - 450, 0, ); await waitMethod(); }

djanicekpach · 2024-05-16T15:52:21Z

webui/react/src/e2e/models/hew/DataGrid.ts

+ await page.mouse.move((box.x + box.width) / 2, (box.y + box.height) / 2);
+ // scroll the table
+ await page.mouse.wheel(
+ 'xAbsolute' in args ? args.xAbsolute : Math.max(box.width - args.xRelative, 200),


The Math for the relative doesn't make sense to me. What is it relative to? The docs make it sound like the default scroll behavior is already relative to current position. It doesn't look like the we can ever scroll back(negative) with relative either.

i just wanted to scroll the table relative to the fixed left side. maybe i should just set increment=true and scroll ~400px

test: datagrid scrolling [INFENG-687]

690883a

JComins000 requested a review from a team as a code owner May 15, 2024 23:48

cla-bot bot added the cla-signed label May 15, 2024

JComins000 commented May 15, 2024

View reviewed changes

djanicekpach self-requested a review May 16, 2024 15:14

djanicekpach requested changes May 16, 2024

View reviewed changes

remove extra generic stuff

e308a78

JComins000 force-pushed the jcom/INFENG-687/datagrid-scrolling branch 2 times, most recently from 5f34030 to 06854b6 Compare May 21, 2024 17:13

djanicekpach approved these changes May 21, 2024

View reviewed changes

increment arg

756d302

JComins000 force-pushed the jcom/INFENG-687/datagrid-scrolling branch from 06854b6 to 756d302 Compare May 21, 2024 20:52

JComins000 enabled auto-merge (squash) May 21, 2024 21:15

JComins000 disabled auto-merge May 21, 2024 21:15

Merge branch 'main' into jcom/INFENG-687/datagrid-scrolling

e1b615a

JComins000 force-pushed the jcom/INFENG-687/datagrid-scrolling branch from c6b18c1 to e1b615a Compare May 21, 2024 21:35

JComins000 added 2 commits May 21, 2024 15:02

try another number

9404973

try again, maybe off by 1

0cf9ed2

JComins000 merged commit 0ef3e10 into main May 21, 2024
84 of 97 checks passed

JComins000 deleted the jcom/INFENG-687/datagrid-scrolling branch May 21, 2024 23:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: datagrid scrolling [INFENG-687] #9379

test: datagrid scrolling [INFENG-687] #9379

JComins000 commented May 15, 2024 •

edited

netlify bot commented May 15, 2024 •

edited

JComins000 May 15, 2024

codecov bot commented May 15, 2024 •

edited

djanicekpach left a comment

djanicekpach May 16, 2024

JComins000 May 19, 2024

djanicekpach May 16, 2024

JComins000 May 19, 2024

JComins000 May 20, 2024 •

edited

JComins000 May 20, 2024 •

edited

djanicekpach May 16, 2024

JComins000 May 19, 2024 •

edited

test: datagrid scrolling [INFENG-687] #9379

test: datagrid scrolling [INFENG-687] #9379

Conversation

JComins000 commented May 15, 2024 • edited

Ticket

Description

Test Plan

Checklist

netlify bot commented May 15, 2024 • edited

✅ Deploy Preview for determined-ui ready!

JComins000 May 15, 2024

Choose a reason for hiding this comment

codecov bot commented May 15, 2024 • edited

Codecov Report

djanicekpach left a comment

Choose a reason for hiding this comment

djanicekpach May 16, 2024

Choose a reason for hiding this comment

JComins000 May 19, 2024

Choose a reason for hiding this comment

djanicekpach May 16, 2024

Choose a reason for hiding this comment

JComins000 May 19, 2024

Choose a reason for hiding this comment

JComins000 May 20, 2024 • edited

Choose a reason for hiding this comment

JComins000 May 20, 2024 • edited

Choose a reason for hiding this comment

djanicekpach May 16, 2024

Choose a reason for hiding this comment

JComins000 May 19, 2024 • edited

Choose a reason for hiding this comment

JComins000 commented May 15, 2024 •

edited

netlify bot commented May 15, 2024 •

edited

codecov bot commented May 15, 2024 •

edited

JComins000 May 20, 2024 •

edited

JComins000 May 20, 2024 •

edited

JComins000 May 19, 2024 •

edited