Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

doc: introduce a page for CI best practices #42119

Open
mvdan opened this issue Oct 21, 2020 · 19 comments
Open

doc: introduce a page for CI best practices #42119

mvdan opened this issue Oct 21, 2020 · 19 comments
Labels
Documentation NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@mvdan
Copy link
Member

mvdan commented Oct 21, 2020

Most CI scripts and defaults do just the basics: go test ./...

There are many more commands that likely make sense for a majority of projects, and some are well known, but we don't have a single place to document and maintain them well. For example:

  • Enforce gofmt from a specific version of Go, e.g. test -z $(gofmt -l .)
  • Use go test -race ./... to catch data races in tests
  • Use go vet ./..., since go test only runs a subset of go vet
  • Use go mod tidy and ensure that there aren't changes to go.mod or go.sum (via go mod tidy -mod=readonly maybe?)
  • Use go mod verify to ensure that go.sum agrees with what's in the module cache
  • Use go test -run=- -bench=. -benchtime=1x ./... to catch stale benchmarks (see proposal: cmd/go: run benchmarks for one iteration when testing #41824)

And potentially staticcheck ./... too, since it's now sort of recommended by gopls as an extension to go vet. Though note that this should be a small set of recommendations that most projects should follow, so it shouldn't include an endless list of linters that might be interesting to some users.

We should include a ready-to-use script containing all of the recommended steps, for example in POSIX Shell. We could document each step directly via script comments, meaning that the entire thing could be copy-pasted and self-documenting.

I'm happy to kickstart this work during the freeze. /cc @bcmills @jayconrod @myitcv @jadekler

@mvdan
Copy link
Member Author

mvdan commented Oct 21, 2020

The reason I want this to be a doc on the main site or the main repo is two-fold: so that it's code reviewed (so the wiki doesn't work), and so that it can be maintained and updated over time (so a blog post doesn't work).

@bcmills
Copy link
Contributor

bcmills commented Oct 21, 2020

CC @stevetraut

@bcmills bcmills added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Oct 21, 2020
@bcmills bcmills added this to the Backlog milestone Oct 21, 2020
@mvdan
Copy link
Member Author

mvdan commented Nov 18, 2020

Friendly bump - who should I get the green light from for this document? I think the freeze is probably the best time to work on docs, so I'd like to start writing this doc before the end of the year.

If no green light is needed I can just start writing and send a CL, but I'd rather not do that if the decision will likely be "we don't want this doc in the website" :)

@jayconrod
Copy link
Contributor

Our tech writer @stevetraut would be the best person to coordinate with. He's in the process of writing a programmer's guide to Go with many different topics. I think CI best practices would fit into that nicely.

@stevetraut
Copy link

Sorry for the late response, folks.

@mvdan Thanks for the idea! I think it could be useful content. I'm not sure it's within the "programming with Go" focus that the programmer's guide Jay mentioned has, but content aimed at team development would be useful. Currently, though, the guide is still taking shape and we're not sure yet what's in and what's out.

May I suggest that you add it to the wiki? That would get it in front of folks right away. And I'm sure I'll be collecting other content and ideas from the wiki along the way, and may add a topic on CI.

@rsc

@mvdan
Copy link
Member Author

mvdan commented Nov 18, 2020

Thanks for the response. I honestly don't think the wiki is the best place, because it's not code-reviewed and anyone can edit it, so it's not a good place to document best practices. That is the reason why I opened #34038 last year, though unfortunately the process is taking a long time. That's why I was hoping this new document could be added directly to a git repository or the main site.

I could always start writing it while we decide where it goes, but I still think it has relatively less value (and can't be properly maintained) if it ends up in the wiki.

@rsc
Copy link
Contributor

rsc commented Nov 19, 2020

@mvdan, you're right that the wiki isn't the right long-term place, and we agree. But we have limited attention, and right now this specific page is not at the top of our list. We don't even have the framework set up for where the page would properly go. So if you want to publish something now, the wiki is still the best place. Long term, we'll have a better answer.

@mvdan
Copy link
Member Author

mvdan commented Mar 22, 2021

Use go mod tidy and ensure that there aren't changes to go.mod or go.sum (via go mod tidy -mod=readonly maybe?)

Unfortunately go mod tidy -mod=readonly doesn't work, so the current workarounds are either assuming git diff works, or using a bit of cp and diff. So I think #27005 should be given some priority for the sake of CI best practices.

KlotzAndrew added a commit to TestRecall/reporter that referenced this issue Apr 3, 2021
golang/go#42119

Signed-off-by: Andrew Klotz <agc.klotz@gmail.com>
@mvdan
Copy link
Member Author

mvdan commented May 20, 2021

Given that we'll get go mod tidy -check, gofmt will remain as the only command that needs extra machinery (e.g. shell code) to do the right thing here. So I've raised #46289 as an attempt to reconsider some mode for gofmt to tell us, directly, if any files require changes.

@mvdan
Copy link
Member Author

mvdan commented May 25, 2021

Another interesting point is 1.17's go test -shuffle. I think we should recommend its use in CI, similar to -race.

@mvdan
Copy link
Member Author

mvdan commented Jun 1, 2021

Yet another idea, as a bonus: ensure that go test module-path/... works even when the module in question is not the main module. For example, using a temporary go.mod file that simply requires our module and replaces it to the right directory. This would ensure that the module works as a dependency too, i.e. without directives like replace or exclude.

@myitcv
Copy link
Member

myitcv commented Jun 1, 2021

A question/suggestion (depending whether it's in scope or not): would this guide also provide guidance on how to work with multi-module repositories? In particular, dealing with the fact that go test ./... does not cross module boundaries, and therefore how to work with that?

@mvdan
Copy link
Member Author

mvdan commented Jun 1, 2021

Probably, yes, though I think the first version should aim to tackle the common case: a single module.

Ideally all of this would be "best practices for testing a module in CI", but since most CI systems work at the VCS repository level, I think we'll need to bridge the gap a little with extra recommendations.

@mvdan
Copy link
Member Author

mvdan commented Jun 29, 2022

Judging by Steve's Gerrit profile, it seems like he's left Google. Who is the next tech writer, or whomever I should coordinate these efforts with? I'd like to contribute documentation which is sorely needed in my opinion, like this thread and #48539, and for the past few years I haven't been able to find anyone to help make that happen.

@mvdan
Copy link
Member Author

mvdan commented Mar 18, 2023

Another one that pops to mind: using -trimpath in CI is probably a good idea as well. Full paths are usually not useful when looking at CI logs, because it's not a filesystem one can typically access directly. And using -trimpath helps with reproducible builds, which should help with keeping build and test caches more reusable between CI runs.

@mvdan
Copy link
Member Author

mvdan commented Mar 22, 2023

Another one we figured out with @myitcv: go test -count=1 ./... can be a good way to ensure that all tests are run, without any test caching happening. This can help catch flaky tests quicker, for example if one test only fails 1% of the time.

We wanted a setup where pushes to master ran all the tests, but PRs/CLs being tested would reuse master's test cache to speed up iteration and testing on patches, just like we already did with the build and module caches. In this scenario, using go test -count=1 ./... in master is not quite right, because it disables the test cache entirely - test cache entries are not filled in at all. To ensure that go test ./... in master runs all the tests and fills the test cache, we're now using go clean -testcache && go test ./..., which works well.

@mvdan
Copy link
Member Author

mvdan commented May 23, 2023

Yet another idea: another command that we want to ensure results in zero changes, like gofmt or go mod tidy, is go fix ./.... I was still carrying // +build lines in a handful of projects that had dropped support for Go 1.16 or older over a year ago.

@mvdan
Copy link
Member Author

mvdan commented Sep 28, 2023

One more idea: for monorepos with checked in go.work files, one likely wants to ensure that:

  • go work sync results in zero changes
  • All the usual checks (go test ./..., go vet ./..., go mod tidy, ...) are happy on each of the modules, e.g. via a loop using -C moddir
  • For each module that's meant to be used externally (e.g. not internal), GOWORK=off go test ./... is happy, to check that the module still works without the workspace mode

@eccles
Copy link

eccles commented Mar 6, 2024

great to know what good practice is. I usually run go mod tidy and go mod verify as part of any PR. Also what is the effect of not keeping go.mod up to date? I recently started working with a repo where the go.mod files did not reflect reality but this appeared to do no harm. Why and when cmds like go mod tidy/verify are executed would be useful

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

8 participants