From 818f8bf65b8752f27601c7176a173003873eae9d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Mart=C3=AD?= Date: Wed, 18 Sep 2024 15:13:34 +0100 Subject: [PATCH] internal/ci: `go generate` after `go test` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Some packages like ./encoding/jsonschema use `go generate` to produce testdata files, which is an entirely reasonable use case. Because these files get overwritten every time, getting new modtimes, this causes `go test ./...` to treat the test input files as changed, and so CI never reuses a cached test run of the jsonschema package. For this reason, as well as to avoid confusion with developers looking at CI failures (which has happened twice already), test and check first, and generate after. In the happy case, where code generation is clean, this results in no change in behavior other than more test cache hits. In the case where the user forgot to re-run `go generate`, CI will still fail, just a little bit later, which is fine. Signed-off-by: Daniel Martí Change-Id: I2635095eec950d392f5ccb6da2147e20f7c83584 Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/1201440 Reviewed-by: Roger Peppe TryBot-Result: CUEcueckoo Unity-Result: CUE porcuepine --- .github/workflows/trybot.yaml | 6 +++--- internal/ci/github/trybot.cue | 7 ++++++- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/.github/workflows/trybot.yaml b/.github/workflows/trybot.yaml index 871701b2c93..c609cbba81c 100644 --- a/.github/workflows/trybot.yaml +++ b/.github/workflows/trybot.yaml @@ -110,9 +110,6 @@ jobs: - name: Early git and code sanity checks if: (matrix.go-version == '1.23.x' && matrix.runner == 'ubuntu-22.04') run: go run ./internal/ci/checks - - name: Generate - run: go generate ./... - if: (matrix.go-version == '1.23.x' && matrix.runner == 'ubuntu-22.04') - name: Test if: |- ((github.ref == 'refs/heads/master' || startsWith(github.ref, 'refs/heads/release-branch.')) && (! (contains(github.event.head_commit.message, ' @@ -169,6 +166,9 @@ jobs: echo "Did you forget about refs/attic branches? https://github.com/cue-lang/cue/wiki/Notes-for-project-maintainers" exit 1 fi + - name: Generate + run: go generate ./... + if: (matrix.go-version == '1.23.x' && matrix.runner == 'ubuntu-22.04') - name: Check that git is clean at the end of the job if: always() run: test -z "$(git status --porcelain)" || (git status; git diff; false) diff --git a/internal/ci/github/trybot.cue b/internal/ci/github/trybot.cue index 858a655c018..852d269ecef 100644 --- a/internal/ci/github/trybot.cue +++ b/internal/ci/github/trybot.cue @@ -67,7 +67,6 @@ workflows: trybot: _repo.bashWorkflow & { // so we only need to run them on one of the matrix jobs. if: _isLatestLinux }, - _goGenerate, _goTest & { if: "\(_repo.isProtectedBranch) || !\(_isLatestLinux)" }, @@ -78,6 +77,12 @@ workflows: trybot: _repo.bashWorkflow & { for v in _e2eTestSteps {v}, _goCheck, _checkTags, + // Run code generation towards the very end, to ensure it succeeds and makes no changes. + // Note that doing this before any Go tests or checks may lead to test cache misses, + // as Go uses modtimes to approximate whether files have been modified. + // Moveover, Go test failures on CI due to changed generated code are very confusing + // as the user might not notice that checkGitClean is also failing towards the end. + _goGenerate, _repo.checkGitClean, ] }