[no ticket][risk=no] Fix e2e genomic-extraction-to-vcf test: Update waitForComplete function #6532

aweng98 · 2022-04-05T18:33:48Z

Checking for whether the genomic extraction job has finished was relying on existences of Success icon in Genomic Extraction History table. But, if job has failed, test just waits until test timed out because test code only look for the Success icon.

PR checklist

aweng98 · 2022-04-06T17:51:42Z

e2e/app/component/table.ts

-          break;
-        }
-      }
+  async findCellByRowValue(searchColumnHeader, searchCellValue, resultColumnHeader): Promise<Cell | null> {


This func did not work for the History table. So I had to fix this. Some changed files in this PR are due to this func change.

aweng98 · 2022-04-06T17:55:47Z

e2e/tests/nightly/genomic-extraction-to-vcf.spec.ts

  });

-  // Check creation status.
-  async function waitForComplete(page: Page, maxTime: number): Promise<boolean> {


waitForComplete() is just too big. Broke it down into two smaller functions: waitForRuntimeComplete() and waitForExtractionComplete.

aweng98 · 2022-04-06T17:58:31Z

e2e/app/sidebar/genomic-extractions-sidebar.ts

+  private async getStatusSpinnerXpath(datasetName: string): Promise<string> {
+    const historyTable = this.getHistoryTable();
+    await historyTable.waitUntilVisible();
+    const statusCell = await historyTable.findCellByRowValue('DATASET NAME', datasetName, 'STATUS');


Since the Extraction History table could contain more than 1 row (from running few times in same day), look for the spinner icon in STATUS column in the same row that contains the dataset name, not look for it in the whole table.

calbach

Thanks, mostly minor style comments

calbach · 2022-04-07T23:18:16Z

e2e/app/component/table.ts

+    // Find the searchColumnHeader index
+    const allColumns = await this.getColumns();
+    const colValues = await Promise.all(allColumns.map((column) => getPropValue<string>(column, 'innerText')));
+    console.log({ colValues });


Looks like leftover debug statements, would remove

calbach · 2022-04-07T23:23:08Z

e2e/app/component/table.ts

-          break;
-        }
-      }
+  async findCellByRowValue(searchColumnHeader, searchCellValue, resultColumnHeader): Promise<Cell | null> {


Please add types to the parameters, it makes it easier to follow

Added types to parameters. Added comments to explain the function. Removed console.log.

calbach · 2022-04-07T23:26:01Z

e2e/app/component/table.ts

-          break;
-        }
-      }
+  async findCellByRowValue(searchColumnHeader, searchCellValue, resultColumnHeader): Promise<Cell | null> {


Please add method documentation of what this is supposed to do - the semantics are not obvious from the method name.

calbach · 2022-04-07T23:31:42Z

e2e/app/component/table.ts

-    }
+    // Find the row index which contains searchCellValue
+    const allRows = await this.getRows();
+    const tds = await Promise.all(allRows.map((row) => row.$x('./td')));


There's a lot of extraneous state variables introduced compared to the previous approach which is making this more challenging to read, in my opinion. I'm thinking it may be more readable to group these together, e.g.:

const searchColumnTdValues = await Promise.all(allRows.map(async (row) => { const td = await row.$x('./td'); return getPropValue<string>(td[columnIndex], 'innerText'); }))

I like new code. Going to try it if it works.

calbach · 2022-04-07T23:32:29Z

e2e/app/sidebar/genomic-extractions-sidebar.ts

