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.

Sign up for GitHub

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

Jump to bottom

Publish test report #5030

Merged
merged 14 commits into from
Dec 24, 2023
Merged

Publish test report #5030

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
7468c2c
Try publish unit tests
Dec 24, 2023
7f6e2d6
Add branch
Dec 24, 2023
40396b7
Convert to junit.xml report
Dec 24, 2023
d2e01fc
Install test tools
Dec 24, 2023
a26c734
Update permissions
Dec 24, 2023
6d002f4
Update test names
Dec 24, 2023
a53b45e
Remove test branch and add to go-tip
Dec 24, 2023
9943f11
Remove mkdir
Dec 24, 2023
ed69e54
Merge branch 'main' into test-report
albertteoh Dec 24, 2023
5cd1dc0
Echo command and remove id
Dec 24, 2023
396fb72
Remove event file
Dec 24, 2023
aa357c1
Remove upload to see what happens
Dec 24, 2023
2be51fa
Remove pull-request and check write permissions
Dec 24, 2023
e5827a6
Merge branch 'main' into test-report
albertteoh Dec 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions .github/workflows/ci-unit-tests-go-tip.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ on:
# See https://github.com/ossf/scorecard/blob/main/docs/checks.md#token-permissions
permissions: # added using https://github.com/step-security/secure-workflows
contents: read
checks: write
pull-requests: write

jobs:
unit-tests-go-tip:
Expand All @@ -22,11 +24,15 @@ jobs:
- name: Install Go Tip
uses: ./.github/actions/setup-go-tip

- name: Install tools
run: make install-test-tools

yurishkuro marked this conversation as resolved.
Show resolved Hide resolved
- name: Run unit tests
run: make test-ci

- name: Publish Unit Test Summary 📑
uses: EnricoMi/publish-unit-test-result-action@v2
if: always()
with:
check_name: Unit Tests Summary
junit_files: junit-report.xml

- name: Lint
run: echo skip linting on Go tip
26 changes: 26 additions & 0 deletions .github/workflows/ci-unit-tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ concurrency:
# See https://github.com/ossf/scorecard/blob/main/docs/checks.md#token-permissions
permissions: # added using https://github.com/step-security/secure-workflows
contents: read
checks: write
pull-requests: write
yurishkuro marked this conversation as resolved.
Show resolved Hide resolved

jobs:
unit-tests:
Expand All @@ -34,6 +36,20 @@ jobs:
- name: Run unit tests
run: make test-ci

- name: Publish Unit Test Summary 📑
uses: EnricoMi/publish-unit-test-result-action@v2
if: always()
with:
check_name: Unit Tests Summary
junit_files: junit-report.xml

- name: Upload Test Results
if: always()
uses: actions/upload-artifact@v3
with:
name: Test Results
path: junit-report.xml

- name: Setup CODECOV_TOKEN
uses: ./.github/actions/setup-codecov

Expand All @@ -45,3 +61,13 @@ jobs:
flags: unittests
fail_ci_if_error: true
token: ${{ env.CODECOV_TOKEN }}

event_file:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what does this do?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, good catch, that was me experimenting with https://github.com/EnricoMi/publish-unit-test-result-action/blob/v2.12.0/README.md#support-fork-repositories-and-dependabot-branches and was an accidental half-baked change. Reverted in 396fb72 and aa357c1.

That may have been why we could see the test report in https://github.com/jaegertracing/jaeger/actions/runs/7315855796?pr=5030#summary-19929936699, but I'll check once the test CI job is done.

name: "Event File"
runs-on: ubuntu-latest
steps:
- name: Upload
uses: actions/upload-artifact@v3
with:
name: Event File
path: ${{ github.event_path }}
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,3 +45,4 @@ deploy-staging/
sha256sum.combined.txt
resource.syso
.gocache
test-results.json
11 changes: 8 additions & 3 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ index-rollover-integration-test: docker-images-elastic

.PHONY: cover
cover: nocover
$(GOTEST) -tags=memory_storage_integration -timeout 5m -coverprofile cover.out ./...
$(GOTEST) -tags=memory_storage_integration -timeout 5m -coverprofile cover.out ./... > test-results.json
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@albertteoh what makes it a JSON file? If I change it to | tee test-results.json then I see the standard text output:

