feat: first draft of anvil-cmg filter tests (#4068) #4071

jpaten · 2024-07-08T15:57:21Z

Ticket

Closes #4068 .

Reviewers

@NoopDog .

Changes

Added the following tests for AnVIL-CMG

Check that all filters defined in explorer/e2e/anvilTabs.js display on each tab, are clickable, and cause a checkbox to be displayed
Check that selecting the first 5 checkboxes on one arbitrary filter on the datasets tab does not remove any elements from the table
Check that selecting the first checkbox on an one arbitrary filter on the datasets causes that box to remain checked when each of the other tabs is selected
Check that the stated number of entries visible matches the number displayed next to the entry on the filter menu for an arbitrary but constant set of filters
Check that the blue filter bubbles match the filter settings and that clicking them deselects the filter for an arbitrary but constant set of filters
Check that the "Clear All" button functions correctly for an arbitrary but constant set of filters

MillenniumFalconMechanic · 2024-07-24T17:44:23Z

explorer/e2e/anvil/anvil-filters.spec.ts

+} from "../testFunctions";
+import { anvilFilters, anvilTabs, anvilTabTestOrder } from "./anvil-tabs";
+
+test.describe.configure({ mode: "parallel", timeout: 60 * 1000 });


Would you be able to tell me why you needed to add this here rather than in a global config file?

I made a mistake - that shouldn't have been necessary, I think I just copied the header of this file form another test where I had also forgotten to remove it. I'll remove this header from all the tests in #4078 , thanks for catching it here!

MillenniumFalconMechanic · 2024-07-24T17:45:52Z

explorer/e2e/anvil/anvil-filters.spec.ts

+  );
+});
+
+test.setTimeout(120000);


Could you give me the background on why we need the test.setTimeout calls?

Hi! When I ran these tests locally, I found that they would sometimes take too long and timeout. Adding the setTimeout calls here means the timeout for these particular tests is extended from 60 seconds to 120 seconds, which resolved the issue. I can also reduce the number of tests they run on if that is preferable. If this timeout value was changed in the global config, the timeout for all tests would be longer than necessary, causing tests to take longer if they fail.

MillenniumFalconMechanic · 2024-07-26T19:26:47Z

explorer/e2e/anvil/anvil-filters.spec.ts

+const filter_index_list = [3, 4, 5, 10, 6, 2];
+const filter_index_list_short = [1, 10, 3];


Would you be able to update these two to be MACRO_CASE to help indicate they are constants? Also, can you create constants for the integer values so it's more obvious what they are? For example:

const FILTER_ASSAY_INDEX = 3; const FILTER_GENDER_INDEX = 4; const FILTER_INDEX_LIST = [FILTER_ASSAY_INDEX, FILTER_GENDER_INDEX];

Will do! Do you think it makes sense to store those constants in this file or in anvilTabs.ts?

MillenniumFalconMechanic · 2024-07-26T19:29:33Z

explorer/e2e/anvil/anvil-filters.spec.ts

+    .click();
+  // Expect all checkboxes to be unchecked initially and to work properly
+  await expect(page.getByRole("checkbox").first()).toBeVisible();
+  const all_checkboxes = await page.getByRole("checkbox").all();


Could you update this variable to be camel-case (and also any other snake-case local variables throughout)?

MillenniumFalconMechanic · 2024-07-26T19:30:33Z

explorer/e2e/anvil/anvil-filters.spec.ts