-        return true;
+      .waitForXPath(statusSpinnerXpath, {
+        visible: true,
+        timeout: timeout


nit: just timeout

calbach · 2022-04-07T23:35:45Z

e2e/app/sidebar/runtime-panel.ts


  /**
-   * Open Runtime sidebar, check status for running.
+   * Open Runtime sidebar, wait for running or error status.


Logically, why would waitForRunning return when it's in the error state? Consider renaming the method or else updating the semantics some other way

Added comments. Renamed func to waitForFinish.

/** * Open the Runtime sidebar, wait for the Running or Error status. * The Running or Error status in runtime creation is considered as done/complete. */ async waitForFinish(timeout?: number): Promise<boolean>

calbach · 2022-04-07T23:37:14Z

e2e/tests/nightly/admin/user-profile-admin.spec.ts

+      const cell = await accountAccessTable.findCellByRowValue(
+        TableColumns.ACCESS_MODULE,
+        accessModule,
+        TableColumns.REQUIRED_FOR_TIER


Interesting - how did this work previously, without specifying a search column?

I think it worked in this table because the TableColumns.ACCESS_MODULE is the first column and it was the only column where the search area was.

calbach · 2022-04-07T23:38:06Z

e2e/tests/nightly/genomic-extraction-to-vcf.spec.ts

    await signInWithAccessToken(page);
  });

  const waitForCompletionMaxTime = 60 * 60 * 1000;


This limit should be shorter than the overall timeout. By the time we reach this point in the code, we don't have 60 minutes left

Yeah, forgot to update this timeout after shorten jest timeout.

calbach · 2022-04-07T23:38:41Z

e2e/tests/nightly/genomic-extraction-to-vcf.spec.ts

-    // Total Count should be greater than 1.
+    // Total Count should be greater than or equal to 1 and less than or equal to 5.
+    // At the time of writing test, Total Count is 2. Total Count can change if synthetic dataset changes.
+    // We want keep Total Count is larger, it will take longer time to for the extraction job to complete.


calbach · 2022-04-07T23:40:08Z

e2e/tests/nightly/genomic-extraction-to-vcf.spec.ts


-  // Check creation status.
-  async function waitForComplete(page: Page, maxTime: number): Promise<boolean> {
+  async function waitForRuntimeComplete(page: Page, maxTime: number): Promise<boolean> {


Why do we have so much custom code in this test case for waiting on a runtime to start? Isn't this a very central function which should be supported by a library or utility? Is this not duplicated somewhere else in the code?

This function can be a common function but no other tests can use it as it is now. I would need more time to refactor this function so it can be used in other places. This function doesn't initiate the Create Runtime process. The only other runtime test is nightly/runtime.spec.ts. It's much simpler and doesn't have same test steps. But it could use this function if it is more generalized.

calbach · 2022-04-11T16:33:54Z

e2e/app/sidebar/genomic-extractions-sidebar.ts

 import DataTable from 'app/component/data-table';
-import { waitWhileLoading } from 'utils/waits-utils';
 import { exists } from 'utils/element-utils';
+import Cell from '../component/cell';


please avoid path relative imports like this (except same dir)

Good catch. I missed this.

aweng98 commented Apr 6, 2022

View reviewed changes

aweng98 requested a review from calbach April 6, 2022 18:00

calbach reviewed Apr 7, 2022

View reviewed changes

aweng98 added 3 commits April 8, 2022 14:39

refactor waitForComplete functions

db56430

changes

3232f62

PR feedback

4856ec1

aweng98 force-pushed the aw/e2e-genomic-extraction-to-vcf-test branch from 20df4bf to 4856ec1 Compare April 8, 2022 21:05

calbach approved these changes Apr 11, 2022

View reviewed changes

PR feedback

cf008b0

aweng98 merged commit 5c1cc6f into main Apr 11, 2022

aweng98 deleted the aw/e2e-genomic-extraction-to-vcf-test branch April 11, 2022 19:17

[no ticket][risk=no] Fix e2e genomic-extraction-to-vcf test: Update waitForComplete function #6532

[no ticket][risk=no] Fix e2e genomic-extraction-to-vcf test: Update waitForComplete function #6532

Conversation

aweng98 commented Apr 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

calbach left a comment

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

aweng98 commented Apr 5, 2022 •

edited

Loading