-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
test: create tests for index export button (#4117) #4217
test: create tests for index export button (#4117) #4217
Conversation
7cc3e57
to
f05f64d
Compare
There was a problem hiding this 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.
explorer/e2e/testFunctions.ts
Outdated
header: detail, | ||
value: ((await page.getByText(detailBoxRegexp).innerText()) | ||
.trim() | ||
.match(detailBoxRegexp) ?? ["", "ERROR"])[1], |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
85d46fd
to
30b8182
Compare
explorer/e2e/testFunctions.ts
Outdated
await expect(exportButtonLocator).toBeVisible(); | ||
await exportButtonLocator.click(); | ||
await expect( | ||
page.getByText(tab.indexExportPage.requestLandingMessage ?? "") |
There was a problem hiding this comment.
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(...)
}
There was a problem hiding this comment.
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
explorer/e2e/testFunctions.ts
Outdated
header: detail, | ||
value: ((await page.getByText(detailBoxRegexp).innerText()) | ||
.trim() | ||
.match(detailBoxRegexp) ?? ["", "ERROR"])[1], |
There was a problem hiding this comment.
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.
30b8182
to
fc99db6
Compare
@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! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks Jonah!
Ticket
Closes #4117.
Reviewers
@NoopDog .
Changes
Questions