Skip to content

✨ feat: add a go test check to the go toolchain - #36

Closed
yeazelm wants to merge 1 commit into
mainfrom
matt/pcc-1157-go-toolchain-test-check
Closed

✨ feat: add a go test check to the go toolchain#36
yeazelm wants to merge 1 commit into
mainfrom
matt/pcc-1157-go-toolchain-test-check

Conversation

@yeazelm

@yeazelm yeazelm commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Summary

The toolchain checks tidiness and vet but never runs the suite. Four repos pin
it — tenant-api, export-cassette, search-cassette, skills-cassette — and none
of them run go test on a pull request; for a repo whose only other CI job
builds an image, that means no gate on its tests at all. Three of those repos
are public, so an outside contributor's PR shows green with no suite executed.

CheckGoTest joins the two existing checks, running in the container this
module already builds.

Why here rather than in each consumer's CI

Not only to avoid four copies. A suite run through a Nix shell writes its test
binary under that shell's TMPDIR, and on a GitHub runner the binary was not
there at exec time:

fork/exec /tmp/nix-shell.57IBNC/go-build3001282519/b001/export-cassette.test: no such file or directory

That is a failure about the environment wearing the costume of a failure about
the code. The container has its own filesystem and the question does not arise.

-count=1 disables the test cache, because a check that a previous run's
cached result can satisfy is not running the thing it reports on. The mounted
build cache keeps compilation incremental regardless.

The fixture change

go test ./... reports success against a package with no test files, so the
empty fixture would have proven only that the command exits zero. It gains a
function and a test for it, and the check's output now shows the suite
actually ran:

✔ .checkGoTest: String!
ok  	example.com/fixture/app	0.001s

Rollout

Adoption is per-repo and opt-in: consumers pin this module by SHA, so no
repo starts gating on its tests until it re-pins. Re-pinning the four
consumers is follow-up work, and some may have work to do before their suites
are green — which is the point.

Test plan

  • make -C go test green, with all three checks run against ./fixtures
  • checkGoTest output shows ok example.com/fixture/app, confirming it
    compiled and ran a test rather than reporting "no test files"
  • Re-pin the four consumers, one PR each

Part of PCC-1253

The toolchain checked tidiness and vet but never ran the suite, so a repo
whose only other CI was an image build had no pull-request gate on its tests
at all — four repos pin this toolchain and none of them ran `go test`.

Running it here rather than leaving it to each consumer's CI is not only
about duplication. A suite run through a Nix shell writes its test binary
under that shell's TMPDIR, and a binary that is not there at exec time fails
as "fork/exec ...: no such file or directory" — a failure about the
environment wearing the costume of a failure about the code. The container
this module already builds has its own filesystem and the question does not
arise.

-count=1 disables the test cache, because a check that a previous run's
cached result can satisfy is not running the thing it reports on; the
mounted build cache is what keeps that affordable.

The fixture gains a function and a test for it. `go test ./...` reports
success against a package with no test files, so an empty fixture would have
proven only that the command exits zero.
@linear-code

linear-code Bot commented Aug 20, 2026

Copy link
Copy Markdown

PCC-1157

PCC-1253

@greptile-apps

greptile-apps Bot commented Aug 20, 2026

Copy link
Copy Markdown

Greptile Summary

The PR adds an opt-in Go test check alongside the existing module-tidiness and vet checks.

  • Runs go test -count=1 ./... inside the existing Go toolchain container.
  • Adds the check to the Go module’s Makefile test target.
  • Expands the fixture with a real function and unit test so the new check demonstrably executes a test.

Confidence Score: 5/5

The PR appears safe to merge, with no concrete blocking or non-blocking defects identified.

The new command follows the established check and CLI conventions, executes within the existing source-mounted container, and is covered by a valid fixture test.

Important Files Changed

Filename Overview
go/main.go Adds a +check method that runs the uncached Go test suite using the same container setup as the existing checks.
go/Makefile Extends the test target to invoke the newly exposed check-go-test command.
go/fixtures/app/app.go Adds a deterministic fixture function for compilation and test execution.
go/fixtures/app/app_test.go Adds a focused unit test proving that the toolchain check executes an actual test binary.

Reviews (1): Last reviewed commit: "✨ feat: add a go test check to the go to..." | Re-trigger Greptile

@yeazelm

yeazelm commented Aug 20, 2026

Copy link
Copy Markdown
Contributor Author

Closing: the premise was wrong.

This was opened on the claim that four repos pin the go toolchain and none
of them run go test. Two of the three cassettes already do, and they do it
the right way — a repo-local Test function carrying +check in their own
.dagger module, reported as search-cassette:test and
skills-cassette:test on every pull request. tenant-api has the same shape.
Only export-cassette lacked one, and it now has export-cassette:test
(papercomputeco/export-cassette#8).

Adding the check here would not have helped any of them, which is the more
useful finding. Every repo that runs Go tests needs a different container:
search-cassette binds a Postgres service and sets GOEXPERIMENT=jsonv2,
tenant-api pins envtest control-plane binaries alongside its own Postgres,
skills-cassette adjusts PATH. A generic go test ./... in the shared
toolchain serves none of those, so they would all have kept their own
regardless — and the one repo whose needs are plain has its own now too.

The +check pragma plus a per-repo Test function is the convention here,
and it is the right one, because test environments are a property of the repo
rather than of the language.

@yeazelm yeazelm closed this Aug 20, 2026
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.

1 participant