Test-writing discipline
SkillDev toolsUse when writing, fixing, or reviewing tests for any Handsontable change (unit, E2E, or wrapper), and whenever a test is red during feature work. Enforces that tests prove intended behavior — not just execute code, and never "green for the sake of green". Covers intent-first (write the test from the requirement, ideally before the code), deciding whether the code or the test is wrong when red (default: the code), the banned ways of faking green, write-the-failing-test-first, verify-with-a-real-run, no hollow assertions, not mocking the unit under test, and migrating broken legacy tests.
Available today. Use it from your connected AI after setup.
No other account needed.
Connect ahel once, and every AI you use reads what you have installed.
Then ask your AI: use the Test-writing discipline skill
What this skill tells your AI
The instructions your AI receives, as published by handsontable/handsontable in .claude/skills/test-writing-discipline/SKILL.md and read by ahel’s review.
Green is not the goal — correct behavior is. A test that passes but asserts nothing, or asserts the buggy output, is worse than no test: it certifies the bug and reads as coverage. Never make a red test pass by weakening it. These rules keep agent-written tests meaningful. They apply on top of the framework guides (handsontable-playwright-e2e, handsontable-e2e-testing, handsontable-unit-testing).
The test encodes intent, not the implementation
Write the test from the requirement — the behavior the user or the API is supposed to have — not from what the code currently does. Where feasible, write it first (or independently of the implementation) so it is an oracle you cannot accidentally fit to a bug. For E2E, state the user-observable expectation (what the grid should show or do) before you wire a single selector.
When a test is red, decide what is actually wrong — don't just chase green
At the feature stage the code is the prime suspect, not the test. Before you touch the test, ask: does its expectation match the intended behavior?
- Expectation correct, code wrong → fix the code. This is the common case. Leave the test alone.
- Expectation genuinely mis-encoded the intent → fix the test by tightening it toward the real behavior — never by loosening it to match the current (possibly wrong) output.
If you cannot tell which is wrong, that is a signal to re-read the requirement, not to relax the test.
Banned ways of faking green
Never reach green by any of these — they defeat the point of the test:
- Deleting or loosening an assertion, or widening a tolerance, to match what the code emits.
.skip/xit/xdescribe, or focusing with.only/test.only/describe.only/fit/fdescribe(focusing silently drops the rest of the suite).- Wrapping the body in try/catch to swallow a failure.
- Asserting whatever the code happened to produce (a "snapshot of the bug").
it.flaky/ retries to paper over a real intermittent failure.
The Stop hook and pre-push run the test you touched and block on red — that forces you to reconcile red, it does not authorize you to make it green by weakening it. Reconcile by fixing the right thing.
Weakening over an admitted race needs a ticket in the same PR
An assertion weakened or deleted with a race as the reason — "held 15/15 locally", "flakes on classic", "racy on develop" — descopes a product defect; it does not fix a test. It ships only with a filed ticket in the same PR, named beside the change. "Still open, no ticket" is a review blocker. The strand-window fix (#13332) descoped two racy shapes that way, and the tickets are the only thing that keeps them from vanishing into a green run.
"Passes on retry" and "passes in isolation" are not determinism evidence
- A test that passed on its second attempt, or alone after failing inside the suite, has shown you the flake — not its absence. Before you call it deterministic, hammer it focused across every leg:
cd tests && npx playwright test e2e/<spec>.spec.ts --repeat-each 20(no--projectfilter, so all six theme × bundle legs run). The hidden-init migration needed ~700 such runs to expose three rAF-starvation timeouts that any single green run hid. - A test that fails about half the time under that load on unchanged develop is reporting a product race. File a product ticket and keep the assertion — softening it to green certifies the race.
Bug fixes: write the failing test first
- Reproduce the bug as a test and watch it fail — for the right reason (the missing behavior, not a typo or a bad selector).
- Then apply the fix and watch the same test pass.
- A regression test that was never red proves nothing. On a bugfix PR, name the spec that fails without the fix.
Cover what the change actually adds
- A timing-semantics claim is verified against the primitive that implements it. A JSDoc that says "closes when the task ends" above a
setTimeout(0)is wrong:setTimeout(0)is the next macrotask, not end-of-task, so a synchronous caller in the same task runs inside the window the comment calls closed. That mismatch shipped a data-corruption bug — an editor stranded byalter()committed through stale coordinates and appended records (#13332). Read the primitive (setTimeout,queueMicrotask,requestAnimationFrame, a hook's call site) and write the test from what it does, not from what the comment says. - A dedicated X-during-Y path requires a test that drives X during Y.
flush,drain,cancel,pending,suspend, a depth counter — those names are the tell that the code handles one operation landing inside another. A test that runs X and Y in sequence never enters that path. Fire X from inside Y: a hook callback, a nestedalter(), an edit inside a batch. - Every documented form of a new option executes at least once — the guide's lead form especially. The shorthand a guide opens with is the form users try first, and the one most often left untested.
Verify before you say "done"
- Run the exact test command fresh, read the full output and the exit code, then state the result with that evidence.
- Banned phrasings: "should work", "this fixes it" without a run, "tested manually, looks fine". If you did not run it, say so.
- After editing source, run the impacted test — not the whole suite, the impacted one — and confirm green before finishing.
No hollow assertions
- Assert the behavior, not that the code ran.
expect(getDataAtCell(0, 0)).toBe('x'), notexpect(true).toBe(true)and not anit()with noexpectat all. - One meaningful assertion beats five that restate the setup.
- Coverage-on-new-code can be satisfied by a test that executes a line without checking its result — do not rely on it to judge quality; the assertion is what matters.
Don't mock the unit under test
- Never mock the thing you are testing. Mock at the real boundary (network, timers, ResizeObserver — see the mocks in
test/__mocks__/). - When you must build a mock, mock the complete real data shape; an incomplete mock gives a false pass. In this repo the default is to favor an E2E test over a unit test that would need to mock a module.
Fix broken legacy tests by migrating them
- The Jasmine suite is frozen. If a legacy
*.spec.jsis broken or flaky, do not patch it in place and do not add asleep()— rewrite it as a Playwright test intests/e2e/and delete the Jasmine one. The tests that hurt most migrate first; the suite shrinks by attrition. - Routine, low-risk edits to an existing Jasmine spec are still fine.
Signals
- GitHub stars
- 22k
- Forks
- 3k
- Last commit
- Sep 2026
Advanced
- Catalog kind
- skill
- Gateway key
test-writing-discipline- Source
- github.com/handsontable/handsontable