ci: add 'go test -race' #8269
ci: add 'go test -race' #8269
Conversation
dnephin
force-pushed
the
dnephin/ci-add-go-test-race
branch
from
c077a05
to
6bc675c
Compare
dnephin
added
the
type/ci
.circleci/config.yml
Outdated
| # but we can run a little over that limit. | ||
| steps: | ||
| - checkout | ||
| - run: *install_gotestsum |
There was a problem hiding this comment.
this will conflict with #8327 sorry!
There was a problem hiding this comment.
No worries! Luckily it does not appear to conflict
There was a problem hiding this comment.
It might not conflict but it won't work properly when merged so you'll have to rebase since install_gotestsum doesn't exist anymore in master
There was a problem hiding this comment.
Oh right! The alias is a dash now, not an underscore. I missed that! I'll rebase now
| --junitfile $TEST_RESULTS_DIR/gotestsum-report.xml -- \ | ||
| -tags="$GOTAGS" -p 2 \ | ||
| -race -gcflags=all=-d=checkptr=0 \ | ||
| ./agent/{ae,cache,checks,config,pool,proxycfg,router} \ |
There was a problem hiding this comment.
This is great! Even though we may be more prescriptive here, it allows us to incrementally fix package by package and get incremental improvements.
|
Were you able to fix the failures in your initial test of adding the data race check or did you just filter them out of the package list to test for now? Curious but in your testing did you see GOMAXPROCS have an affect on the race detector? ie: Does the overhead of running the race detector make any tests more flaky or OOM? |
No fixes yet. For now I filtered out the packages and tracked them in #8329. I wanted to enable this for the new
I did not experiment with GOMAXPROCS at all. I copied the setting from the |
Running every test with the race detector would add significant time to CI. That additionaltime won't provide much value as many of the integration tests use much of the same code. For now we can run -race on some of the smaller packages. As we move more code into smaller packages we should be able to add more packages to the list that runs with '-race'. For now this is running without parallelism, but we can enable that as well when we need it. boltdb fails the 'checkptr' check, which is automatically enabled by '-race', so I've disabled checkptr as well.
dnephin
force-pushed
the
dnephin/ci-add-go-test-race
branch
from
6bc675c
to
099000e
Compare
I tried
-raceon all the packages (example) but there were many failures. I expect it to add a lot of time to CI, but it actually ran in pretty reasonable time.Since there were over 100 failures, I'm hoping we can get this enabled on a smaller number of packages with a few fixes and opt more in over time.
-racealso enables-d=checkptr(ref), which fails in the boltdb package. It looks like this was still an issue with bbolt until very recently (etcd-io/bbolt#187, etcd-io/bbolt#201). So for now that check is disabled.There are still a bunch of failures in the smaller list of packages. I need to look over those failures to see if we should drop some of the packages from the list. Making this a draft PR until CI is green.