=== RUN   TestRegisterStaticHandler/basePath=/
=== RUN   TestRegisterStaticHandler/basePath=/jaeger
--- PASS: TestRegisterStaticHandler (0.03s)
    --- PASS: TestRegisterStaticHandler/basePath= (0.01s)

If it is meant to be std output, then I would suggest changing to |tee because it makes the CI logs show the test progress.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test-ci make target defines GOTEST as: GOTEST := $(GOTEST_QUIET) -json; so it's the extra -json flag that makes the output json: https://github.com/jaegertracing/jaeger/blob/main/Makefile#L460.

So the full command would resemble (with | tee):

go test -race -json -tags=memory_storage_integration -timeout 5m -coverprofile cover.out ./... | tee test-results.json

Adding a | jq . to the end of that, and the output looks like:

{
  "Time": "2023-12-27T21:40:21.085452+11:00",
  "Action": "pass",
  "Package": "github.com/jaegertracing/jaeger/storage/spanstore/metrics",
  "Elapsed": 3.848
}

It's a good callout though, because I hadn't considered the impact of this change on the stdout display of test results which are useful for debugging unit test runs. Maybe test summaries are not worth the trade-off for harder to read test output?

What do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. I think jq will be overkill, but changing to tee is good. The test-ci is not really meant to be run locally, but with -json flag the output is still readable

{"Time":"2023-12-27T10:16:06.335612-05:00","Action":"start","Package":"github.com/jaegertracing/jaeger"}
{"Time":"2023-12-27T10:16:06.72618-05:00","Action":"run","Package":"github.com/jaegertracing/jaeger","Test":"TestDummy"}

