docs: add testing guidance#1118
Open
CEREBR4L wants to merge 1 commit into
Open
Conversation
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
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.
Please describe what issue(s) this PR fixes.
Not tied to a specific issue. Testing isn't documented anywhere right now,
CONTRIBUTING.mddoesn't mention it at all, so it's a guess for anyonecontributing whether a change needs a vitest spec, a playtest, or nothing.
I ran into that gap myself while trying to get more involved here, and
figured writing down what's actually going on (a few different tools for
different situations, most
tests/playtestsfiles aren't even tagged@teston purpose, etc.) would give everyone, myself included, the sameshared understanding instead of each contributor re-deriving it on their own.
Summary
TESTING.md: the three test tiers, where each kind of fix goes, andwhen it's fine to skip a test.
CONTRIBUTING.md, and addedpnpm run test/test:viteto the pre-push checklist.
One thing I want to float rather than just decide: the doc suggests saying
why you're skipping a test, even if it's just "visual change, verified with
pnpm dev". Open to cutting that if it's not something you want tonormalize. As does quietly add the stupulation all PRs should at least
address testing.
Also aware #1116 is reworking testing too (Playwright,
tests/e2e, a new PRtemplate with reviewer-side test checkboxes). This doc will need a follow-up
once that lands, or I'm happy folding it into that PR instead if you'd
rather not have two testing changes in flight at once.