Skip to content

Add in-house snapshot testing and convert output assertions to snapshots - #443

Open
skyrpex wants to merge 23 commits into
mainfrom
claude/go-testing-tools-11f5d0
Open

Add in-house snapshot testing and convert output assertions to snapshots#443
skyrpex wants to merge 23 commits into
mainfrom
claude/go-testing-tools-11f5d0

Conversation

@skyrpex

@skyrpex skyrpex commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Motivation

Exploration of modern Go testing tooling for more readable tests: many integration tests pinned CLI output shape through long Contains/NotContains chains that are hard to read, easy to under-assert, and painful to update.

Solution

  • internal/snap: in-house file-snapshot helper, no new dependencies. One archive per test file (__snapshots__/<test_file>.snap) in a plain bracket-header format: a [TestName_N] header per Match call, the value verbatim, a --- terminator line, blank-line separated. The format has no escaping, so a value containing a bare --- line is rejected (round-trip guard, covered by tests). Match for text, MatchJSON with dotted-path masking (<any>) for JSON envelopes, Clean for obsolete-entry/orphan-archive detection via TestMain. Missing snapshots are created locally but fail in CI; UPDATE_SNAPS=true go test rewrites.
  • Assertions stay testify, matching main's existing require-for-preconditions / assert-for-expectations split (an in-house must package was trialed mid-PR and reverted).
  • Mass conversion of output assertions to snapshots (~220 test sites across ~35 integration test files plus internal/output): every Contains/NotContains chain on CLI output whose full stream is deterministic across runs and platforms is now a snapshot. Streams are always snapshotted separately (one Match per stream, never stdout+stderr concatenated). Deliberately left as Contains: Docker-backed tests (volatile pull/timing output), streams embedding temp paths or host-dependent text (runtime-detection hints, platform dial errors), PTY/Bubble Tea transcripts, and platform-gated tests whose snapshots can't be generated cross-platform.
  • Test-env hardening found by the conversion: a verification pass running the suite under a polluted shell (AWS_PROFILE, AWS_REGION, real-looking keys, endpoint overrides) showed ambient AWS env leaking into wrapped-tool env dumps and snapshotted output. The test/integration/env base constructors now strip ambient AWS_* credential/region/profile vars plus AWS_ENDPOINT_URL/LSTK_ENDPOINT_URL (same spirit as the existing never-inherit-real-$HOME rule); tests exercising ambient behavior set them explicitly, which still wins.
  • Bugs the snapshots caught: lstk-extensions.toml listed as a phantom extensions extension in --help on Windows (fixed in internal/extension/resolve.go — listing now honors PATHEXT like dispatch already did); a CI failure diff leaked the real auth token into public artifacts (fixed by stripping ambient tokens in test envs + sanitizer redaction; label-anchored sanitizer masks ports, hosts, versions, dates, sizes).
  • .gitattributes: *.snap -text so Windows checkouts can't CRLF-mangle archives.

Docs

Docs impact

Nothing user-facing to document: test infrastructure and test-only changes. Contributor-facing usage (snap.Match, UPDATE_SNAPS, sanitization rules, per-stream convention) is documented in CLAUDE.md's Testing section.

Review

Human review advisable: this introduces shared test infrastructure (internal/snap) and a repo-wide testing convention, and the mass conversion intentionally deletes NotContains checks wherever a full-stream snapshot subsumes them — worth a second pair of eyes on the kept-vs-converted judgment calls. All 176 snapshot-backed tests pass (also under a deliberately polluted AWS environment), unit tests and lint are green on both modules.

Co-Authored-By: Claude noreply@anthropic.com

🤖 Generated with Claude Code

@skyrpex skyrpex added semver: patch docs: skip Pull request does not require documentation changes labels Aug 10, 2026
@skyrpex skyrpex changed the title Adopt shoenig/test and go-snaps in output package tests Add in-house test assertion and snapshot helpers Aug 11, 2026
skyrpex and others added 2 commits August 11, 2026 14:43
Co-Authored-By: Claude <noreply@anthropic.com>
@skyrpex
skyrpex force-pushed the claude/go-testing-tools-11f5d0 branch from cd92645 to f3cd58b Compare August 11, 2026 12:43
skyrpex and others added 13 commits August 11, 2026 14:45
Co-authored-by: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Re-applies the testify-to-must migration and snapshot conversions on top of
the Windows PTY/fake-tool test rework from #444.

Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
…utput

Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
@skyrpex skyrpex changed the title Add in-house test assertion and snapshot helpers Add in-house snapshot testing and convert output assertions to snapshots Aug 12, 2026
skyrpex and others added 6 commits August 12, 2026 13:36
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
# Conflicts:
#	test/integration/logout_test.go
#	test/integration/logs_test.go
#	test/integration/status_test.go
@skyrpex
skyrpex marked this pull request as ready for review August 12, 2026 15:18
@skyrpex
skyrpex requested review from a team and peter-smith-phd as code owners August 12, 2026 15:18

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

In-house file-snapshot helper plus a mass Contains→snapshot conversion; careful, well-documented, and it caught two real bugs along the way — I ran the non-Docker packages (internal/snap, internal/output, internal/extension) locally and they pass.

  1. suggestion(non-blocking): on test/integration/json_envelope_test.go — the conversion replaces named assertions (assert.Equal("NOT_JSON_CAPABLE", ...)) with full-envelope MatchJSON. I confirmed the .snap files pin code/category/status/schemaVersion, so this is strictly stronger for the agent-facing contract; the only cost is the failure surface shifting from a named expectation to a diff. This is exactly the kept-vs-converted judgment your Review note asks a human to double-check.
  2. thought(non-blocking): on test/integration/snap_sanitize_test.go — a few masks are shape-anchored across the whole stream (sanitizeSizeRe, and sanitizeGatewayHostRe masking any host on :4566). Correct today since those values are environment-dependent, but if a test ever needs to assert a specific size or endpoint host on :4566 it would be silently masked — worth a comment or a narrower anchor if that case appears.
  3. praise: the env hardening + defense-in-depth AUTH_TOKEN= redaction, and the two bugs the conversion surfaced (the token leaking into public CI artifacts and the Windows phantom extensions extension in internal/extension/resolve.go, now correctly gated on PATHEXT to match dispatch) are a strong argument for the whole exercise.

Automated review on behalf of @gtsiolis.


Generated by Claude Code

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Re-review of the delta since my last pass — a main merge brought in the new extension MACHINE_ID field and be4fbca masks it in the snapshots, so my earlier shape-anchored-mask thought is now addressed inline.

  1. praise: on test/integration/snap_sanitize_test.go — the MACHINE_ID mask correctly folds into the existing (?:SESSION|MACHINE)_ID= anchor and documents why the value is environment-dependent (origin prefix + host hash); I confirmed via the API that every MACHINE_ID= line in the committed snapshots renders as <id>, so cross-host/CI determinism holds.

Automated review on behalf of @gtsiolis.


Generated by Claude Code

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

Labels

docs: skip Pull request does not require documentation changes semver: patch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant