Introduce Testing to Stacks

[giamir]

This PR builds on top of the PoC Testing PR and supports the Testing Strategy ADR with a concrete implementation.

Context

Prerequisit: we are now tracking visual regression tests baseline images with git lfs (github supports that by default) and you need to have the CLI installed in your local machine to pull and upload those images. You can follow this guide to do so.
Apart from that there is nothing else to learn. You can use git as normal.

There are several file changes because we had many files violating the existing linting rules. You can review those fixes in isolation by checking the individual commits:
- fix style rules violations (664bef5)
- fix eslint rules violations (53937b3)
- fix prettier formatting violations (b5d1aea)

Fix existing linting and formatting issues has been now moved into a separate PR.

I also updated the README.md extensively and added a basic GH workflow to build and run tests on every push.

Open Issues and Workarounds

Testing Library Overhead in Browser/ESM Context

Testing Library was initially created to run in a Node.js environment and as a consequence it is relying on several dependencies which supports only CJS and cannot be made ESM-compatible in an easy way by bundler plugins (in our case we use @rollup/plugin-node-resolve and @rollup/plugin-commonjs).
By using @web/test-runner to run our tests all the code is evaluated in a real browser context and everthing is dynamically imported as an ES module. Since not all our dependencies are ESM we need a bundler (in our case rollup) which takes care to resolve (plugin-node-resolve) and "esmify" (plugin-commonjs).

I had to use npm ovverides to fix a transitive dependency of testing library (aria-query) to a version which does not consume deep-equal (they introduced that last month in a minor release). deep-equal is using a couple of old dependencies (like call-bind) which rollup was not able to easily "esmify" and resolve. This could be fixed by instructing rollup to do something special about those deps with a small plugin. I did not want get into that yet since we are not even sure if we will keep testing-library (therefore the temporary npm override).

Testing library comes with a great a11y-friendly API which is familiar to most frontend developers. It also come with a tooling overhead cost to make it compatible with esm and, as a heads up, if we will use shadow dom in the future we also will have to work around some other limitations.

