Repository navigation
ci: build, vet and test this repository - #296
Merged
Merged
Conversation
There was no CI. No .github/workflows directory, and none in the history. Every change to the indexer serving mainnet merged without being compiled, vetted or tested by anything but whoever wrote it, including the three fixes open right now. The suite could not have run anyway. Thirteen tests in pkg/analyzer build an analyzer against a hardcoded http://localhost:5052 and call t.Errorf when nothing answers, so `go test ./...` was red on any machine without a beacon node. A suite that cannot tell an absent prerequisite from a defect is one nobody can run, and CI would have been red from its first minute. They take their endpoints from GOTETH_TEST_BN_ENDPOINT and GOTETH_TEST_EL_ENDPOINT now, defaulting to the local ones, and skip when nothing answers - matching what goteth-v4 already does. Checked that the skip is what makes them green rather than an empty stub: turn it into a log and all thirteen fail again. Two consequences of removing the builder's err, both preserving existing behaviour rather than changing it. requireNodes returns a pointer, because ChainAnalyzer holds an atomic.Bool and returning a copy trips vet's copylocks check. And several RequestBeaconBlock calls were reusing that err without ever reading it, so they now discard it explicitly. Those tests were already ignoring a request error and asserting on the block regardless; that is untouched here and worth its own look. go build, go vet and go test -race all pass on a clean checkout.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This repository has no CI. Not "no checks on some branches" - no
.github/workflowsdirectory, and none anywhere in its history.Every change to the indexer serving mainnet has merged without being compiled, vetted or tested by anything other than whoever wrote it. That includes #289, #293 and #294, open right now.
The suite could not have run anyway
Thirteen tests in
pkg/analyzerbuild an analyzer against a hardcodedhttp://localhost:5052and callt.Errorfwhen nothing answers. Sogo test ./...was red on any machine without a beacon node, and CI would have been red from its first minute - which is the surest way to have a job everyone learns to ignore.A suite that cannot tell an absent prerequisite from a defect is one nobody can run. They take their endpoints from the environment now, defaulting to the local ones, and skip when nothing answers:
That matches what goteth-v4 already does, so the two repositories behave the same way.
I checked the skip is what makes them green rather than an empty stub: turn the
t.Skipfinto at.Logfand all thirteen fail again.Two consequences worth reading
Both preserve existing behaviour rather than change it.
requireNodesreturns a pointer, becauseChainAnalyzerholds anatomic.Booland handing back a copy trips vet's copylocks check. I introduced that error and then fixed it; vet was clean before and is clean now.Several
RequestBeaconBlockcalls were reusing the builder'serrvariable without ever reading it. With the builder gone they discard it explicitly instead. Those tests were already ignoring a request error and asserting on the block regardless, which would be a nil dereference if the request failed. I have left that exactly as it was rather than change test semantics in a CI change, but it is worth its own look.The workflow
Build, vet,
go test -race, on pushes tomain,masteranddevand on every pull request.submodules: recursive, sincego.modreplacesgo-relay-clientwith a local path.All three steps pass on a clean checkout.
Note on branches
It triggers on
masteranddevboth. Those have diverged -devis 32 commits ahead,masterhas 13 thatdevdoes not - and this does not attempt to resolve that, only to cover whichever is being worked on.