Skip to content

test: create tests for index export button (#4117)#4217

Merged
mrtopsyt merged 7 commits intomainfrom
jonah/4117-create-tests-for-main-table-export-button
Nov 8, 2024
Merged

test: create tests for index export button (#4117)#4217
mrtopsyt merged 7 commits intomainfrom
jonah/4117-create-tests-for-main-table-export-button

Conversation

@mrtopsyt
Copy link
Copy Markdown
Contributor

Ticket

Closes #4117.

Reviewers

@NoopDog .

Changes

  • Added the following tests for the "index" export page:
  • Workflow smoke test
    • Click the "Export" button
    • Click the "Request File Manifest" button
    • Select one checkbox from the File Format table and all possible Organism Type checkboxes
    • Click the "Prepare Manifest" button
    • Click the "Download Manifest" Button
    • Check that a file downloads
  • Selected Data Summary test
    • Read the counts next to the Index Export Button (see screenshot)
    • Click the "Export" button
    • Check that the counts on the index page match the counts under "Selected Data Summary"
Screenshot 2024-10-30 at 2 25 16 PM

Questions

  • Should we attempt to capture the download from the "Download Manifest" button, and if so, should we make any checks about it? Right now the download is made but we do not check the file name or size

@mrtopsyt mrtopsyt linked an issue Oct 30, 2024 that may be closed by this pull request
@mrtopsyt mrtopsyt marked this pull request as draft October 30, 2024 21:31
@mrtopsyt mrtopsyt force-pushed the jonah/4117-create-tests-for-main-table-export-button branch from 7cc3e57 to f05f64d Compare November 4, 2024 23:14
@mrtopsyt mrtopsyt marked this pull request as ready for review November 4, 2024 23:30
@mrtopsyt mrtopsyt requested a review from NoopDog November 4, 2024 23:30
Copy link
Copy Markdown
Contributor

@MillenniumFalconMechanic MillenniumFalconMechanic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really solid Jonah - thank you! Just a few nits and q's.

Comment thread explorer/e2e/testReadme.md Outdated
Comment thread explorer/e2e/anvil/anvil-tabs.ts Outdated
Comment thread explorer/e2e/testFunctions.ts
Comment thread explorer/e2e/testFunctions.ts Outdated
Comment thread explorer/e2e/testFunctions.ts
Comment thread explorer/e2e/testFunctions.ts
Comment thread explorer/e2e/testFunctions.ts Outdated
Comment thread explorer/e2e/testFunctions.ts Outdated
Comment thread explorer/e2e/testFunctions.ts
Comment thread explorer/e2e/testFunctions.ts Outdated
header: detail,
value: ((await page.getByText(detailBoxRegexp).innerText())
.trim()
.match(detailBoxRegexp) ?? ["", "ERROR"])[1],
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an interesting one! I feel like it'd be slightly more readable if you handled the error case separately/explicitly rather than creating a dummy match array. I'd probably write it more like:

  • Get the inner text, trim, match
  • If the match is truthy, push the [1] value to the headers array
  • Otherwise, push "ERROR" to the headers array

Thoughts?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I refactored it so that it just fails the test if the regex doesn't match fully, let me know if that works! I think it's a little easier to read this way.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. I think the regex complexities should be resolved when we started adding test IDs.

@mrtopsyt mrtopsyt force-pushed the jonah/4117-create-tests-for-main-table-export-button branch 2 times, most recently from 85d46fd to 30b8182 Compare November 6, 2024 23:47
Comment thread explorer/e2e/testFunctions.ts Outdated
await expect(exportButtonLocator).toBeVisible();
await exportButtonLocator.click();
await expect(
page.getByText(tab.indexExportPage.requestLandingMessage ?? "")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need the nullish coalescing here now that requestLandingMessage is a required value? If it is still necessary, I feel it might make more sense to only execute the expect if there is a value for requestLandingMessage; that way, we will never be in a situation where we're calling page.getByText(""). Something like:

const requestLandingMessage = tab.indexExportPage.requestLandingMessage;
if ( requestLandingMessage ) {
  expect(...)
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops, no we don't! Thanks for catching that, I'll fix it

Comment thread explorer/e2e/testFunctions.ts Outdated
header: detail,
value: ((await page.getByText(detailBoxRegexp).innerText())
.trim()
.match(detailBoxRegexp) ?? ["", "ERROR"])[1],
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. I think the regex complexities should be resolved when we started adding test IDs.

@mrtopsyt mrtopsyt force-pushed the jonah/4117-create-tests-for-main-table-export-button branch from 30b8182 to fc99db6 Compare November 8, 2024 01:59
@mrtopsyt
Copy link
Copy Markdown
Contributor Author

mrtopsyt commented Nov 8, 2024

@MillenniumFalconMechanic I addressed your one comment, and also updated the test timeouts, since I noticed it had failed once because the loading screen after clicking the request button was taking long enough that it timed out. I'll document this as part of #4213, but just wanted to let you know!

Copy link
Copy Markdown
Contributor

@MillenniumFalconMechanic MillenniumFalconMechanic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks Jonah!

@mrtopsyt mrtopsyt merged commit 71df859 into main Nov 8, 2024
@mrtopsyt mrtopsyt deleted the jonah/4117-create-tests-for-main-table-export-button branch November 8, 2024 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Create tests for main table export button

3 participants