test: add e2e tests to context menu #1571

ethanalvizo · 2023-10-12T06:27:40Z

Note: Wait for #1579 to merge first before merging this in

codecov · 2023-10-12T06:35:31Z

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (ef52749) 46.74% compared to head (9395443) 46.74%.

❗ Current head 9395443 differs from pull request most recent head f6b2ceb. Consider uploading reports for the commit f6b2ceb to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1571   +/-   ##
=======================================
  Coverage   46.74%   46.74%           
=======================================
  Files         583      583           
  Lines       36262    36262           
  Branches     9072     9072           
=======================================
  Hits        16951    16951           
  Misses      19259    19259           
  Partials       52       52

Flag	Coverage Δ
unit	`46.74% <83.33%> (ø)`

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

Files	Coverage Δ
packages/iris-grid/src/GotoRow.tsx	`58.87% <ø> (ø)`
packages/grid/src/Grid.tsx	`64.43% <87.50%> (ø)`
...kages/grid/src/key-handlers/SelectionKeyHandler.ts	`67.59% <75.00%> (ø)`

... and 12 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

mofojed

Not sure why the double context menu and advanced filter are failing... advanced filter looks like it may have gotten stuck to the bottom and then scrolled to the bottom of the grid somehow? Shouldn't happen if you're updating a filter, it should reset to the top...
And the double context menu, maybe just rebase off latest main?

packages/iris-grid/src/GotoRow.tsx

ethanalvizo · 2023-10-13T04:07:47Z

Not sure why the double context menu and advanced filter are failing... advanced filter looks like it may have gotten stuck to the bottom and then scrolled to the bottom of the grid somehow? Shouldn't happen if you're updating a filter, it should reset to the top... And the double context menu, maybe just rebase off latest main?

Yea it was pretty funky. There were two errors coming from the tests

Iris grid scrolled down to the bottom every once in a while -> I created a function that scrolls up in the IrisGrid to circumvent this
Console input couldn't be found when a context menu was open -> I made sure to click somewhere else in the UI to close the context menu before ending the test

dsmmcken · 2023-10-13T16:17:14Z

Iris grid scrolled down to the bottom every once in a while -> I created a function that scrolls up in the IrisGrid to circumvent this

#1477 ?

mofojed

Pull the changes out for the isStuckToBottom fix #1477 into a separate PR and merge that first.

packages/grid/src/Grid.tsx

packages/grid/src/key-handlers/SelectionKeyHandler.ts

tests/context-menu.spec.ts-snapshots/advanced-filters-2-chromium-linux.png

tests/context-menu.spec.ts-snapshots/advanced-filters-3-webkit-linux.png

Closes #1477 Previous comments are in #1571 PR. Note: Once this PR is merged #1571 can go in as well

Release notes https://github.com/deephaven/web-client-ui/releases/tag/v0.52.0 # [0.52.0](deephaven/web-client-ui@v0.51.0...v0.52.0) (2023-10-27) ### Bug Fixes * stuck to bottom on filter clear ([#1579](deephaven/web-client-ui#1579)) ([ef52749](deephaven/web-client-ui@ef52749)), closes [#1477](deephaven/web-client-ui#1477) [#1571](deephaven/web-client-ui#1571) [#1571](deephaven/web-client-ui#1571) * Theming - switched from ?inline to ?raw css imports ([#1600](deephaven/web-client-ui#1600)) ([f6d0874](deephaven/web-client-ui@f6d0874)), closes [#1599](deephaven/web-client-ui#1599) ### BREAKING CHANGES * Theme css imports were switched from `?inline` to `?raw`. Not likely that we have any consumers yet, but this would impact webpack config. Co-authored-by: deephaven-internal <[email protected]>

mofojed

Looks good overall, some suggestions for possible cleanup/renaming of methods.

mofojed · 2023-10-30T13:33:15Z

tests/context-menu.spec.ts

+async function openAdvancedFilters(page: Page) {
+  await page
+    .locator('.iris-grid .grid-wrapper')
+    .click({ button: 'right', position: { x: 20, y: 20 } });
+  await page.getByRole('button', { name: 'Advanced Filters' }).click();
+}


This method does open advanced filters, but it just seems to be doing just the first column of the first grid (e.g. you can't pass in a specific column or even a specific grid to open the advanced filter on). I'd rather be more specific with the name of the function (e.g. openFirstAdvancedFilters), or allow passing in the grid locator and an optional offset, e.g.:

/** * Open advanced filters for a grid. * @param gridLocator Locator to specify which grid to open the advanced filters on * @param xOffset Offset of the column to open advanced filters for. Defaults to the first column. */ async function openAdvancedFilters(gridLocator: Locator, xOffset = 20) ... }

mofojed · 2023-10-30T13:37:14Z

tests/context-menu.spec.ts

+async function moveMouseAwayFromTable(page: Page) {
+  await page.mouse.move(0, 0);
+  await page.mouse.click(0, 0);
+}


Similarly with this method - you're using it to move the mouse away from the table, but the actual function is just trying to reset the mouse position and focus. I'd probably do something like:

async function resetMousePosition(page: Page, resetFocus = true) { await page.mouse.move(0, 0); if (resetFocus) { await page.mouse.click(0, 0); } }

mofojed · 2023-10-30T13:39:38Z

tests/context-menu.spec.ts

