Run unit and integration tests with gotestsum #22541
Conversation
botelastic
bot
added
the
needs_team
🐛 Flaky test report❕ There are test failures but not known flaky tests. Expand to view the summary
Test stats 🧪
Genuine test errors
💔 There are test failures but not known flaky tests, most likely a genuine test failure.
|
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
Steps errors
Expand to view the steps failures
|
| Test | Results |
|---|---|
| Failed | 0 |
| Passed | 17387 |
| Skipped | 1345 |
| Total | 18732 |
|
I managed to spin up a new VM to run the commands directly there And I diff with the Jenkins builds and it's shown below diff --git a/jenkins.txt b/cli.txt
index b7f6f47ef..b59b0ee2d 100644
--- a/jenkins.txt
+++ b/cli.txt
@@ -2,6 +2,26 @@ mage build test
>> build: Building filebeat
>> go test: Unit Testing
FAILURES:
+Package: github.com/elastic/beats/v7/filebeat/config
+Test: TestReadConfig2
+ config_test.go:43:
+----
+Package: github.com/elastic/beats/v7/filebeat/config
+Test: TestMergeConfigFiles
+ config_test.go:113:
+ config_test.go:115:
+----
+Package: github.com/elastic/beats/v7/filebeat/fileset
+Test: TestLoadManifestNginx
+ fileset_test.go:57:
+panic: runtime error: invalid memory address or nil pointer dereference
+/home/vmartinez/.gvm/versions/go1.14.7.linux.amd64/src/testing/testing.go:988 +0x30d
+/home/vmartinez/.gvm/versions/go1.14.7.linux.amd64/src/testing/testing.go:991 +0x3f9
+/home/vmartinez/.gvm/versions/go1.14.7.linux.amd64/src/runtime/panic.go:969 +0x166
+/home/vmartinez/src/github.com/elastic/beats/filebeat/fileset/fileset_test.go:58 +0xa1
+/home/vmartinez/.gvm/versions/go1.14.7.linux.amd64/src/testing/testing.go:1039 +0xdc
+/home/vmartinez/.gvm/versions/go1.14.7.linux.amd64/src/testing/testing.go:1090 +0x372
+----
Package: github.com/elastic/beats/v7/filebeat/placeholder/goroutinepanic
Test: TestWrongPanic
----
@@ -10,11 +30,11 @@ Test: TestWrongPanic/setup_failing_go-routine
----
Package: github.com/elastic/beats/v7/filebeat/placeholder/goroutinepanic
Test: TestWrongPanic/false_positive_failure
-/var/lib/jenkins/workspace/Beats_beats_PR-22541/.gvm/versions/go1.14.7.linux.amd64/src/testing/testing.go:654 +0x11a
-/var/lib/jenkins/workspace/Beats_beats_PR-22541/.gvm/versions/go1.14.7.linux.amd64/src/testing/testing.go:676 +0x2b
-/var/lib/jenkins/workspace/Beats_beats_PR-22541/.gvm/versions/go1.14.7.linux.amd64/src/testing/testing.go:766 +0x78
-/var/lib/jenkins/workspace/Beats_beats_PR-22541/src/github.com/elastic/beats/filebeat/placeholder/goroutinepanic/run_test.go:12 +0x76
-/var/lib/jenkins/workspace/Beats_beats_PR-22541/src/github.com/elastic/beats/filebeat/placeholder/goroutinepanic/run_test.go:10 +0x3f
+/home/vmartinez/.gvm/versions/go1.14.7.linux.amd64/src/testing/testing.go:654 +0x11a
+/home/vmartinez/.gvm/versions/go1.14.7.linux.amd64/src/testing/testing.go:676 +0x2b
+/home/vmartinez/.gvm/versions/go1.14.7.linux.amd64/src/testing/testing.go:766 +0x78
+/home/vmartinez/src/github.com/elastic/beats/filebeat/placeholder/goroutinepanic/run_test.go:12 +0x76
+/home/vmartinez/src/github.com/elastic/beats/filebeat/placeholder/goroutinepanic/run_test.go:10 +0x3f
----
Package: github.com/elastic/beats/v7/filebeat/placeholder/withassert
Test: TestSimpleAssertFails
@@ -85,13 +105,12 @@ Test: TestLogsWithNewlines
42
----
SUMMARY:
- Fail: 14
- Skip: 1
- Pass: 659
+ Fail: 17
+ Skip: 0
+ Pass: 602
Packages: 29
- Duration: 50.947041551s
- Coverage Report: /var/lib/jenkins/workspace/Beats_beats_PR-22541/src/github.com/elastic/beats/filebeat/build/TEST-go-unit.html
- JUnit Report: /var/lib/jenkins/workspace/Beats_beats_PR-22541/src/github.com/elastic/beats/filebeat/build/TEST-go-unit.xml
- Output File: /var/lib/jenkins/workspace/Beats_beats_PR-22541/src/github.com/elastic/beats/filebeat/build/TEST-go-unit.out
+ Duration: 1m4.684273816s
+ JUnit Report: /home/vmartinez/src/github.com/elastic/beats/filebeat/build/TEST-go-unit.xml
+ Output File: /home/vmartinez/src/github.com/elastic/beats/filebeat/build/TEST-go-unit.out
>> go test: Unit Test Failed
-Error: go test failed: 14 test failures
\ No newline at end of file
+Error: go test failed: 17 test failures |
andresrc
added
the
Team:Services
botelastic
bot
removed
the
needs_team
|
So far I tracked down the issue to a custom test result parser in our mage tools. The VM/Jenkins setup seems to be ok. |
|
I'm trying if I can improve/fix reporting using another project. The current approach tries to capture stdout,stderr and then tries to parse the final test output using https://github.com/jstemmer/go-junit-report. This project seems to be somewhat unmaintained and has quite a few open PRs and issues trying to improve parsing. Instead of trying to fix go-junit-reporer, I'm giving https://github.com/gotestyourself/gotestsum#json-file-output a try. The tool is a
This is just a "test". Running the changes locally, logs are correctly captured. Only problem is still the panics, which the |
|
Something is still wrong with the package selection. Right now all unit tests are executed per Beat it seems. But the output with Case 1 (goroutine panic): The output is still somewhat missleading. If a leaked go-routine panics the wrong test is still marked as failed. As there is no panic handler, the full test suite of the current package is put to a halt. All in all this is to be expected. On the positive side, the original Test that did leak the routine failing late was also marked as failed. This has not been the case before. All in I would say this case is reported as good as possible. Case 2 (with assert): Success. All logs, failures, and newlines are printed correctly: Case 3: The logs output looks correct. CI output: |
| return c | ||
| } | ||
|
|
||
| // GoTestSummary is a summary of test results. | ||
| type GoTestSummary struct { |
There was a problem hiding this comment.
GoTestSummary is not used anymore. gotestsum prints it's own summary, not as rich as this one, but should be good enough.
There was a problem hiding this comment.
Thanks for fixing this! And great to have tests covering these problems 👍
I think we need to pin the version of gotestsum used to avoid surprises if something changes in upstream.
I have added some comments about this, but summarizing it would be to:
- Add gotestsum to
tools/tools.goas we do with other external tools so they are managed by go modules. - Install it with
go installinstead ofgo get. - If possible, install it only as a dependency of the mage target that uses it, so we don't need to keep track of its installation along all the build scripts and Dockerfiles.
.ci/scripts/install-tools.sh
Outdated
| @@ -4,4 +4,5 @@ set -exuo pipefail | |||
| .ci/scripts/install-go.sh | |||
| .ci/scripts/install-docker-compose.sh | |||
| .ci/scripts/install-terraform.sh | |||
| (cd /tmp && go get gotest.tools/gotestsum) | |||
There was a problem hiding this comment.
Installing tools with go get has lead to unexpected failures in the past, could we include this tool in tools/tools.go, add it to go.mod, and install it with go install when needed?
There was a problem hiding this comment.
I originally tried with go install and it failed 👍
Sure, I can add it to go.mod and make the go install a build dependency.
| @@ -189,34 +185,65 @@ func GoTestIntegrationForModule(ctx context.Context) error { | |||
| func GoTest(ctx context.Context, params GoTestArgs) error { | |||
There was a problem hiding this comment.
Could we create a InstallGotestsum target and add it here as dependency?
| func GoTest(ctx context.Context, params GoTestArgs) error { | |
| func GoTest(ctx context.Context, params GoTestArgs) error { | |
| mg.Deps(InstallGotestsum) |
We do this with InstallGoLicenser for example.
metricbeat/Dockerfile
Outdated
| @@ -14,6 +14,7 @@ RUN \ | |||
| RUN pip3 install --upgrade pip==20.1.1 | |||
| RUN pip3 install --upgrade setuptools==47.3.2 | |||
| RUN pip3 install --upgrade docker-compose==1.23.2 | |||
| RUN go get gotest.tools/gotestsum | |||
There was a problem hiding this comment.
I think we need to control the version of this tool, if we do something like adding the installation as dependency of the mage targets we wouldn't need to add it here in docker images and other build scripts.
Will do, thanks. In general I'm not a big fan of us building and installing development and build dependencies over and over again. if we haven't tools/tools.go (or are not disciplined in using it), dependencies are buried in mage files and others. Plus, dependencies added to go.mod automatically become Beats dependencies and must be mentioned in the notice file. License wise this also restricts what can be used for development. Long term I would prefer if we require a specific set of tools to be available (plus version) and provide build images + docker containers for testing that have the dependencies preinstalled. We should not be required to install debian packages or use |
|
Updated the PR to install gotestsum via mage + add the dependency to go.mod. Now I remember why I installed the tool into a temporary directory. I wonder if it would help if we have had a separate |
Yep, I agree that always running
What you mention about using a separated
Yep, this can be a good idea, specially for dependencies that cannot be installed with |
|
@jsoriano Have had to update some dependencies in go.mod, but it seems beats-ci is still happy. Any idea about the E2E-tests? Are the failures known or is there some other problem we need to fix in the Beats repo? |
Yes, fully agree. Some scripts for users/developers to install and update their dev environment should be available. |
Umm, I don't think these changes affect E2E testing, but just in case, @mdelapenya do you know if these failures in E2E tests are known? |
|
About the two failures on the E2E side:
|
|
@mdelapenya Thank you! |
The PR introduces gotestsum to drive the go based unit and integration tests. This is meant as replacement for github.com/jstemmer/go-junit-report, which seems to be unmaintained for a while. Only the GoTest mage helper is modified to use gotestsum instead of go-junit-reporter. In addition to the change we add unit tests for GoTest that do run actual tests. The output of these helper-tests is captured and validated against known regular expressions. (cherry picked from commit 2eb3c29)
What does this PR do?
The PR introduces gotestsum to drive the go based unit and integration tests. This is meant as replacement for
github.com/jstemmer/go-junit-report, which seems to be unmaintained for a while.Only the GoTest mage helper is modified to use gotestsum instead of go-junit-reporter. In addition to the change we add unit tests for GoTest that do run actual tests. The output of these helper-tests is captured and validated against known regular expressions.
Why is it important?
We use go-junit-reporter in order to create a xunit like report that is uploaded after the tests have finished. Unfortunately the project seems to be unmaintained for at least a year.
We discovered that we have a few issues with our test output. For example lines have been missing, wrong tests have been marked as failed (on panic in a leaked go-routine), or failed tests have not been reported at all. The new unit tests try to cover most cases that have not been reported correctly, in order to validate that the change actually fixes reporting.
goroutinepanic handling
This one might be a little flaky. But it seems that a 'badly' scheduled unmanaged go-routine can bring down and flag unrelated tests. The stack trace in the failed test actually matches the original test that did spawn the go-routine.
We did try to reproduce the issue by running 2 tests. The first test creates a go-routine that sleeps for 2 seconds and finally panics. The second tests waits for 5 seconds. Depending on the order the test is run we did see the second test to be marked as "Failed". A sample output using go-junit-reporter shows
false_positive_failureas failed:Unfortunately the problem is not solved by switching to gotestsum (or running
go test). Here it is up to us to ensure we never leak go-routines when a test returns. For example in auditbeat we found tests like:The tests did fail (especially on windows) and the go routine did bring down another unrelated test.
testify assert output missing
We use the testify package for validation. The package adds a newline before the actual report. Unfortunately the assert output was missing from all test output in jenkins. We finally tracked down the problem to go-junit-reporter "eating" our test output.
We did create some failing sample tests in order debug the issue. The expected output was:
Unfortunately the actual output was missing the failed assertions:
The new unit tests for GoTest show that the problem is solved by gotestsum
Checklist
- [ ] I have made corresponding changes to the documentation- [ ] I have made corresponding change to the default configuration files- [ ] I have added an entry inCHANGELOG.next.asciidocorCHANGELOG-developer.next.asciidoc.How to test this PR locally
make testand the differentmagetesting commands will usegotestsumnow.Limitations
Tests must not print directly or indirectly to stdout/stdout. The
go test -jsoncommand does not mark a test as passed/failed in that case, which will subsequently mark the test as failed (unknown state) in the junit report. The test run itself succeeds, though.Related issues