(btw jq wouldn't work at all because ^ is not a well-formed JSON, it has no top-level container).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, I've created a PR to use | tee: #5050.

(btw jq wouldn't work at all because ^ is not a well-formed JSON, it has no top-level container)

Running it locally, jq seemed to be okay with it; it just outputs separate JSON blobs:

go test -race -json -tags=memory_storage_integration -timeout 5m -coverprofile cover.out ./... | tee test-results.json | jq .
{
  "Time": "2023-12-28T07:02:35.584992+11:00",
  "Action": "output",
  "Package": "github.com/jaegertracing/jaeger",
  "Test": "TestDummy",
  "Output": "=== RUN   TestDummy\n"
}
{
  "Time": "2023-12-28T07:02:35.585097+11:00",
  "Action": "output",
  "Package": "github.com/jaegertracing/jaeger",
  "Test": "TestDummy",
  "Output": "--- PASS: TestDummy (0.00s)\n"
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there's also the question of pipe buffering, I think it gets worse so we'd less the output less frequently (but maybe I was just impatient). Anyway, I think one line is fine for now. I am more concerned with now failing the rest of the workflow when test results cannot be uploaded for whatever reason.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am more concerned with now failing the rest of the workflow when test results cannot be uploaded for whatever reason.

There are no upload/download steps in our test summary reporting workflows at the moment. You're right though, because I vaguely recall codecov upload failures may have failed the rest of the workflow in the past.

If we do want test summary reports on our PRs as comments and in the check runs, then artifact uploads and downloads would be required. Is this feature desirable enough to warrant introducing uploads? If not, I can hit the pause button on my WIP.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't feel like it's worth the hustle. The fact that test results are already showing in the summary is good, although if all it does is show the count (not the details of the failures) then it's pretty useless. Adding comments and checks is nice to have, but not at the expense of stability of the CI, which has been happening in the past few days due to these changes.

go tool cover -html=cover.out -o cover.html

.PHONY: nocover
Expand Down Expand Up @@ -494,6 +494,7 @@ draft-release:
install-test-tools:
$(GO) install github.com/golangci/golangci-lint/cmd/golangci-lint@v1.55.2
$(GO) install mvdan.cc/gofumpt@latest
$(GO) install github.com/jstemmer/go-junit-report/v2@latest

.PHONY: install-build-tools
install-build-tools:
Expand All @@ -507,8 +508,12 @@ install-tools: install-test-tools install-build-tools
install-ci: install-test-tools install-build-tools

.PHONY: test-ci
test-ci: GOTEST := $(GOTEST_QUIET)
test-ci: build-examples cover
test-ci: GOTEST := $(GOTEST_QUIET) -json
test-ci: install-test-tools build-examples cover test-report

.PHONY: test-report
test-report:
cat test-results.json | go-junit-report -parser gojson > junit-report.xml

.PHONY: thrift
thrift: idl/thrift/jaeger.thrift thrift-image
Expand Down
21 changes: 11 additions & 10 deletions model/keyvalue_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,24 +115,25 @@ func TestKeyValueAsStringAndValue(t *testing.T) {
expectedBinaryStr := `42656e6465722042656e64696e6720526f647269677565732042656e6465722042656e64696e6720526f647269677565732042656e6465722042656e64696e6720526f647269677565732042656e6465722042656e64696e6720526f647269677565730a0942656e6465722042656e64696e6720526f647269677565732042656e6465722042656e64696e6720526f647269677565732042656e6465722042656e64696e6720526f647269677565732042656e6465722042656e64696e6720526f647269677565732042656e6465722042656e64696e6720526f647269677565730a0942656e6465722042656e64696e6720526f647269677565732042656e6465722042656e64696e6720526f647269677565732042656e6465722042656e64696e6720526f647269677565732042656e6465722042656e64696e6720526f647269677565732042656e6465722042656e64696e6720526f647269677565730a0942656e6465722042656e64696e6720526f647269677565732042656e6465722042656e64696e6720526f647269677565732042656e6465722042656e64696e6720526f647269677565732042656e6465722042656e64696e6720526f647269677565732042656e6465722042656e64696e6720526f647269677565730a0942656e6465722042656e64696e6720526f647269677565732042656e6465722042656e64696e6720526f647269677565732042656e6465722042656e64696e6720526f647269677565732042656e6465722042656e64696e6720526f647269677565732042656e6465722042656e64696e6720526f6472696775657320`
expectedBinaryStrLossy := `42656e6465722042656e64696e6720526f647269677565732042656e6465722042656e64696e6720526f647269677565732042656e6465722042656e64696e6720526f647269677565732042656e6465722042656e64696e6720526f647269677565730a0942656e6465722042656e64696e6720526f647269677565732042656e6465722042656e64696e6720526f647269677565732042656e6465722042656e64696e6720526f647269677565732042656e6465722042656e64696e6720526f647269677565732042656e6465722042656e64696e6720526f647269677565730a0942656e6465722042656e64696e6720526f647269677565732042656e64...`
testCases := []struct {
name string
kv model.KeyValue
str string
strLossy string
val interface{}
}{
{kv: model.String("x", "Bender is great!"), str: "Bender is great!", val: "Bender is great!"},
{kv: model.Bool("x", false), str: "false", val: false},
{kv: model.Bool("x", true), str: "true", val: true},
{kv: model.Int64("x", 3000), str: "3000", val: int64(3000)},
{kv: model.Int64("x", -1947), str: "-1947", val: int64(-1947)},
{kv: model.Float64("x", 3.14159265359), str: "3.141592654", val: float64(3.14159265359)},
{kv: model.Binary("x", []byte("Bender")), str: "42656e646572", val: []byte("Bender")},
{kv: model.Binary("x", []byte(longString)), str: expectedBinaryStr, val: []byte(longString)},
{kv: model.Binary("x", []byte(longString)), strLossy: expectedBinaryStrLossy, val: []byte(longString)},
{name: "Bender is great!", kv: model.String("x", "Bender is great!"), str: "Bender is great!", val: "Bender is great!"},
{name: "false", kv: model.Bool("x", false), str: "false", val: false},
{name: "true", kv: model.Bool("x", true), str: "true", val: true},
{name: "3000", kv: model.Int64("x", 3000), str: "3000", val: int64(3000)},
{name: "-1947", kv: model.Int64("x", -1947), str: "-1947", val: int64(-1947)},
{name: "3.141592654", kv: model.Float64("x", 3.14159265359), str: "3.141592654", val: float64(3.14159265359)},
{name: "42656e646572", kv: model.Binary("x", []byte("Bender")), str: "42656e646572", val: []byte("Bender")},
{name: "binary string", kv: model.Binary("x", []byte(longString)), str: expectedBinaryStr, val: []byte(longString)},
{name: "binary string (lossy)", kv: model.Binary("x", []byte(longString)), strLossy: expectedBinaryStrLossy, val: []byte(longString)},
}
for _, tt := range testCases {
testCase := tt // capture loop var
t.Run(testCase.str, func(t *testing.T) {
t.Run(testCase.name, func(t *testing.T) {
if testCase.strLossy != "" {
assert.Equal(t, testCase.strLossy, testCase.kv.AsStringLossy())
} else {
Expand Down
Loading