Report ingest pipeline processor coverage #585

adriansr · 2021-11-15T18:05:37Z

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

Screenshots taken using Report Generator.

elasticmachine · 2021-11-15T18:48:28Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Start Time: 2021-12-22T08:32:44.603+0000
Duration: 28 min 50 sec
Commit: 627102c

Test stats 🧪

Test	Results
Failed	0
Passed	472
Skipped	0
Total	472

🤖 GitHub comments

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

/test : Re-trigger the build.

adriansr · 2021-11-16T09:14:34Z

The coverage reports in CI are not including the pipeline source code:

Looks like by the time the report is generated, the source code is not available. @mdelapenya @mtojek can you help?

mtojek

Cool feature, Adrian!

Looks like by the time the report is generated, the source code is not available. @mdelapenya @mtojek can you help?

First guess: I think this might be caused by having paths as names and you can't refer to these paths from the repository root.

I'm afraid that you need to analyze the Cobertura XML report manually and adjust the implementation later on. This is the path I went last time: elastic/integrations#755 (comment) and static files in #440 .

Screenshot taken from Report Generator.

If you plan to merge this feature, we'll kindly ask you to prepare a doc page describing how to use it.

mtojek · 2021-11-16T09:43:45Z

cmd/testrunner.go

 					return cobraext.FlagParsingError(err, cobraext.DataStreamsFlagName)
 				}
-
-				if testCoverage && len(dataStreams) > 0 {


This condition was placed here to prevent coverage miscalculation. The runner is not aware of data stream presence (or being skipped) so it always assumes that all data streams have been selected.

I understand that. But given that (afaik) the CI never tests a single datastream, it would be very useful for pipeline developers to be able to run tests for a single datastream locally and get coverage results.

mtojek · 2021-11-16T09:45:26Z

internal/testrunner/coverageoutput.go

 	BranchRate float32            `xml:"branch-rate,attr"`
 	Complexity float32            `xml:"complexity,attr"`
-	Methods    []*coberturaMethod `xml:"methods>method"`
+	Methods    []*CoberturaMethod `xml:"methods>method"`


You will have to update the docs with new mapping information: https://github.com/elastic/integrations/blob/master/docs/testing_and_validation.md#review-test-coverage

adriansr · 2021-11-16T11:39:03Z

@mtojek I'm not sure how it works during CI, but my understanding is that the coverage results are uploaded to GCloud storage (in

elastic-package/.ci/Jenkinsfile

Line 205 in 490e70d

    
           googleStorageUploadExt(bucket: getCoverageBucketURI(), credentialsId: "${JOB_GCS_EXT_CREDENTIALS}", pattern: "build/test-coverage/*.xml")

) and then downloaded before generating the Coverage report (in

elastic-package/.ci/Jenkinsfile

Line 192 in 490e70d

    
           googleStorageDownload(bucketUri: bucketUri, credentialsId: "${JOB_GCS_CREDENTIALS}", localDirectory: 'build/test-coverage', pathPrefix: getCoveragePathPrefix())

).

The generated coverage file contains absolute paths to the code. For example in zoom package, during a local run:

  <sources>
    <source>/Users/adrian/go/src/github.com/elastic/integrations/packages/zoom</source>
  </sources>

and then:

        <class name="account" filename="data_stream/webhook/elasticsearch/ingest_pipeline/account.yml" line-rate="0" branch-rate="0" complexity="0">

My understanding is that when the report is generated, the source code is not available at this path, but I don't understand in what context the coverageReport function is executed (can't find a definition for it). Maybe @mdelapenya could chime in as I see that he wrote the coverage handling in Jenkinsfile.

jsoriano · 2021-11-16T11:41:32Z

coverageReport is defined here: https://github.com/elastic/apm-pipeline-library/blob/4a58b62664d47cb32e6c2924baf75627bf8346c9/vars/coverageReport.groovy

mdelapenya · 2021-11-16T17:52:30Z

internal/testrunner/coverageoutput.go

+		if tcd.cobertura != nil && result.Coverage != nil {
+			tcd.cobertura.Merge(result.Coverage)
+		} else {
+			tcd.cobertura = result.Coverage


What happens if result.Coverage is nil and tcd.cobertura is not? In that line we will be setting the latter to nil. Is that correct?

Fixed, thanks

mdelapenya · 2021-11-16T17:57:41Z

internal/testrunner/coverageoutput.go

 }

