tests: add backup reminder banner test. #3666
Conversation
thisconnect
left a comment
thisconnect
left a comment
There was a problem hiding this comment.
LGTM, with one point about not using id for tests, please change to data-testid
|
|
||
| const { stdout } = await execAsync(cmd); | ||
| return stdout.trim(); | ||
| } |
There was a problem hiding this comment.
I don't understand engouh docker and am always confused about when to use spawn, fork or exec.
It looks very cool what you do in this file with the regtest, but I am not the right person to review this.
There was a problem hiding this comment.
Thanks, I will revisit this file, add some comments, and ask for another reviewer :)
bznein
force-pushed
the
regtest
branch
8 times, most recently
from
fa11eee to
62f6f60
Compare
This was accidentally changed in 21ebd32
thisconnect
left a comment
thisconnect
left a comment
There was a problem hiding this comment.
LGTM with suggestion/question
| for (let i = 0; i < units.length; i++) { | ||
| await page.locator(`header [data-testid="amount-unit-${units[currentIndex]!}"]`).click(); | ||
| const nextIndex = (currentIndex + 1) % units.length; | ||
| await page.waitForTimeout(1000); // wait for the UI to update |
There was a problem hiding this comment.
q: instead of always waiting 1s, maybe waitFor() would be a better alternative?
https://playwright.dev/docs/api/class-locator#locator-wait-for
There was a problem hiding this comment.
I'm not sure how it would be possible to use waitFor here, based on the documentation. This are the possible states we can wait for: https://playwright.dev/docs/api/class-locator#locator-wait-for-option-state and none of them suit our case, if I'm not mistaken
|
adding @benma mostly for the regtest part, but feel free to dig around everything else as well :D |
2 participants
Also solves a few flakiness issues and reverts an accidental change.
Before asking for reviews, here is a check list of the most common things you might need to consider: