Skip to content

test: smoke-test the built release files - #114

Closed
hsd-bot wants to merge 2 commits into
masterfrom
hsd/issue-113
Closed

hsd-bot wants to merge 2 commits into
masterfrom
hsd/issue-113

Conversation

@hsd-bot

@hsd-bot hsd-bot commented Oct 30, 2026 •

Copy link
Copy Markdown
Collaborator

Closes #113

Summary

  • npm test can run against a built file: IMAGEDIFF_LIBRARY=<path from repo root> makes the Node specs require that file (including the load specs: no canvas, strict-mode wrapper) and makes the browser spec page load it as a plain <script> global. If it's unset, the specs use js/imagediff.js as before.
  • npm run test:release is the one release command:
    1. npm run build
    2. IMAGEDIFF_LIBRARY=imagediff.js npm test
    3. IMAGEDIFF_LIBRARY=imagediff.min.js npm test. Running the same suite against both files is how "the minified file behaves the same" is checked.
    4. git diff --exit-code -- imagediff.js imagediff.min.js, which fails if the fresh build differs from the committed build outputs.
  • README documents both commands, and a new CONTRIBUTING "Releasing" section says to run npm run test:release on the release PR before publishing.

Decisions

  • Env var, not an npm test argument (per review): npm test -- arg only reaches the last command in test:node && test:browser, so a single env var is the simplest way to retarget both.
  • Committed-match check via git diff: test:release is only meant for release PRs. On other branches it rebuilds the files in the working tree and fails at the diff step, which is the expected result there.
  • CLI coverage: bin/imagediff requires ../js/imagediff.js (since build: rebuild committed build outputs only at release #111), so the published CLI runs the source, not a built file. npm test already runs the CLI specs, and they do so in both test:release runs, but they exercise the source whichever build is selected. I didn't change what the CLI loads.
  • chore(release): 2.0.0 #101 (release ticket): its "packed tarball verified" step should run npm run test:release. I didn't edit chore(release): 2.0.0 #101 itself because it is outside this ticket.

Testing (in the dev container)

  • After npm run build, IMAGEDIFF_LIBRARY=imagediff.js npm test and IMAGEDIFF_LIBRARY=imagediff.min.js npm test each pass: 44 Node specs and 39 browser specs.
  • git diff --exit-code -- imagediff.js imagediff.min.js is clean after the build: the committed outputs match. I ran this step on the host because git can't see this worktree's repository from inside the container.
  • IMAGEDIFF_LIBRARY=nope.js fails in both Node and the browser, so the variable really switches targets.
  • npm test with the variable unset still passes against the source.

🤖 Generated with Claude Code

Add `npm run test:release`, which builds imagediff.js and imagediff.min.js
fresh from the files npm would publish and runs the Node, CLI and browser
specs against each. With --committed it also checks that the committed
build outputs match the fresh build.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@hsd-bot

hsd-bot commented Oct 30, 2026

Copy link
Copy Markdown
Collaborator Author

hsd review round 1

Verdict: approve. This does what #113 asks.

Checked in the dev container (hsd-js-imagediff-issue-113):

  • npm run test:release -- --committed passes. For each of imagediff.js and imagediff.min.js it runs 44 Node specs, including LoadSpec and the CLI, and 39 browser specs. Both committed outputs match the fresh build.
  • The env vars do switch targets. Running IMAGEDIFF_SCRIPT=.release/nope.js npm run test:browser fails, and so does IMAGEDIFF_LIBRARY=.../nope.js npm run test:node.
  • npm test against the source still passes.
  • The worktree stays clean after a run, since .release/ is gitignored.

Against the issue:

  • Release files are tested as a browser global, through require (the unminified build loads through the package main), and through the CLI. ✅
  • The minified file runs the same full suite as the unminified one. ✅
  • --committed compares the committed outputs to a fresh build byte for byte. ✅
  • README and CONTRIBUTING both document the command. ✅ chore(release): 2.0.0 #101 still needs its line added by hand, as the PR notes.

Nits (not blocking):

  • The CLI check runs the staged bin/imagediff, which requires js/imagediff.js (the source), not the built file. That matches what users actually get, and the PR explains it. Just note that "through the CLI" here checks the published package layout, not the build output.
  • npm pack --dry-run lists .devcontainer/, .github/, CLAUDE.md, spec/ and other dev-only files because there's no files field or .npmignore. This predates the PR and is out of scope, but it's worth a follow-up before publishing 2.0.
  • In spec/index.html, the ?lib= value goes into document.write unescaped. That's fine for a local test page.

@hsd-bot

hsd-bot commented Oct 30, 2026

Copy link
Copy Markdown
Collaborator Author

hsd: ready for human review after 1 review round(s). Agent sessions are still live in tmux -L hsd-js-imagediff attach -t issue-113-test-smoke-test-the-built-release-files.

Comment thread make/release-check.js Outdated
Comment on lines +1 to +7
// Smoke-tests the release files before publishing (npm run test:release).
//
// Copies the files npm would publish into .release/, builds imagediff.js and
// imagediff.min.js there, and runs the specs against each build the way users
// load it: through `require` in Node, as a <script> global in Chromium, and
// through the imagediff CLI. With --committed (on a release PR), it also
// checks that the committed build outputs match the fresh build.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we just test the built files before committing, could all of this be an argument to npm run test which points to different sources?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hsd: Yes. That's simpler, so I've changed it in 6c4f906:

  • make/release-check.js and the .release/ staging are gone.
  • npm test takes IMAGEDIFF_LIBRARY=<path from repo root> and runs the same Node and browser specs against that file instead of js/imagediff.js, e.g. IMAGEDIFF_LIBRARY=imagediff.min.js npm test. It's an env var rather than a CLI argument because npm test -- arg only reaches the last command in test:node && test:browser.
  • npm run test:release is now npm run build, then npm test against imagediff.js and imagediff.min.js, then git diff --exit-code on the two builds. That last step covers the issue's "committed outputs match a fresh build" requirement, and it replaces the --committed flag.

One thing doesn't change: bin/imagediff loads js/imagediff.js, so the CLI specs exercise the source whichever build is selected.

Replace make/release-check.js and its .release/ staging with one
IMAGEDIFF_LIBRARY variable that runs the Node and browser specs against a
built file. test:release builds, runs npm test against both builds, and
fails if the build changed the committed outputs.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@hsd-bot

hsd-bot commented Oct 30, 2026

Copy link
Copy Markdown
Collaborator Author

hsd review round 2

Verdict: approve. 6c4f906 makes the change @cesutherland asked for: the built file is now just a different target for npm test, chosen with IMAGEDIFF_LIBRARY. make/release-check.js and the .release/ staging are gone. This is simpler, and the issue is still covered.

Checked in the dev container (hsd-js-imagediff-issue-113), at 6c4f906:

  • After npm run build, both IMAGEDIFF_LIBRARY=imagediff.js npm test and IMAGEDIFF_LIBRARY=imagediff.min.js npm test pass, with 44 Node specs and 39 browser specs each.
  • Setting IMAGEDIFF_LIBRARY=nope.js fails both test:node and test:browser, so the variable really changes which file is tested.
  • npm test with the variable unset still passes against the source.
  • On the host, git diff --exit-code -- imagediff.js imagediff.min.js is clean after the rebuild, so the committed outputs match a fresh build.

Against the issue: one command (npm run test:release) tests both built files as a page global and through require, runs the CLI specs, gives the minified file the same suite as the unminified one, and fails if the fresh build differs from the committed files. The README and CONTRIBUTING both document it. ✅

Nits (not blocking):

  • The full npm run test:release won't run inside the dev container for an hsd worktree, because git there can't see the repository. That's specific to this setup; it's fine in a normal clone.
  • The VAR=x cmd syntax in the test:release script only works in POSIX shells, so it won't work under Windows cmd. That's acceptable for a maintainer-only release script.
  • IMAGEDIFF_LIBRARY must be a path relative to the repository root. The specs join it onto the repo root with path.join, so an absolute path won't work. This is documented.
  • The Node specs load the built file by its path, not through the package's main. Since main is ./imagediff.js, the result is the same.
  • Carried over from round 1: the CLI always loads the source, whichever file is selected (the PR says so), and the published package still includes dev-only files (worth a follow-up ticket).

@hsd-bot

hsd-bot commented Oct 30, 2026

Copy link
Copy Markdown
Collaborator Author

hsd: ready for human review after 1 review round(s). Agent sessions are still live in tmux -L hsd-js-imagediff attach -t issue-113-test-smoke-test-the-built-release-files.

@cesutherland

Copy link
Copy Markdown
Member

Closing for a re-write of #113.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

build: add an npm release command that builds and validates

2 participants