Refine SKILL.md: principles, structure, examples#130
Open
DmitrySharabin wants to merge 2 commits into
Open
Conversation
- Frontload foundational principles (move Best Practices + Common Mistakes to right after Core Pattern so they frame the mechanics that follow) - Add two foundational principles atop Best Practices: "what abstraction makes tests easier to understand for humans" and "write the tests first; scaffold around them" - Add Common Mistakes rows for reading values via this.arg (use the parameter list) and inline comments for test intent (use description) - Add a new data example showing a setup hook stashing a per-test fixture for run, with the hook-vs-run parameter-access asymmetry called out - Clarify hook signatures in the Lifecycle property-table row (called with no parameters) - Link to https://htest.dev as source of truth + a specific link to the data docs section - Reduce duplications: args-vs-arg warning lived in both property table and Common Mistakes; "don't test the language" lived in both Best Practices and Common Mistakes; the over-prescriptive run (value) Common Mistakes row contradicted the looser this.arg row, dropped
✅ Deploy Preview for h-test ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
LeaVerou
reviewed
May 15, 2026
|
|
||
| - **What abstraction makes the tests easier to understand for humans?** If a rule below would impose an abstraction that makes a test harder to read, the rule is wrong for that test. Declarativeness exists to remove repetitive noise — not to force inherently imperative code into a data shape. | ||
|
|
||
| - **Write the tests first; scaffold around them.** Express each test as data (`name`, `arg`, `expect`) in whatever shape gives you maximum signal-to-noise on the intent. Write `run()`, `beforeEach()`, and other scaffolding *at the end*, derived from what the tests need. Starting with `run()` first shapes the tests around what `run()` can express, instead of letting the tests express the intent. |
Member
There was a problem hiding this comment.
Maybe add something around "The goal is maximizing signal-to-noise ratio, not characters. DRY is not the endgame, it just often correlates."
But also run() can sometimes be legit part of the test, like in the ones we were discussing.
Maybe instead of discouraging run() altogether, tell the agent to write the test both with and without a run() and evaluate what expresses intent better?
Member
Author
There was a problem hiding this comment.
Updated. Take a look, please.
LeaVerou
reviewed
May 15, 2026
LeaVerou
reviewed
May 15, 2026
| | Reading values via `this.arg.X` / `this.args[0]` inside `run` | The parameter list IS where `arg`/`args` arrive. Name and unpack them however fits the case: `run ({ x })`, `run ([first, ...rest])`, `run (args) { let first = args[0]; }`, etc. Reserve `this` for `this.data` (lifecycle state) and instance props. | | ||
| | Inline `//` comment explaining what the test verifies | Use the `description` field — that's its home as structured metadata next to the test. **Keep it brief** (a sentence, maybe two): add what the `name` can't carry — an issue reference, a subtle invariant, a non-obvious edge case. Don't write a paragraph and don't restate the name in prose. Reserve comments for short notes about mechanics. | | ||
| | Setup in `beforeAll` but used in `arg` expressions | Move to module top level — `arg`/`expect` evaluate at import time, before hooks | | ||
| | `new Instance()` in both `arg` and `expect` | Share one instance variable | |
Member
There was a problem hiding this comment.
Example? (for me, not the docs)
Member
Author
There was a problem hiding this comment.
The row's pointing at hTest's default check using === at leaf level — two structurally-equal-but-distinct instances fail. Short example:
// ❌ Two `new Map()` calls — different instances, fails
{ arg: new Map([["k", "v"]]), expect: { result: new Map([["k", "v"]]) } }
// ✅ Share one instance
let map = new Map([["k", "v"]]);
{ arg: map, expect: { result: map } }Same pattern is spelled out under Reference Equality for Instances later in the same file. I also tightened the row itself with a cross-link + brief "why" since your "Example?" suggested it was too terse for cold readers — happy to revert if you'd rather leave the row as-is.
LeaVerou
reviewed
May 15, 2026
| | -------------------------- | ---------------------------------------------------------------------------------------------- | | ||
| | `beforeEach` / `afterEach` | Run before/after each test. Inherited. `afterEach` runs even if the test throws. Sync or async | | ||
| | `beforeAll` / `afterAll` | Run before/after all tests in the group where defined. **Not inherited** | | ||
| | `beforeEach` / `afterEach` | Run before/after each test. Receive no parameters — access state via `this.data`, `this.arg`, etc. Inherited. `afterEach` runs even if the test throws. Sync or async | |
Member
There was a problem hiding this comment.
I wonder if we should be passing parameters to them!
Member
Author
There was a problem hiding this comment.
They probably need arg/args in most cases. And yes, I think it's useful. Everything else can be accessed via this.
LeaVerou
requested changes
May 15, 2026
Member
LeaVerou
left a comment
LeaVerou
left a comment
There was a problem hiding this comment.
See comments, but otherwise this looks great!
- Bullet 2: add DRY-isn't-the-endgame framing; move 'write both ways and pick' tactic into the principle; drop the discouragement that framed run() as a smell - First Common Mistakes row: broaden to 'minor differences'; tell the reader how to extract (push variations into arg/args) - new Instance() row: explain ===-at-leaves cause; cross-link to Reference Equality for Instances
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
2 participants
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.
Summary
Iterates on
SKILL.mdfrom a real session using it to refactor a downstream project's tests. Three themes: frontload principles, cut duplications, capture gotchas.Key changes
this.arg.Xinsiderun(use the parameter list);//comments for test intent (usedescription, kept brief).expect: true(smell, with yes/no-question-name exception); customrunper test (identical bodies vs unique imperative content).dataexample: setup hook stashing a per-test fixture forrun, with the hook-vs-runparameter-access asymmetry called out.datadocs.argsvsargwarning, "don't test the language", over-prescriptiverun (value)row.Preview: https://deploy-preview-130--h-test.netlify.app/skill.html