-
Notifications
You must be signed in to change notification settings - Fork 148
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
ci: enable coverage #377
ci: enable coverage #377
Conversation
This pull request does not have a backport label. Could you fix it @v1v? 🙏
NOTE: |
go.sum
Outdated
@@ -266,6 +266,7 @@ github.com/bmizerany/pat v0.0.0-20170815010413-6226ea591a40/go.mod h1:8rLXio+Wji | |||
github.com/boltdb/bolt v1.3.1/go.mod h1:clJnj/oiGkjum5o1McbSZDSLxVThjynRyGBgiAx27Ps= | |||
github.com/bonitoo-io/go-sql-bigquery v0.3.4-1.4.0/go.mod h1:J4Y6YJm0qTWB9aFziB7cPeSyc6dOZFyJdteSeybVpXQ= | |||
github.com/boumenot/gocover-cobertura v1.2.0/go.mod h1:fz7ly8dslE42VRR5ZWLt2OHGDHjkTiA2oNvKgJEjLT0= | |||
github.com/boumenot/gocover-cobertura v1.2.0/go.mod h1:fz7ly8dslE42VRR5ZWLt2OHGDHjkTiA2oNvKgJEjLT0= |
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.
I'd also add a reference to tools.go file (on mobile, will paste link asap)
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.
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.
Btw, lines 268 and 269 are exactly the same. Can you run go mod tidy and verify that?
🌐 Coverage report
|
.ci/Jenkinsfile
Outdated
@@ -111,6 +112,7 @@ pipeline { | |||
post { | |||
always { | |||
junit(allowEmptyResults: true, keepLongStdio: true, testResults: "${BASE_DIR}/build/TEST-*.xml") | |||
coverageReport(baseDir: "**/build", reportFiles: 'TEST-go-unit.html', coverageFiles: 'TEST-go-unit-cov.xml') |
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.
Question: is it required to aggregate the results from all the different platforms (windows, linux, arm)? If so, https://github.com/elastic/beats/blob/main/dev-tools/aggregate_coverage.py is the one in charge?
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.
Having only the Linux coverage can be a good first step.
I am fine with only Linux, @jlind23 We should probably exclude |
Thanks, 499e299 is the one only driving the test_coverage for linux |
@ph agree, let's remove it. |
How? I could strip those entries in the generated cov file, but I'm not sure if the go coverage supports something natively, any hints? |
@v1v I've look at that, seems not a simple solution other than removing the files or path from the coverage output https://stackoverflow.com/questions/50065448/how-to-ignore-generated-files-from-go-test-coverage |
@ph, |
@v1v sure move ahead, we will update it afterwards. |
I only need some 👍 in the code review, so I can merge it :) |
(cherry picked from commit 752c699)
(cherry picked from commit 752c699) Co-authored-by: Victor Martinez <[email protected]>
…use-orka * 'main' of github.com:elastic/elastic-agent: (23 commits) [Automation] Update go release version to 1.17.10 (elastic#432) [Automation] Update elastic stack version to 8.3.0-4149272f for testing (elastic#435) [Automation] Update elastic stack version to 8.3.0-19aba912 for testing (elastic#430) Add extra k8s resources in clusterRole (elastic#424) [Automation] Update elastic stack version to 8.3.0-8ee1196f for testing (elastic#422) [Automation] Update elastic stack version to 8.3.0-53513548 for testing (elastic#421) Add tags option during enroll/install (elastic#336) validate kubernetes templates in .CI (elastic#417) add missing kube-api resources from managed agent manifest (elastic#381) Create snyk-scan.yml (elastic#397) [Automation] Update elastic stack version to 8.3.0-d380914f for testing (elastic#414) [Automation] Update elastic stack version to 8.3.0-5c1ff35f for testing (elastic#413) [Automation] Update elastic stack version to 8.3.0-6ba9f710 for testing (elastic#410) [Automation] Update elastic stack version to 8.3.0-a1c5cfff for testing (elastic#406) [Automation] Update elastic stack version to 8.3.0-7f585873 for testing (elastic#401) [Automation] Update elastic stack version to 8.3.0-0b6ea9f2 for testing (elastic#399) ci: enable coverage (elastic#377) Remove last dependencies on beats repo (elastic#387) Remove dependency on libbeat (elastic#344) [Automation] Update elastic stack version to 8.3.0-cb2ce38c for testing (elastic#383) ...
This is something that was introduced with elastic#377 and was meant to be used with Jenkins to show coverage report. Now with Jenkins been replace and BK + Sonar enabled on the repo, there is no need to create such artifacts anymore. Signed-off-by: Alexandros Sapranidis <[email protected]>
What does this PR do?
Report Test Coverage in the CI
Why is it important?
Help to visualise when the Test coverage gets increased or decreased (see #377 (comment))
Questions
Current implementation does run coverage for each platform (windows, linux and ARM), therefore the coverage results are not aggregated, I've seen https://github.com/elastic/beats/blob/main/dev-tools/aggregate_coverage.py.
Tasks