Playwright improvements

[Irev-Dev]

Here's some broad stokes of what we'd love to see happen. Break these into as many PRs as you see fit.

• [ ] Fix flakey tests axiom should be a good guidance of what are the worst ones (it might be a good idea to have a done condition on this one*)
• [ ] Review existing tests for speed-up optimisations (timeouts etc), and other best practices.

Optional

• [ ] Work with us on ways we can further improve the speed of the suite, maybe running on our own infra etc, very optional but would be worth having the conversation

• [ ] Look into if we can remove the nasty home-rolled-retires for playwright, I can give some context on this once we get to it. It's not super urgent because it does work, but we can discuss why we were having issues with the playwright-native retries and if we get that working instead

• axiom only gives total failures, which lets us know which are the worst, but really we would want the failures to be something like no more than 1 in 10 failures etc, maybe I need to add a reference log to axiom each time a playwright test runs to act as a reference so we can get that ration out of it 🤷 thinking out load here

[ryanrosello-og]

Other actionable items:

• [ ] Add a separate playwright config for CI. This will alleviate the need to have logic in the config file. Essentially, we'll have two config files: playwright.config.ts < for local playwright.ci.config.ts < used in GH actions
• [ ] Enable Playwright + react test coverage reporting to show gaps in the test coverage
• [ ] Resolve the underlying issue of why we need to have all of the tests in a single spec file (flow-tests.spec.ts is yuge and annoying to work it).
• [ ] Make the tests fail if there are console errors detected. Implement a whitelist for errors that are known/don't care about
• [ ] Use a global.types.ts (or some other mechanism) to better organize types and interfaces
• [ ] Implement either page object pattern or a function based way of organizing locators to avoid duplication of locators
• [ ] Add test-id attributes to the major components in the UI to ensure the tests don't become brittle by relying on css selectors that are prone to changes
• [ ] There are numerous places in the code where the any type is being used. It would be good if these were properly typed (where possible)
• [ ] Have some process for triaging flaky tests. e.g. We could tag flaky tests as @flaky and exclude them from the main CI suite. Once they are stable, we could remove the @flaky tag and reinstate into the main CI suite. Perhaps run these flaky tests in a separate schedule GH action
• [ ] Trivial - remove magic numbers being used in timeouts
• [ ] Trivial - remove redundant awaits in a bunch of places

[Irev-Dev]

Excited for all this.

Tests in a single spec file I can add some context here. The main reason why there are just two files snapshot-tests.spec.ts and flow-tests.spec.ts is because the way we decided to do the snapshot tests, first of all we try and use snapshots sparingly, but there are a few places where it's the best way to verify something. But the motivation for having them in their own dedicated file was I've not seen a great way to update snapshot when they need to. In another repo I had this elaborate series of actions that would make a comment on PRs with the failed snapshots, and then if you made a special comment on the PR another action would run and update the snapshots, but honestly it was a big PITA.

So what we've opted for here is no always run snapshot-tests.spec.ts with --update-snapshots and then automatically commit the images in the PR, that way the images show up as part of the PR, if they needed updating then good, if the diff revealed a bug, you should notice and you can either force push away the image commit or just leave it since we squash merge our PR.

All that is to say in order to break up flow-tests.spec.ts then we'd just need to change the action from run only flow-tests.spec.ts to run everything except for snapshot-tests.spec.ts.

Add test-id attributes to the major components in the UI to ensure the tests don't become brittle by relying on css selectors that are prone to changes

Probably the worst cases for selectors are things like cm-content or cm-xxx really. The cm stands for code mirror, i.e. it's a library, I've not looked into how hard it would be get other attributes onto the elements we're interested in.

[ryanrosello-og]

ah good info.

I think we should be able to make use of Playwrights' Tagging mechanism

Essentially, we can tag all of the snapshot-tests with @snapshot

Tweak the GH Action:-

Run snapshot tests only:

yarn playwright test --project="Google Chrome" --grep @snapshot

Run all of the other tests (excluding snapshot tests)

yarn playwright test --project="Google Chrome" --grep-invert @snapshot

^ This way, we are not bound to the spec files.