Vitest migration #3076
Merged
Vitest migration #3076
Conversation
This file contains 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
github-actions
bot
added
dependencies
QA Summary
Test CoverageCoverage report for `packages/client`Coverage report for `packages/server`
Pusher: @TylerHendrickson, Action: |
Terraform Summary
Hint: If "Terraform Format & Style" failed, run OutputValidation OutputPlan Summary
Pusher: @TylerHendrickson, Action: |
TylerHendrickson
requested changes
May 21, 2024
.github/workflows/ci.yml
Outdated
Comment on lines
4
to
5
| pull_request_target: {} | ||
| # pull_request_target: {} | ||
| pull_request: {} # TEST TRIGGER - DO NOT MERGE |
There was a problem hiding this comment.
Adding a comment here that will be made outdated by reverting ff47a47
|
@jeffsmohan Responding to your points on the main PR comment:
Agree that performance arguments don't seem compelling here – GHA is spending all of 12 seconds running these tests and <1 second generating the coverage report, which doesn't feel too egregious. FWIW, I don't think that the client- and server-side coverage tools need to be the same, but I would like to retain the ability to include a coverage report in a
I'm not certain I'm following here – are you just saying that this PR increases coverage in files where it was previously lacking? Or would you mind elaborating a bit?
Agree with your preference, and yes, I think that having more code that's explicit/obvious and less magical is especially useful for our codebases.
There can be weird implications for doing this in Python with good reason to avoid, but colocated tests are generally my preference. Are there any caveats we should be aware of / reasons why having tests organized in a mirrored directory structure (as they are today) would be beneficial? If not, I think it's worth changing (in a separate PR). |
…anch" This reverts commit ff47a47.
Scratch that, the coverage reports come out the same. I was looking at a report from before I had tweaked some settings.
Yeah, I think it's worth following language conventions when we don't have a good reason not to. For JS, it's well supported and most common to colocate the tests next to the code files. I'm not aware of any real downsides. I'll work up a follow-up PR to relocate the code. |
|
@TylerHendrickson thanks for the review! Responded to comments, reverted the workflow testing commit, and this should be ready to go. |
TylerHendrickson
approved these changes
May 21, 2024
This was referenced May 22, 2024
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Ticket #
Description
This PR migrates our current client-side test runner/assertions/mocking (based on
vue-cli,mocha,chai,sinon) over tovitest(https://vitest.dev/). This supports our upcoming migration fromvue-cliover toviteas our development server and production build utility. (See Decision #0008 for background.)This PR is meant to be easiest to review commit by commit. The key changes are:
sinonusage, a few minor fixes)nycconfig,babelconfig, packages no longer used)describe,expect,it, etc.) to explicit importsI also want to specifically call out a few design considerations that might be worthy of more discussion:
Note that the coverage report itself is slightly different than it was. (Notably, some files that I think we'd want coverage for are not included in the previous reports.) If continuity in our coverage stats is important, I can try to chase down the differences and reconcile them, but my assumption is the coverage data is right now just providing helpful comments on PRs.After fixing a setting, this is no longer the case. The coverage reports match exactly.describe,expect,it, etc.) to explicit imports in our tests. I have a strong preference personally for explicit, since it makes code clearer and less magic-seeming for new developers. (And the minor downside of writing slightly more code should be minimized by your editor's automation.) That said, it's a meaningful change, so I wanted to check in on it.Foo.vueandFoo.spec.vuewould live in the same folder.) My personal conviction here isn't strong enough to warrant changing the status quo in this PR, but the argument is that colocated tests are easier to find, easier to work on, and perhaps make it easier for devs to write more tests.Screenshots / Demo Video
Testing
Automated and Unit Tests
Manual tests for Reviewer
Checklist