Add playwright e2e tests #868
Conversation
owi92
force-pushed
the
add-playwright
branch
24 times, most recently
from
2dfb5d9
to
eab8606
Compare
owi92
force-pushed
the
add-playwright
branch
2 times, most recently
from
58c550e
to
39b2774
Compare
There was a problem hiding this comment.
Finally some tests 🎉
This is just a first review: I have not yet looked at all the files in tests, but at everything else. Most of these comments are mainly questions for me to understand decisions :D After resolving all of that I can have another look at the actual tests.
| await page.keyboard.press("s"); | ||
| await expect(searchField).toBeFocused(); |
There was a problem hiding this comment.
Nothing wrong with this, just reminded me: we certainly need tests of typing something in a different input that contains s and making sure in that case, it does NOT jump to the search bar.
github-actions
bot
added
the
status:conflicts
|
This pull request has conflicts ☹ |
Unfortunately, we can't test everything on safari because it doesn't allow logins on http sites. On the other hand, skipping some tests also speeds up the total run time...
This should speed up local test runs. Though using more than 4 workers apparently still produces more flaky results. The implemented solution also doesn't work for multiple concurrent runs (i.e. using `--repeat-each` in cli runs), because there is a limited number of dummy users to test user realms simulaneously.
Where possible, text content (like placeholder, label etc) is used instead. Some elements don't have any text or the text is variable. In these cases this still uses the `_react=ReactComponent` locator, but those could also be replaced by using `testId`s on the components instead. It's not super clear for what reason that would be preferable, but playwright's docs indicate that it might indeed be (the `_react` locator is marked as experimental.. well I guess that's clear enough).
github-actions
bot
removed
the
status:conflicts
|
Dangit I pushed the rebase combined with my changes again 🤦. |
|
I took a look at the tests. The tests are looking good so far and I think they cover pretty much everything. |
There was a problem hiding this comment.
Then let's just merge this! I still haven't done a full review due to time contraints, but that's fine. This is not code that is shipped anyway. The worst that could happen is a bit of flaky tests in CI, with random CI failures. But that's easy to fix and no big problem. No need to block this any longer. Makes more sense to test-drive these tests on master anyway.
This adds the playwright framework for e2e UI tests as outlined in #855.
Several tests are already included but are still subject to change and improve. Unfortunately some of these can be flaky and are prone to getting stuck when loading and or navigating (in like 10% of all runs). I'm unsure if that is caused by poor test design or outside conditions. Right now the config allows for 2 retries to be extra sure everything passes, but in in my experience 1 retry should suffice.
Edit: Actually, I haven't encountered any test failures in CI so I changed that back to 1 retry.
Especially the
realmandblocktests might still benefit from breaking them down into smaller chunks, but since so much of these tests depend on one another, I started by doing them in one swoop and in sequence.I broke each test down into several steps and sub-steps using
test.step, but there are definitely other ways of doing this. The benefit of usingtest.stepis mainly for readability and sequentiality (is that even a word? google says it is 🤷)Another thing to consider for these tests:
Right now they rely on certain events and series to be present somewhere in Tobira, and at least one series block to be present on the "welcome page". I believe this is suboptimal since that data might change in the future (correct me if I'm wrong of course).