feat(require-annotation-reason): require a reason on skip and fixme - #497
Open
unlikelyzero wants to merge 1 commit into
Open
feat(require-annotation-reason): require a reason on skip and fixme#497unlikelyzero wants to merge 1 commit into
unlikelyzero wants to merge 1 commit into
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.
Adds a
require-annotation-reasonrule.The enforceable norm on most teams isn't "never skip a test" — it's "never skip a test silently."
test.skip(isMobile)tells the next reader nothing: six months later nobody knows whether the underlying bug was fixed, whether the condition still holds, or who to ask, so the annotation stays forever and the coverage never comes back.test.skip(isMobile, 'ref WET-204 — layout breaks below 768px')costs nothing and answers all three.That's a norm a team can turn on at
errortoday, which is not true of a blanket prohibition on skipping. It came out of runningno-skipped-testagainst a mature suite where everyfixmealready carried a Jira key — enforced purely by convention and code review.Scope
The rule checks the conditional form, where
descriptionis a documented Playwright parameter:skipandfixmeare checked by default;failandslowdon't remove coverage the same way, so they're opt-in viaannotations. An empty or whitespace-only reason counts as missing. A reason whose value isn't known statically (a call, an interpolated template) is accepted as present but not pattern-checked.The declaration form (
test.skip('title', fn)) is deliberately not reported — Playwright's signature has nodescriptionparameter there, so there'd be no way to satisfy the rule. That case belongs tono-skipped-test, and the two rules are complementary and independently adoptable: this one governs how an annotation is written, that one governs whether it may be used at all.patternAn optional regex the reason must match, which is what makes the ticket-reference convention enforceable in CI rather than in review:
{ "playwright/require-annotation-reason": ["error", { "pattern": "\\b[A-Z]+-\\d+\\b" }] }Not enabled in the recommended config.
Note
This branches off
main, sotestInfo.skip(...)/testInfo.fixme(...)aren't recognised yet. It picks that up for free once #490 lands, since detection lives inparseFnCall.