Generate Cobertura report #441
Conversation
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
Trends 🧪 |
mtojek
force-pushed
the
755-generate-coverage-report
branch
from
92e7d56 to
d3ddcd4
Compare
internal/builder/packages.go
Outdated
| // FindBuildDirectory locates the target build directory. | ||
| func FindBuildDirectory() (string, bool, error) { | ||
| // MustFindBuildDirectory function locates the target build directory. If the directory doesn't exist, it will create it. | ||
| func MustFindBuildDirectory() (string, error) { |
There was a problem hiding this comment.
Nit. Must* functions use to imply that no error handling needs to be done but they panic if something fails, this one returns an error, and never panics.
If you want to indicate that the function finds or creates the directory, it could be called FindOrCreateBuildDirectory.
If it is a public method to obtain the build directory no matter what, it might be just called BuildDirectory().
There was a problem hiding this comment.
Ah yes, I can rename it to just BuildDirectory(). This will fit too! Thanks for this feedback!
| @@ -0,0 +1,248 @@ | |||
| // Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | |||
There was a problem hiding this comment.
Nit. Underscores shouldn't be used in go file names, unless they mean something for go build. This file could be named coverageoutput.go.
There was a problem hiding this comment.
I'm afraid we're overusing them here. I will adjust this file unit.
Do you know if there is any official recommendation written down? I must have missed reading it.
There was a problem hiding this comment.
I think there is not, but for packages (and then for directories), there is a recommendation to don't use underscores. And for files, some suffixes have special meanings for go build, as the ones for architectures or operating systems, so in general I think that is a good idea to avoid them.
There was a problem hiding this comment.
Thanks for the suggestion, fixed!
| for i, tc := range testCases { | ||
| methods = append(methods, &coberturaMethod{ | ||
| Name: tc, | ||
| Lines: []*coberturaLine{{Number: i + 1, Hits: 1}}, |
There was a problem hiding this comment.
Thoughts for a possible future improvement. Coverage is at the data stream level now, right? Maybe in the future we can do it at least at the file level, e.g. saying that pipeline tests add coverage for pipelines, but not for the manifest or other config files, and system tests add coverage for all the files.
There was a problem hiding this comment.
Ah I know what you mean... We shouldn't mark a missing pipeline test if there are no pipelines at all. Sounds like a follow up.
There was a problem hiding this comment.
Yes, this would be for the future for sure, no need to do anything else by now 🙂
| return cobraext.FlagParsingError(err, cobraext.ReportOutputFlagName) | ||
| } | ||
|
|
||
| testCoverage, err := cmd.Flags().GetBool(cobraext.TestCoverageFlagName) |
There was a problem hiding this comment.
Should this flag only be allowed if --report-output file is also used?
There was a problem hiding this comment.
The --report-output refers to test results, so I assumed it's something else. I don't see a value in printing the coverage to STDOUT, so I didn't bind these flags together.
Do you think we should introduce additional flag for --coverage-output file (or stdout) or is there low gain?
There was a problem hiding this comment.
I was thinking that it may be weird to have files created even when there is only stdout output by default. But improving this, if needed, sounds like something for the future. For example we might show coverage info in the normal test reports, this could be nice, then --report-output would control the output of both features.
There was a problem hiding this comment.
I see, let's keep it for further improvement if there is feature need for this feature.
3 participants
Issue: elastic/integrations#755
This PR introduces logic to generate Cobertura test coverage reports.
Sample reports:
https://beats-ci.elastic.co/job/Ingest-manager/job/elastic-package/job/PR-441/7/cobertura/
https://beats-ci.elastic.co/job/Ingest-manager/job/elastic-package/job/PR-441/9/cobertura/