+}) => {
+  await testFilterPersistence(
+    page,
+    anvilFilters[3],


Could you create a constant for 3 so help indicate its value?

MillenniumFalconMechanic · 2024-07-26T19:32:07Z

explorer/e2e/anvil/anvil-filters.spec.ts

+  testFilterPersistence,
+  testFilterPresence,
+} from "../testFunctions";
+import { anvilFilters, anvilTabs, anvilTabTestOrder } from "./anvil-tabs";


This is outside the scope of this PR but would you be able to update anvilFilters, anvilTabs, and anvilTabTestOrder to be MACRO_CASE to help indicate they are constants?

Yeah sure! I'll do it in the cleanup ticket

MillenniumFalconMechanic · 2024-07-26T19:33:22Z

explorer/e2e/anvil/anvil-filters.spec.ts

+    page,
+    anvilTabs.datasets,
+    filter_index_list.map((x) => anvilFilters[x]),
+    25


Let's create a constant for this default page size.

Based on how I've done other tests, it'd might make sense to have it as an attribute of each tab object (so each tab would have an associated maxPages), since I've tried to store constants that relate to the site there. I can make it a constant in this file though if you think that'd make more sense though, I've just tried to keep the constants there those that relate to the configuration of the actual test.

MillenniumFalconMechanic · 2024-07-26T19:36:41Z

explorer/e2e/testFunctions.ts

+
+export const getFirstElementTextLocator = (
+  page: Page,
+  workColumnPosition: number


Rename this to columnIndex? I'm not familiar with "work column" so feel free to keep it as is if this is a known concept.

Nope, just a random variable name I thought of at the time but that only made sense in my head, I'll make sure to review those in more detail for future PRs!

Can you go ahead and rename workColumnPosition to columnIndex, here and throughout?

MillenniumFalconMechanic · 2024-07-26T19:37:44Z

explorer/e2e/testFunctions.ts

+export async function testFilterPresence(
+  page: Page,
+  tab: TabDescription,
+  filters: string[]


Is this an array of filter names? If so, what about renaming this to filterNames?

Can you update this to filterNames, here and throughout? Can you also update the corresponding for (const filter of filters) to for (const filterName of filterNames)?

MillenniumFalconMechanic

Looks good, thanks Jonah! I've asked for a couple of changes. Once you've pushed your updates, I'll do another review round on testFunctions.

MillenniumFalconMechanic · 2024-08-13T19:14:43Z

explorer/e2e/anvil/anvil-filters.spec.ts

+  page,
+}) => {
+  test.setTimeout(120000);
+  await testFilterBubbles(


It looks like we call the bubbles "tags"; can you rename bubbles to "tags" here and throughout?

explorer/e2e/anvil/anvil-tabs.ts

MillenniumFalconMechanic · 2024-08-13T19:15:46Z

explorer/e2e/testFunctions.ts

+
+export const getFirstElementTextLocator = (
+  page: Page,
+  workColumnPosition: number


Can you go ahead and rename workColumnPosition to columnIndex, here and throughout?

MillenniumFalconMechanic · 2024-08-13T19:21:56Z

explorer/e2e/testFunctions.ts

 import { TabDescription } from "./testInterfaces";

 /* eslint-disable sonarjs/no-duplicate-string  -- ignoring duplicate strings here */
 // Run the "Expect each tab to appear as selected when the corresponding url is accessed" test
+
+export const getFirstElementTextLocator = (


It looks like this function is grabbing the first row in the table and then grabbing the text from the nth column; if so, can you rename this to getFirstRowNthColumnCellTextLocator or similar? It's a long name but I feel its gives a clearer idea of the function's intention.

MillenniumFalconMechanic · 2024-08-13T19:22:49Z

explorer/e2e/testFunctions.ts

@@ -207,4 +215,208 @@ export async function testPreSelectedColumns(
  }
 }

+export const filterRegex = (filter: string): RegExp =>


Can you add a comment describing the regex?

MillenniumFalconMechanic · 2024-08-13T19:23:56Z

explorer/e2e/testFunctions.ts

+export async function testFilterPresence(
+  page: Page,
+  tab: TabDescription,
+  filters: string[]


Can you update this to filterNames, here and throughout? Can you also update the corresponding for (const filter of filters) to for (const filterName of filterNames)?

MillenniumFalconMechanic · 2024-08-13T19:25:17Z

explorer/e2e/testFunctions.ts

+    /\.\S*/
+  );
+  if (filterNameMatch == null) {
+    test.fail();


Would it be possible to return a boolean here and call test.fail in the calling code? This matches the pattern in testFilterCount.

MillenniumFalconMechanic · 2024-08-13T19:28:16Z

explorer/e2e/testFunctions.ts

@@ -1,8 +1,22 @@
-import { expect, Page } from "@playwright/test";
+import { expect, Locator, Page, test } from "@playwright/test";
 import { TabDescription } from "./testInterfaces";

 /* eslint-disable sonarjs/no-duplicate-string  -- ignoring duplicate strings here */
 // Run the "Expect each tab to appear as selected when the corresponding url is accessed" test


Does this comment go with the function below? If so, can you remove the whitespace below?

MillenniumFalconMechanic · 2024-08-13T19:31:30Z

explorer/e2e/testFunctions.ts

 import { TabDescription } from "./testInterfaces";

 /* eslint-disable sonarjs/no-duplicate-string  -- ignoring duplicate strings here */
 // Run the "Expect each tab to appear as selected when the corresponding url is accessed" test
+
+export const getFirstElementTextLocator = (


Would you be able to add TSDocs for each function in this functions file? You can see examples of what I'm expecting here, here, or here.

MillenniumFalconMechanic

LGTM, thanks Jonah!

…nce test on webkit (#4068)

jpaten linked an issue Jul 8, 2024 that may be closed by this pull request

Add E2E Test for Filters #4068

Closed

github-actions bot added the canary Done by the Clever Canary team label Jul 8, 2024

jpaten force-pushed the 4068-add-e2e-test-for-filters branch 2 times, most recently from 78bf69e to ac63c9d Compare July 15, 2024 01:33

jpaten marked this pull request as ready for review July 15, 2024 04:32

jpaten requested a review from NoopDog July 15, 2024 04:37

jpaten assigned MillenniumFalconMechanic Jul 22, 2024

jpaten mentioned this pull request Jul 22, 2024

Jpaten/4076 add list of current and candidate tests #4077

Merged

MillenniumFalconMechanic self-requested a review July 22, 2024 20:47

jpaten mentioned this pull request Jul 22, 2024

test: cleanup playwright tests (#4078) #4079

Merged

MillenniumFalconMechanic reviewed Jul 24, 2024

View reviewed changes

MillenniumFalconMechanic reviewed Jul 26, 2024

View reviewed changes

jpaten mentioned this pull request Aug 1, 2024

test: create tests for datasets backpages (#4080) #4089

Merged

MillenniumFalconMechanic requested changes Aug 13, 2024

View reviewed changes

jpaten force-pushed the 4068-add-e2e-test-for-filters branch from 85752ed to cca06e0 Compare August 16, 2024 19:18

jpaten requested a review from MillenniumFalconMechanic August 16, 2024 19:19

MillenniumFalconMechanic approved these changes Aug 22, 2024

View reviewed changes

jpaten added 6 commits August 22, 2024 20:39

feat: first draft of anvil-cmg filter tests (#4068)

e217f20

fix: refactored and debugged filter tests (#4068)

ae2eddb

fix: added extra check to testFilterPresence (#4068)

185e331

fix: refactored tests (#4068)

90085d1

fix: refactored tests (#4068)

d23eeec

fix: refactored tests (#4068)

88c0c89

jpaten added 10 commits August 22, 2024 20:39

fix: attempted fix for filter test issues (#4068)

d753c76

fix: switched setchecked to click action (#4068)

3541949

fix: fixed typo that caused tests to fail, added clear all test (#4068)

89c30f5

fix: shortened certain tests that would timeout (#4068)

db8313f

fix: fixed redundant code and bad timeouts (#4068)

6771978

fix: made all variables camelcase in filter tests (#4068)

a18b866

fix: updated variable names and created constants (#4068)

a19a6df

fix: updated variable names, created constants, fixed bug in persiste…

1f8966f

…nce test on webkit (#4068)

fix: updated variable names and tsdocs for e2e tests (#4068)

31f9304

fix: fixed filter persistence test errors (#4068)

ffa69f5

jpaten force-pushed the 4068-add-e2e-test-for-filters branch from 6d4b8b4 to ffa69f5 Compare August 23, 2024 03:39

jpaten merged commit 7657375 into main Aug 23, 2024
4 checks passed

jpaten deleted the 4068-add-e2e-test-for-filters branch August 23, 2024 03:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: first draft of anvil-cmg filter tests (#4068) #4071

feat: first draft of anvil-cmg filter tests (#4068) #4071

jpaten commented Jul 8, 2024 •

edited

Loading

MillenniumFalconMechanic Jul 24, 2024

jpaten Jul 26, 2024

MillenniumFalconMechanic Jul 24, 2024

jpaten Jul 26, 2024

MillenniumFalconMechanic Jul 26, 2024

jpaten Jul 29, 2024

MillenniumFalconMechanic Jul 26, 2024

MillenniumFalconMechanic Jul 26, 2024

MillenniumFalconMechanic Jul 26, 2024

jpaten Jul 29, 2024

MillenniumFalconMechanic Jul 26, 2024

jpaten Jul 29, 2024

MillenniumFalconMechanic Jul 26, 2024

jpaten Jul 29, 2024

MillenniumFalconMechanic Aug 13, 2024

MillenniumFalconMechanic Jul 26, 2024

MillenniumFalconMechanic Aug 13, 2024

MillenniumFalconMechanic left a comment

MillenniumFalconMechanic Aug 13, 2024

MillenniumFalconMechanic Aug 13, 2024

MillenniumFalconMechanic Aug 13, 2024

MillenniumFalconMechanic Aug 13, 2024

MillenniumFalconMechanic Aug 13, 2024

MillenniumFalconMechanic Aug 13, 2024

MillenniumFalconMechanic Aug 13, 2024

MillenniumFalconMechanic Aug 13, 2024

MillenniumFalconMechanic left a comment

		const filter_index_list = [3, 4, 5, 10, 6, 2];
		const filter_index_list_short = [1, 10, 3];

feat: first draft of anvil-cmg filter tests (#4068) #4071

feat: first draft of anvil-cmg filter tests (#4068) #4071

Conversation

jpaten commented Jul 8, 2024 • edited Loading

Ticket

Reviewers

Changes

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

MillenniumFalconMechanic 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

MillenniumFalconMechanic left a comment

Choose a reason for hiding this comment

jpaten commented Jul 8, 2024 •

edited

Loading