Skip to content

CONTRIBUTING.md

Latest commit

 

History

History
97 lines (68 loc) · 4.28 KB

CONTRIBUTING.md

File metadata and controls

97 lines (68 loc) · 4.28 KB
Raw

Contributing

Getting set up

npm install
npx playwright-core install chromium
npm test

Node 18.18 or newer. No build step, no transpiler, no linter config to fight — the code is plain ESM that runs as written.

Running the tests

npm test                                   # everything
node --test test/config.test.js            # one file
node --test --test-concurrency=1           # serial, easier to read when debugging

The suite never touches the network. Fixture pages are served by a node:http server on an ephemeral port, and the fixture server asserts that shotdiff only ever issues GET requests.

Tests that need Chromium skip with an explanatory message when none is available. Do not rely on that. If you touch capture, stabilisation, or image encoding, make sure a browser is installed and the tests actually run — a skipped test proves nothing.

The rules that matter

A few invariants are load-bearing. Changing them is not a refactor.

Lossy pixels never enter a comparison

Read the header comment in src/imagecodec.js before touching anything to do with image formats.

  • Baselines are lossless PNG. Always. --baseline ignores storage.format, and combining --baseline with --format is a usage error rather than a silent override.
  • The comparison runs on the in-memory PNG buffer, before any storage encoding.
  • compare refuses a lossy operand up front, in collectPairs, before decoding anything.

WebP at q80 produces content-dependent artifacts. Storing a lossy baseline would put a few units of noise on every pixel of every future diff — which looks exactly like a real change and would undo the entire point of the stabilisation layer.

Baselines cannot be pruned

src/prune.js contains no reference to the baselines/ directory and must not gain one. The safety property is that there is no code path to reach it, not that a filter excludes it. If you find yourself adding if (kind !== 'baseline') somewhere, stop and reconsider the layout.

prunableRoots() is exported specifically so a test can assert this.

.complete is written last

capture writes the marker as its final act; prune refuses to delete a run without one. This is what makes a prune safe to run alongside a capture. Do not move the write earlier "for tidiness".

Nothing prints without going through the redactor

All output goes through Logger, which scrubs registered secrets. If you add an output path, route it through the logger rather than calling process.stdout.write directly.

shotdiff is read-only against sites

GETs only. No logins, no POSTs, no form submissions, no mutations. The single exception is purge.command, which is a command the user wrote. A test asserts the fixture server never sees a non-GET request; keep it that way.

Adding a stabilisation technique

If you add one, add a measurement with it. The README states what each technique is worth in measured percentages, not adjectives, and a new one should arrive with:

  1. A fixture that exhibits the nondeterminism, ideally in test/helpers/server.js under /isolate?feature=... so it can be measured in isolation.
  2. A test proving the noise goes away with it on.
  3. A control test proving the noise is really there with it off. Without the control, a fixture that quietly stops being chaotic turns the first test into a tautology.
  4. A row in the README's stabilisation section with the measured numbers.

Style

  • ESM, node: prefixes for builtins.
  • Comments explain why, especially where the obvious-looking simplification is a bug. There are several of those in this codebase and they are commented at the point of temptation.
  • Errors name the thing that is wrong: the site, the page, the file, and what to do next. A message that only says "invalid config" is a bug report waiting to happen.
  • Keep the dependency list at three. Every addition needs a paragraph in the README explaining why it earns its place, and native dependencies are refused outright — that constraint is why WebP encoding goes through Chromium.

Commit and PR

  • One logical change per commit, present tense, explaining why rather than what.
  • npm test green before you open the PR, with a browser installed.
  • Update CHANGELOG.md under an ## [Unreleased] heading for anything user-visible.