tiny-pilot / tinypilot

Use your Raspberry Pi as a browser-based KVM.
https://tinypilotkvm.com
MIT License
2.95k stars 245 forks source link

Group e2e tests #1685

Closed jotaen4tinypilot closed 7 months ago

jotaen4tinypilot commented 8 months ago

I noticed that our end-to-end tests are slightly hard to follow, as they contain sub-scopes that are supposed to divide the test into different aspects (e.g. this one), but these groups don’t have any description.

We could use Playwright’s test.describe helper here to provide more structure. This PR demonstrates how that works:

The sub-tests are run in parallel by default. That works fine for the about dialog. In Pro, we also have sequential tests, which we’d need to configure explicitly via test.decribe.configure.

Advantages:

This PR is not meant for merging, it’s just a demo. If we’d be interested in this, I could either wrap up this branch, or create a ticket for us to refactor this later. I’d estimate as small, including Pro. We’d also have to wait for https://github.com/tiny-pilot/tinypilot/pull/1682.

An alternative would be to put comments above the scoped blocks. These wouldn’t be printed to the CLI in the same neat way as the nested tests, though, e.g. if one aspect fails. Review on CodeApprove

mtlynch commented 8 months ago

Sure, this sounds good to me. I didn't know about these Playwright mechanisms, and it seems like a win overall. At first, I worried we'd take a performance penalty for having to re-open the About dialog a million times, but it looks like we actually end up improving performance because we're doing all the checks in parallel instead of serially.

jotaen4tinypilot commented 7 months ago

@mtlynch I felt so free to add this to the 2.6.3 project, just to give it more visibility, and avoid it falling through the cracks. If you’d prefer to defer, or turn it into a ticket, let me know.

mtlynch commented 7 months ago

@jotaen4tinypilot - Cool this sounds good. I don't want to do too much more refactoring / code conventions tickets this release since it's been pretty heavy in that so far, but a small is fine.

jotaen4tinypilot commented 7 months ago

Closing in favor of https://github.com/tiny-pilot/tinypilot/pull/1693. (I opened a new PR, since it was easier that way due to the changes that happened in the meantime.)