-
Notifications
You must be signed in to change notification settings - Fork 440
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
circleci: record junit test results #959
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments to help review.
contrib/gofiber/fiber.v2/fiber.go
Outdated
@@ -43,7 +43,7 @@ func Middleware(opts ...Option) func(c *fiber.Ctx) error { | |||
opts = append(opts, cfg.spanOpts...) | |||
span, _ := tracer.StartSpanFromContext(c.Context(), "http.request", opts...) | |||
|
|||
fmt.Printf("Starting Span") | |||
fmt.Printf("Starting Span\n") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is needed to make gotestsum happy. Otherwise we get failures parsing the test output.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably just remove this line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DONE. 4732d34
destination: raw-test-output | ||
|
||
- store_test_results: # upload test results for display in Test Summary | ||
path: /tmp/test-results |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The boilerplate above is duplicated for our two main jobs: test-core
and test-contrib
. We could probably try to eliminate it via some YAML anchors, but given the existing duplication I'm not sure it's worth it?
@@ -198,7 +224,20 @@ jobs: | |||
- run: | |||
name: Testing integrations | |||
command: | | |||
INTEGRATION=1 go test -v -race -coverprofile=coverage.txt -covermode=atomic `go list ./contrib/... | grep -v -e grpc.v12 -e google.golang.org/api` | |||
PACKAGE_NAMES=$(go list ./contrib/... | grep -v -e grpc.v12 -e google.golang.org/api | circleci tests split --split-by=timings --timings-type=classname) | |||
gotestsum --junitfile ${TEST_RESULTS}/gotestsum-report.xml -- $PACKAGE_NAMES -v -race -coverprofile=coverage.txt -covermode=atomic |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't cover our special Testing outlier google.golang.org/api
and Testing outlier gRPC v1.2
steps right now. Not sure if this can be done easily given the way CircleCI handles JUnit XML results, but I could try to look into it if you think it's critical.
- run: mkdir -p $TEST_RESULTS | ||
- restore_cache: # restores saved cache if no changes are detected since last run | ||
keys: | ||
- go-mod-v4-{{ checksum "go.sum" }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The extra restore_cache
and save_cache
are documented in the Go language guide. It's not clear if they are needed for capturing the test results, it seems to work without, but in theory it might speed up our tests a little.
I'm also not sure if it's working correctly, given the presence of this in the CI logs. So we could also omit it from this PR for now as far as I'm concerned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a nit.
contrib/gofiber/fiber.v2/fiber.go
Outdated
@@ -43,7 +43,7 @@ func Middleware(opts ...Option) func(c *fiber.Ctx) error { | |||
opts = append(opts, cfg.spanOpts...) | |||
span, _ := tracer.StartSpanFromContext(c.Context(), "http.request", opts...) | |||
|
|||
fmt.Printf("Starting Span") | |||
fmt.Printf("Starting Span\n") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably just remove this line.
@knusbaum thanks for reviewing, I addressed the nit. PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Looks like an improvement.
This PR enables circle CI's Collecting Test Metadata feature by producing JUnit XML. It's modeled after their Language Guide: Go docs.
The motivation behind this is to get insights into flaky tests and to avoid having to scroll through the huge test result log output when a test fails. An example of this can be seen below or by following this link.
FWIW the Insights don't seem to work yet. I don't know if this will fix itself once we merge this or if there is another problem.