+  await page.goto('');
+
+  const consoleInput = page.locator('.console-input');
+
+  const command = makeTableCommand(tableName, TableTypes.AllTypes);
+
+  await pasteInMonaco(consoleInput, command);
+  await page.keyboard.press('Enter');
+
+  // Wait for the panel to show
+  await expect(page.locator('.iris-grid-panel')).toHaveCount(1);
+
+  // Wait until it's done loading
+  await expect(page.locator('.iris-grid-panel .loading-spinner')).toHaveCount(
+    0
+  );
+
+  // Model is loaded, need to make sure table data is also loaded
+  await waitForLoadingDone(page);
+
+  const tableOperationsMenu = page.locator(
+    'data-testid=btn-iris-grid-settings-button-table'
+  );
+  await tableOperationsMenu.click();
+
+  // Wait for Table Options menu to show
+  await expect(page.locator('.table-sidebar')).toHaveCount(1);


This is exactly the same setup as in table-operations.spec.ts. We could refactor this into a utility method used by both, but I think in this case I would rather just move these table specific context menu tests to table-operations.spec.ts (since we're really testing the table operations here, we're just accessing them from the context menu).

ethanalvizo added 3 commits October 11, 2023 13:40

wip

7af3ee8

generated all linux snapshots

ff92198

test: add e2e test to context menu

4c4de6e

ethanalvizo self-assigned this Oct 12, 2023

mofojed requested changes Oct 12, 2023

View reviewed changes

packages/iris-grid/src/GotoRow.tsx Outdated Show resolved Hide resolved

packages/iris-grid/src/GotoRow.tsx Outdated Show resolved Hide resolved

ethanalvizo added 4 commits October 12, 2023 11:38

scroll to top of Iris Grid

5bccaec

fixed tests

d10e121

Merge branch 'main' into e2e-context-menu

df52f75

PR feedback

ea002fe

screenshot icon fix

976add3

ethanalvizo requested a review from mofojed October 13, 2023 04:42

ethanalvizo marked this pull request as ready for review October 13, 2023 04:42

ethanalvizo added 4 commits October 16, 2023 19:59

wip

ae2d664

wip

762b61e

Merge branch '1477-stuck-to-bottom' into e2e-context-menu

baea73d

remove scroll

0cc5db1

mofojed requested changes Oct 17, 2023

View reviewed changes

fix: stuck to bottom on filter clear

5fd344b

ethanalvizo mentioned this pull request Oct 18, 2023

fix: stuck to bottom on filter clear #1579

Merged

remove row hover

c0aca30

ethanalvizo requested a review from mofojed October 18, 2023 02:39

ethanalvizo added 6 commits October 20, 2023 01:19

PR feedback

9f224ef

update JSDocs

6c2b017

Merge branch '1477-stuck-to-bottom' into e2e-context-menu

f4d8672

fix failure

03ca9c7

Merge branch 'main' into e2e-context-menu

4674639

update snapshots for new theme

cec3046

ethanalvizo added 4 commits October 24, 2023 19:55

removed key check

9799e9f

Merge branch '1477-stuck-to-bottom' into e2e-context-menu

9395443

remove commented code

94af812

remove commented code

e42f72b

ethanalvizo added a commit that referenced this pull request Oct 27, 2023

fix: stuck to bottom on filter clear (#1579)

ef52749

Closes #1477 Previous comments are in #1571 PR. Note: Once this PR is merged #1571 can go in as well

ethanalvizo added 2 commits October 27, 2023 11:26

Merge branch '1477-stuck-to-bottom' into e2e-context-menu

390ddfe

Merge branch 'main' into e2e-context-menu

f6b2ceb

ethanalvizo enabled auto-merge (squash) October 27, 2023 18:29

mofojed mentioned this pull request Oct 30, 2023

Update web version 0.52.0 deephaven/deephaven-core#4736

Merged

mofojed approved these changes Oct 30, 2023

View reviewed changes

ethanalvizo merged commit e0bbdf0 into deephaven:main Oct 30, 2023
4 checks passed

github-actions bot locked and limited conversation to collaborators Oct 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: add e2e tests to context menu #1571

test: add e2e tests to context menu #1571

ethanalvizo commented Oct 12, 2023 •

edited

Loading

codecov bot commented Oct 12, 2023 •

edited

Loading

mofojed left a comment

ethanalvizo commented Oct 13, 2023

dsmmcken commented Oct 13, 2023

mofojed left a comment

mofojed left a comment

mofojed Oct 30, 2023

mofojed Oct 30, 2023

mofojed Oct 30, 2023

test: add e2e tests to context menu #1571

test: add e2e tests to context menu #1571

Conversation

ethanalvizo commented Oct 12, 2023 • edited Loading

codecov bot commented Oct 12, 2023 • edited Loading

Codecov Report

mofojed left a comment

Choose a reason for hiding this comment

ethanalvizo commented Oct 13, 2023

dsmmcken commented Oct 13, 2023

mofojed left a comment

Choose a reason for hiding this comment

mofojed left a comment

Choose a reason for hiding this comment

mofojed Oct 30, 2023

Choose a reason for hiding this comment

mofojed Oct 30, 2023

Choose a reason for hiding this comment

mofojed Oct 30, 2023

Choose a reason for hiding this comment

ethanalvizo commented Oct 12, 2023 •

edited

Loading

codecov bot commented Oct 12, 2023 •

edited

Loading