I personally like very much the structure testing-library bring to our tests and I would be more than willing to pay up some of these costs. I might even foresee a nice opportunity to contribute back to the OS community over time since we are not the only one facing similar challenges (see this Adobe's RFC). That said I would also understand if we want to stay away from this complexity and use directly DOM APIs to query elements under test and simulate interactions.

Web Dev Server Temporary Patches

Web Dev Server (the dev server behind our test runner) is currently unable to execute correctly the latest version of the @rollup/plugin-commonjs. This is something I am not too worried about since the maintainer are aware of the issues and already working on a fix. For the time being we have a patch file which we could easily discard once the fix will be released.

On top of that we have found an issue where the rollup adapter won't resolve virtual modules in windows environments. A PR with a fix is waiting to be reviewed and merged by the mantainers. In the meantime we point npm to an ad-hoc patched tarball.

Prettier and Style Formatting

I have experienced a strange behavior running prettier against our less files: the formatter will break the style syntax.
For the time being I have decided to exclude less files from prettier since they already get some format check with stylelint. We might want to revisit this decision though.

Fix existing linting and formatting issues has been now moved into a separate PR.

Typescript/Eslint checking for test files.

At the moment test files are not linted. This is because I did not want to mess around with the tsconfig.json file. Furthermore I have encountered a strange problem where the assertions (e.g. expect) we import from the @open-wc/testing package seems to don't inherit correctly the related Chai types. Further investigation is required although I think this is not very urgent given that we will need to readjust the structure of our repository soon to adhere to the Monorepo ADR. That might be a good time to address this problem.

[netlify]

✅ Deploy Preview for stacks ready!

Name	Link
🔨 Latest commit	483c0cc
🔍 Latest deploy log	https://app.netlify.com/sites/stacks/deploys/639af6d65dd02c00092b1c7c
😎 Deploy Preview	https://deploy-preview-1194--stacks.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site settings.

[b-kelly]

Preliminary comments while I'm attempting to run locally:

• Can we split out the formatting fixes into it's own commit? I'm totally fine with committing those straight to develop without a PR, assuming they're whitespace only changes.
• I can't get the tests to run I keep getting Failed to fetch dynamically imported module style errors. I'll debug this locally and see what's up there.

[b-kelly]

The issue is that the path of script modules being pulled in are pointing to the file:// path on my drive, which the browsers are blocking. The issue is definitely with fromRollupWithFix. Switching that out to the original fromRollup results in a completely different set of files, but they're at least being served via the dev server instead of pulled in from disk.

[giamir]

Can we split out the formatting fixes into it's own commit? I'm totally fine with committing those straight to develop without a PR, assuming they're whitespace only changes.

Formatting and linting are already isolated in their own commits so I could easily cherry pick them to any other branch.
They are not only whitespaces though (especially the fix for the linter errors - although they are still trivial things). Here is an idea I will create another PR just for the formatting/linting fixes and ask @dancormier to quickly double check it before merging it to develop. This branch will be based off that PR then to improve the diffs.

Here are the commits I will cherry pick to the new PR:

• fix style rules violations (664bef5)
• fix eslint rules violations (53937b3)
• fix prettier formatting violations (b5d1aea)

Update: this has now been addressed in this PR
Update2: formatting PR has been merged to develop. Now the diffs should be easier to read. @b-kelly

[giamir]

The issue is that the path of script modules being pulled in are pointing to the file:// path on my drive, which the browsers are blocking. The issue is definitely with fromRollupWithFix. Switching that out to the original fromRollup results in a completely different set of files, but they're at least being served via the dev server instead of pulled in from disk.

This is probably a windows specific issue. I tested both in macos and ubuntu (for the pipeline) and everything worked out fine. I have a windows VM too - will try to make the time to reproduce your error in that environment.

Update: @b-kelly I have described the problem in this issue and made a PR with a fix. In the meantime we point npm to an ad-hoc patched tarball. You should be able to continue your review and run the tests successfully in windows now.

[dancormier]

I like it!* I have a minor note on the font-family rule for the visual regression tests, but that's my only note. Great job @giamir!

A few notes for posterity (no action required):

• I love seeing BackstopJS get removed ❤️ It helped at times, but I'm happy to see WTR/Playwright here now.
• I feel like now/soon would be a good time to switch to a component-based file structure. I went searching for the tooltip test, and that little bit of friction reminded me that we should make the switch. I'm glad to make that change soon, as long as we can time it thoughtfully to avoid a ton of conflicts.

---

* This note is for my own entertainment. Please ignore it.

For years, I've used the phrase "I like it" in place of LGTM. When I heard the phrase in my head, it's always been in this yell, like "I LIKE IT!". I couldn't quite place it, but it finally came to me today. Turns out, I've been hearing the very end of the song Indiscipline from the classic 1981 album Discipline by English progressive rock band King Crimson. Mystery solved!

[giamir]

Thanks @dancormier for the review.

I feel like now/soon would be a good time to switch to a component-based file structure. I went searching for the tooltip test, and that little bit of friction reminded me that we should make the switch. I'm glad to make that change soon, as long as we can time it thoughtfully to avoid a ton of conflicts.

If @b-kelly agrees as well, we should create a task for our backlog and do this even before starting the work to migrate to the monorepo structure.
I remember there was an open topic about keeping component styles in a separate package from the respective TS. Personally I think style, behavior and tests for these components should live in the same package but be exported/published in a way that the consumers can exclusively use styles if they want to. I am making the assumption that any other components we will be creating in future (e.g. web components or framework X) won't attempt to reuse any of those components styles (apart from the atomic classes which will almost certainly leave in a different package).

[b-kelly]

It runs locally on my machine now! 🎉 🎉

The tests pass on Chromium+Webkit, but not on Firefox (looks like text rendering stuff):

I'm re-reviewing the code again now.

[b-kelly]

Thank you for getting this in place. I'm very excited about the approach you've implemented here. Other than the missing upstream fix, this looks good to merge!

[b-kelly]

🎉 Nice, this should keep us from accidentally committing images to the git history

[b-kelly]

Ooooh, I like this! Forcing the tests to use the accessibility tree is also a way to force a11y testing in general.

[b-kelly]

I'm ok with this as a short-term fix (until upstream releases a patched version).

No changes necessary.

[giamir]

Yeah, I think so too. We are also covered from a security perspective since we have the content integrity check thanks to the package-lock.json. I will follow up again about my one-liner PR for WTR.

[giamir]

The tests pass on Chromium+Webkit, but not on Firefox (looks like text rendering stuff)

That is not good, flaky tests are as good as no tests (maybe even worse). I will add an issue for us to explore running the visual regression test suite in a container as Dan suggested. Might use tomorrow's LSG to learn a thing or two about that.

[giamir]

Thanks @b-kelly for the review. I will go ahead then and merge this and the testing ADR PR.
I have followed up also with the maintainers of WTR to get that upstream fix merged.
Finally I have a todo item to write an issue for running visual regression tests in a container.
Very excited to have testing tools at our disposal from now on in Stacks!