You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Title of this pull request clear, concise, and indicative of the issue number it addresses, if any?
Test suite(s) passing?
Code coverage maximal?
Changeset added?
What does this change address?
This is a followup to #1300 which migrates the first test suite to vitest as part of #1009
How does this change work?
This replaces the basic functionality @open-wc/testing was providing with the vitest equivalents as well as adding some custom helper functions to make the transition a little easier:
Helper functions:
Because we are using Lit web components, we need to ensure that when vitest browser renders the components, the inner shadow dom has rendered. There is a new fixture helper which handles this by mounting the component in a div (or optionally an element passed as an option) and waiting for updateComplete to fire before returning it.
Similarly, there is an errorFixture helper which should make testing the correct errors are getting thrown.
Custom matchers:
Many of the tests are currently written to assert specific html content - either in the light dom or the shadow dom, using shadowDom.to.equal. One issue we run into now is that Lit components use marker comments (<!--?lit$864487786$-->) around their bindings. This made alternatives to these matchers (like inline snapshots) run into issues. To get around this without having to radically rewrite our tests I added two helpers: toEqualDom and toEqualShadowDom. These matchers use the @open-wc/semantic-dom-diff library to create html that is clean of those comments as well as cleaning up any whitespace. This allows us to keep our individual tests mostly unchanged.
We also lose the .to.be.accessible assertion, which was using axe under the hood to perform basic accessibility tests. Instead, now we will use axe-core directly, with a new toBeAccessible matcher which will perform the same tests as before and format the result in a more readable way:
Additional context
We could get away without the DOM helpers by rewriting the tests to not check the DOM and instead use classes, roles. etc to validate specific things look right. I debated that, but this seemed both faster, and less likely to accidentally invalidate a test. With such a large test suite this method seemed the best way forward, but open to other opinions if you have them!
Once this gets merged in, I want to continue on replacing the rest of the tests. With this as a guide I think its something an LLM can pump through pretty quick, but I am not which would be preferred:
A PR per component. That's easier to review but there will be a lot of them
One mega PR that replaces all the rest. It will be harder to review but you'll only have to do it once.
Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.
This PR includes no changesets
When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types
The reason will be displayed to describe this comment to others. Learn more.
Looks great to me knowing what I know. The utilities here were easy to understand (though maybe a shame we need to roll some of our own, but none of them are overly complicated from what I can see). Looking forward to how these play out across the code base, and I suppose we'll see if/how much further we need to extend as we go along with other components.
though maybe a shame we need to roll some of our own
Yeah, I looked around a bit because I didn't love it either. There are currently officially supported packages for Vue, React, and Svelt. I looked at the unofficial vitest-browser-lit that provides the Lit equivalent. Since it is unofficial and the helpers we actually need were so small, it seemed this route made more sense.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This change: (check at least one)
Is this a breaking change? (check one)
Is the: (complete all)
What does this change address?
This is a followup to #1300 which migrates the first test suite to vitest as part of #1009
How does this change work?
This replaces the basic functionality
@open-wc/testingwas providing with the vitest equivalents as well as adding some custom helper functions to make the transition a little easier:Helper functions:
fixturehelper which handles this by mounting the component in a div (or optionally an element passed as an option) and waiting forupdateCompleteto fire before returning it.errorFixturehelper which should make testing the correct errors are getting thrown.Custom matchers:
shadowDom.to.equal. One issue we run into now is that Lit components use marker comments (<!--?lit$864487786$-->) around their bindings. This made alternatives to these matchers (like inline snapshots) run into issues. To get around this without having to radically rewrite our tests I added two helpers:toEqualDomandtoEqualShadowDom. These matchers use the@open-wc/semantic-dom-difflibrary to create html that is clean of those comments as well as cleaning up any whitespace. This allows us to keep our individual tests mostly unchanged..to.be.accessibleassertion, which was using axe under the hood to perform basic accessibility tests. Instead, now we will useaxe-coredirectly, with a newtoBeAccessiblematcher which will perform the same tests as before and format the result in a more readable way:Additional context
We could get away without the DOM helpers by rewriting the tests to not check the DOM and instead use classes, roles. etc to validate specific things look right. I debated that, but this seemed both faster, and less likely to accidentally invalidate a test. With such a large test suite this method seemed the best way forward, but open to other opinions if you have them!
Once this gets merged in, I want to continue on replacing the rest of the tests. With this as a guide I think its something an LLM can pump through pretty quick, but I am not which would be preferred:
Let me know which you all would prefer to review!