+// Merge merges two coverage reports for a given class.
+func (c *CoberturaClass) Merge(b *CoberturaClass) {


Do you see feasible adding unit tests for this method? I see a lot of calculations that would benefit for the tests

mdelapenya · 2021-11-16T17:58:04Z

internal/elasticsearch/pipeline/processors.go

+	"gopkg.in/yaml.v3"
+)
+
+type Processor struct {


Do you see feasible adding unit tests for this method? I see a lot of calculations that would benefit for the tests

jsoriano

Thanks @adriansr, this is great!

Is there something else keeping this as draft?

jsoriano · 2021-11-19T12:37:56Z

internal/elasticsearch/node_stats/stats_api.go

+	"github.com/elastic/go-elasticsearch/v7"
+	"github.com/pkg/errors"
+
+	es "github.com/elastic/elastic-package/internal/elasticsearch"


Trying to reduce the need of these es aliases in #596.

jsoriano · 2021-11-19T12:44:55Z

internal/elasticsearch/pipeline/processors.go

+		if err := entry.Content[0].Decode(&proc.Type); err != nil {
+			return nil, errors.Wrapf(err, "error decoding processor#%d type", idx)
+		}
+		proc.Line = entry.Line


Nit. Could the Line of the last Content be used as the last line of the processor so we have per-line coverage instead of per-processor coverage? (and Cobertura paints red/green the whole processor 🙂)

Makes sense, done.

Originally I decided against it because it will mark the inner processors of a compound processor as covered (inner processor of foreach, and on_failure processors), while ES doesn't provide coverage information for those.

adriansr · 2021-11-22T10:18:04Z

This is currently waiting for the required CI fixes to have source code displayed in Jenkins coverage reports.

Once that is sorted out I'll make any required adjustments to the format (for now only tested it with a different coverage tool) and prepare the new documentation.

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

elasticmachine · 2021-12-22T09:32:31Z

Pinging @elastic/integrations (Team:Integrations)

adriansr · 2021-12-22T10:17:56Z

This is ready to review pending adjustments to CI in order to make the new source code reports available.

Updated docs at elastic/integrations#2371

mtojek · 2021-12-22T11:23:16Z

We collect Cobertura reports also for elastic-package. @adriansr Could you please double check if this new feature is enabled also here?

mtojek

To be honest, this PR is massive. Do you think you can split it into smaller, reviewable chunks? It would be easier to fully understand what's going on behind the scenes.

mtojek · 2021-12-22T11:31:39Z

internal/elasticsearch/node_stats/stats_api.go

+// or more contributor license agreements. Licensed under the Elastic License;
+// you may not use this file except in compliance with the Elastic License.
+
+package node_stats


Please rename it to nodestats.

mtojek · 2021-12-22T11:39:14Z

internal/elasticsearch/pipeline/coverage/coverage.go

+)
+
+// Get returns a coverage report for the provided set of ingest pipelines.
+func Get(options testrunner.TestOptions, pipelines []pipeline.Resource) (*testrunner.CoberturaCoverage, error) {


Here is a bit of spaghetti code that has to be untangled first. Code coverage isn't the responsibility of Elasticsearch client or Pipeline, but test runners and it should stay there.

I suggest refactoring (moving around) this dependency first, then we can continue reviewing as this PR is relatively big.

mtojek reviewed Nov 16, 2021

View reviewed changes

mtojek requested a review from jsoriano November 16, 2021 09:54

mdelapenya reviewed Nov 16, 2021

View reviewed changes

jsoriano reviewed Nov 19, 2021

View reviewed changes

adriansr added 7 commits December 22, 2021 15:31

Report ingest pipeline processor coverage

d794489

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

Propagate error from merge

ef9a24c

Fix logic bug

517bc74

Tests and fixes for Processors method

1b406e2

Test for Cobertura classes merge

e0e0ea2

Missing license

f33d0fa

Take all processor lines into account

627102c

adriansr marked this pull request as ready for review December 22, 2021 09:03

adriansr changed the title ~~[Draft] Report ingest pipeline processor coverage~~ Report ingest pipeline processor coverage Dec 22, 2021

adriansr added Team:Integrations Label for the Integrations team Team:Security-External Integrations labels Dec 22, 2021

adriansr mentioned this pull request Dec 22, 2021

Document pipeline tests coverage reports elastic/integrations#2371

Merged

mtojek suggested changes Dec 22, 2021

View reviewed changes

adriansr closed this Dec 27, 2021

adriansr mentioned this pull request Jan 5, 2022

Refactor pipelineResource into a separate package #639

Merged

adriansr mentioned this pull request Mar 8, 2022

Generate a cobertura report for processors in pipeline tests #704

Merged

This was referenced Apr 1, 2022

ci: use the Jenkins Code Coverage Plug-in with source support #775

Closed

ci: use the Jenkins Code Coverage Plug-in with source support elastic/integrations#2986

Closed

ci: enable publishCoverage elastic/apm-pipeline-library#1644

Merged

Report ingest pipeline processor coverage #585

Report ingest pipeline processor coverage #585

Uh oh!

Conversation

adriansr commented Nov 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Nov 15, 2021 • 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!

adriansr commented Nov 16, 2021

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!

adriansr commented Nov 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jsoriano commented Nov 16, 2021

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!

jsoriano 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!

adriansr commented Nov 22, 2021

Uh oh!

elasticmachine commented Dec 22, 2021

Uh oh!

adriansr commented Dec 22, 2021

Uh oh!

mtojek commented Dec 22, 2021

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

adriansr commented Nov 15, 2021 •

edited

Loading

elasticmachine commented Nov 15, 2021 •

edited

Loading

adriansr commented Nov 16, 2021 •

edited

Loading