Generate a cobertura report for processors in pipeline tests #704

adriansr · 2022-02-17T20:30:11Z

This PR updates the pipeline tests to generate code-coverage (at the processor level) for ingest pipelines:

Changes in CI will be needed so that coverage reports are visible in CI.

For local testing, reports are available under build/test-coverage and can be visualized with a tool like Report Generator.

This improves the coverage reporting (--test-coverage flag) to include detailed coverage for ingest pipelines in the pipeline test.

adriansr · 2022-02-17T20:31:28Z

cmd/testrunner.go

-
-				if testCoverage && len(dataStreams) > 0 {
-					return cobraext.FlagParsingError(errors.New("test coverage can be calculated only if all data streams are selected"), cobraext.DataStreamsFlagName)
-				}


This restriction has been removed for convenience, as coverage reports are useful during development and we may want to test as quick as possible and only the data stream being developed.

elasticmachine · 2022-02-17T20:32:14Z

Pinging @elastic/integrations (Team:Integrations)

elasticmachine · 2022-02-17T20:52:14Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
[](https://ci-stats.elastic.co/app/apm/services/beats-ci/transactions/view?rangeFrom=2022-03-08T14:18:52.869Z&rangeTo=2022-03-08T14:38:52.869Z&transactionName=BUILD Ingest-manager/elastic-package/PR-{number}&transactionType=job&latencyAggregationType=avg&traceId=7796a29faab38fc70c5865930f7c6ed6&transactionId=1fe1d68780b695d3)

Expand to view the summary

Build stats

Start Time: 2022-03-08T14:28:52.869+0000
Duration: 16 min 47 sec

Test stats 🧪

Test	Results
Failed	0
Passed	536
Skipped	1
Total	537

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.

mtojek

Could you please describe how will the coverage calculation change once this PR is merged?

Consider the following cases:

Run test - cover all data streams of a package
Run test --data-stream foo - cover only the foo data stream
Run test pipeline - cover all data streams, but only pipeline tests
Run test - but no tests present

Sorry for adding more tasks to do, but I'd like to double-check if there are any risky side effects.

Also, what kind of changes do we need on the CI side?

mtojek · 2022-02-21T09:12:50Z

internal/testrunner/coverageoutput.go


-type coberturaCoverage struct {
+// CoberturaCoverage is the root element for a Cobertura XML report.
+type CoberturaCoverage struct {


Is there any reason why these properties are exposed? Is it for the consistency with CoberturaCoverage?

I think I got mixed here a little bit. The original idea was for the test runners to have the option to generate their own coverage in Cobertura format (via the new TestResult.Coverage), that's why these types are exposed.

To align with this idea, I think it makes more sense to put GetPipelineCoverage into the pipeline runner instead of the global testrunner package as is now.

To align with this idea, I think it makes more sense to put GetPipelineCoverage into the pipeline runner instead of the global testrunner package as is now.

Yes, I have a similar feeling about this :)

internal/testrunner/coverageoutput.go

mtojek · 2022-02-21T09:18:56Z

cmd/testrunner.go

 				API:                esClient.API,
 				DeferCleanup:       deferCleanup,
 				ServiceVariant:     variantFlag,
+				WithCoverage:       testCoverage,


Does this property apply to all test runners (system tests, static tests, etc.)? Maybe we should add a WARN informing if we can't calculate the coverage properly.

The way I see it we have the old default coverage, which only tells whether we have a particular kind of test for the package, and the opt-in detailed coverage added by this PR. Each individual test runner can err if they wanted to calculate detailed coverage but failed, as the pipeline tests will do after this PR.

mtojek · 2022-02-24T10:37:54Z

@adriansr Please re-request the review when this PR is ready for another round. It would be great to research these cases.

adriansr · 2022-03-07T16:54:48Z

Run test - cover all data streams of a package

Before this PR, it generates a reduced coverage report for each kind of test (system, static, asset, pipeline...).

Each of these tests contains a single package (name of integration package), then one class per data_stream (class name: test type, class filename: package/datastream).
Each class contains a single method, named OK or Missing depending on this type of test being present for the given datastream.

After this PR, the output is the same, except for the pipeline coverage report, which contains processor coverage of the ingest pipelines:
Each package represent a data_stream, which contains classes representing each pipeline under the datastream, which consist of methods, one per each processor in the pipeline.

Run test --data-stream foo - cover only the foo data stream

Before: It's not possible to run a coverage report for a single data-stream.
After: you get a coverage report with code coverage for the different pipelines.

Run test pipeline - cover all data streams, but only pipeline tests

Before: You get the reduced coverage report (OK / Missing).
After: you get a coverage report with code coverage for the different pipelines.

Run test - but no tests present

Only the reduced coverage reports are present.

Also, what kind of changes do we need on the CI side?

It needs some investigation, but basically make sure that the source code for pipelines is in place by the time the coverage reports are rendered.

mtojek

Thanks for providing more information, Adrian. I see only concerns around integrating this change with our CI. I left a comment in a relevant place in the source code.

Apart from this, I think it's 👍 .

mtojek · 2022-03-08T09:29:47Z

internal/testrunner/coverageoutput_test.go

+		wantErr            bool
+	}{
+		{
+			name: "merge sources",


nit: I'm thinking now if it isn't simpler and easier to read to load these tests from a testdata directory. WDYT?

mtojek · 2022-03-08T09:33:32Z

internal/testrunner/runners/pipeline/coverage.go

+
+// GetPipelineCoverage returns a coverage report for the provided set of ingest pipelines.
+func GetPipelineCoverage(options testrunner.TestOptions, pipelines []ingest.Pipeline) (*testrunner.CoberturaCoverage, error) {
+	packagePath, err := packages.MustFindPackageRoot()


Isn't the package root present in TestOptions?

Turns out it is.

mtojek · 2022-03-08T09:38:29Z

internal/testrunner/runners/pipeline/coverage.go

+		pipelineName = pipelineName[:nameEnd]
+	}
+
+	// File path has to be relative to the packagePath added to the cobertura Sources list


Let's discuss the safe deployment of this feature.

If we merge this PR and release a new elastic-package, will it break the CI tests for Elastic Integrations? If it breaks, then we need to go the other way round and adjust the Jenkinsfile first and make sure that pipeline sources are in the right directory (relative to the runner?).

There's no reason to believe it will break any tests, I've tested it multiple times with all the existing integrations.

I think you're misunderstanding the CI issues that will need to be addressed. I explained them in this comment: #585 (comment)

Those issues will not cause a test failure. Just the coverage report displayed in CI will not show annotated source code.

Yes, that's what I mean. Let me share my concerns, Jenkins uses coverageReport function to collect the coverage stats. I don't know what will happen if that function files (due to missing pipeline sources). Will it not show the coverage split by source code or fail the entire CI pipeline?

It is not causing a CI pipeline failure in this PR, just the source code is reported missing. See:

https://beats-ci.elastic.co/job/Ingest-manager/job/elastic-package/job/PR-704/4/cobertura/apache_access/apache_data_stream_access_elasticsearch_ingest_pipeline_default_yml/

Understood, thanks for clarifying this.

Report ingest pipeline processor coverage

699b7c2

This improves the coverage reporting (--test-coverage flag) to include detailed coverage for ingest pipelines in the pipeline test.

adriansr commented Feb 17, 2022

View reviewed changes

adriansr added Team:Ecosystem Label for the Packages Ecosystem team Team:Integrations Label for the Integrations team Team:Security-External Integrations labels Feb 17, 2022

mtojek requested a review from a team February 21, 2022 09:04

mtojek reviewed Feb 21, 2022

View reviewed changes

mtojek requested a review from a team February 21, 2022 09:22

adriansr added 2 commits February 22, 2022 16:09

Make merge methods package-private

5f78a2b

Move GetPipelineCoverage to pipeline test package

1184d39

adriansr requested a review from mtojek March 7, 2022 16:55

mtojek reviewed Mar 8, 2022

View reviewed changes

adriansr added 2 commits March 8, 2022 14:49

Use TestOptions.PackageRootPath

9d774e9

Make paths relatives to package parent directory

71312b1

mtojek approved these changes Mar 8, 2022

View reviewed changes

adriansr merged commit 11a955c into elastic:main Mar 9, 2022

adriansr deleted the pipeline_processor_coverage branch March 9, 2022 14:20

andrewkroh mentioned this pull request May 2, 2022

[pfSense] Add OPNsense support and multiple other log types elastic/integrations#2413

Merged

7 tasks

Generate a cobertura report for processors in pipeline tests #704

Generate a cobertura report for processors in pipeline tests #704

Uh oh!

Conversation

adriansr commented Feb 17, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Feb 17, 2022

Uh oh!

elasticmachine commented Feb 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💚 Build Succeeded

Build stats

Test stats 🧪

🤖 GitHub comments

Uh oh!

mtojek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtojek commented Feb 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adriansr commented Mar 7, 2022

Uh oh!

mtojek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

elasticmachine commented Feb 17, 2022 •

edited

Loading

mtojek commented Feb 24, 2022 •

edited

Loading