Skip to content
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

Ingest code coverage #62336

Merged
merged 451 commits into from May 13, 2020
Merged

Ingest code coverage #62336

merged 451 commits into from May 13, 2020

Conversation

wayneseymour
Copy link
Member

@wayneseymour wayneseymour commented Apr 2, 2020

Summary

Ingest code coverage to Kibana Stats

Hosting

Code coverage Collection

Tools and Tests

We collect coverage for

Running locally

set CODE_COVERAGE=1 before kbn bootstrap
In addition, setting NODE_ENV=test is required for x-pack jest tests

  • mocha: yarn run grunt "test:mochaCoverage"
  • jest oss: node scripts/jest --verbose --coverage
  • jest x-pack: node ./legacy/plugins/canvas/scripts/shareable_runtime && node --max-old-space-size=6144 scripts/jest --verbose --detectOpenHandles --coverage
  • functional: node scripts/functional_tests --exclude-tag "skipCoverage"

Reports

Final reports are 3 separate reports: mocha, jest-combined, functional-combined (we merge coverage collected from oss & x-pack tests into a single report)

Ingestion

  • Download the coverage artifacts from GCP and merges them.
  • Adds the current timestamp to the index.html to provide 3 links in the index page, based on the current run (for Jest, Mocha and Functional).
  • Streams each coverage summary json file (mocha, jest, ftr) into the Kibana Stats cluster
    • Collects the current vcs info like author, commit msg, etc...and pushes this data to the cluster
    • The data is pushed to 1 of 2 indexes
      • kibana_code_coverage
      • kibana_total_code_coverage totals index
  • Example data
{
        "testRunnerType": "JEST",
        "jsonSummaryPath": "/Users/tre/development/projects/kibana/src/dev/code_coverage/ingest_coverage/integration_tests/mocks/jest-combined/coverage-summary-manual-mix-for-testing-kibana-path-error-dima-found.json",
        "staticSiteUrl": "https://kibana-coverage.elastic.dev/2020-03-11T03:31:35Z/jest-combined/src/legacy/core_plugins/kibana/public/discover/get_inner_angular.ts.html",
        "lines": {
          "total": 4,
          "covered": 4,
          "skipped": 0,
          "pct": 100
        },
        "functions": {
          "total": 1,
          "covered": 1,
          "skipped": 0,
          "pct": 100
        },
        "statements": {
          "total": 4,
          "covered": 4,
          "skipped": 0,
          "pct": 100
        },
        "branches": {
          "total": 0,
          "covered": 0,
          "skipped": 0,
          "pct": 100
        },
        "originalFilePath": "/var/lib/jenkins/workspace/elastic+kibana+code-coverage/kibana/src/legacy/core_plugins/kibana/public/discover/get_inner_angular.ts",
        "COVERAGE_INGESTION_KIBANA_ROOT": "/var/lib/jenkins/workspace/elastic+kibana+code-coverage/kibana",
        "BUILD_ID": "407",
        "@timestamp": "2020-03-11T03:31:35Z",
        "coveredFilePath": "src/legacy/core_plugins/kibana/public/discover/get_inner_angular.ts",
        "vcs": {
          "branch": "origin/ingest-code-coverage",
          "sha": "f07b34f6206",
          "author": "Tre' Seymour",
          "commitMsg": "Lorem :) ipsum Tre' λ dolor sit amet, consectetur ...",
          "vcsUrl": "https://github.com/elastic/kibana/commit/f07b34f6206"
        },
        "ciRunUrl": "https://kibana-ci.elastic.co/job/elastic+kibana+code-coverage/407/",
        "isTotal": false
      }

Checklist

Delete any items that are not applicable to this PR.

  • Documentation was added for features that require explanation or tutorials
  • Unit or functional tests were updated or added to match the most common scenarios
    • The pipeline is tested here
    • The ingestion is integration tested: src/dev/code_coverage/ingest_coverage/integration_tests/ingest_coverage.test.js

…/kibana into ingest-code-coverage-WITH-MASTER
@wayneseymour
Copy link
Member Author

@tylersmalley thanks for the review sir! :)

@tylersmalley
Copy link
Contributor

@wayneseymour, went through the code but I have a few questions about the overall approach.

What was the reasoning behind publishing the code coverage reports over generating the standard XML and using something like CodeCOV? I am wondering how many people will actually pull up the reports to figure out what should have coverage added. In addition, there is no way for a reviewer to utilize these reports to understand how the coverage is effected.

Lastly, I am concerned with including code coverage of functional tests. This code coverage does not carry the same weight as that of unit tests, and I would argue create a false sense of confidence when there is coverage. We should strive to have code coverage through unit tests, and functional tests should be another matter.

@wayneseymour
Copy link
Member Author

@wayneseymour, went through the code but I have a few questions about the overall approach.

What was the reasoning behind publishing the code coverage reports over generating the standard XML and using something like CodeCOV? I am wondering how many people will actually pull up the reports to figure out what should have coverage added. In addition, there is no way for a reviewer to utilize these reports to understand how the coverage is effected.

Lastly, I am concerned with including code coverage of functional tests. This code coverage does not carry the same weight as that of unit tests, and I would argue create a false sense of confidence when there is coverage. We should strive to have code coverage through unit tests, and functional tests should be another matter.

Well, the first question is good for @dmlemeshko to answer, perhaps tonight as he's in Europe.
The 2nd question sounds like it's good for @LeeDr

Sorry, I just built what I was asked! lol nice to not be in charge sometimes! lol

@wayneseymour
Copy link
Member Author

@tylersmalley @dmlemeshko @LeeDr perhaps a zoom is in order?

@LeeDr
Copy link
Contributor

LeeDr commented May 12, 2020

@tylersmalley previous meetings with @spalger and our team lead led us down the path to use Istanbul/nyc and to ingest the data into Elasticsearch so we could use Kibana dashboards.
We upload the reports to a site so users don't have to download the code coverage tar.gz files and extract them to be able to use them. And so that we can link directly to them from Kibana.

I agree that the functional test coverage can be misleading in that it shows the code executed even if a test didn't verify everything on the page. But what it does show in combination with the other test types, and is important, is what code wasn't executed at all. And of course, it's very easy to filter out from any reports you want to look at.

We should strive to have code coverage through unit tests, and functional tests should be another matter.

I agree. But that doesn't mean we shouldn't have the data on functional tests. There are areas of code that we can't unit test.

It's important for people to understand that you cannot take the average of the different types of tests, nor can you take the sum of them. There's almost certainly a lot of overlap between the different types of tests.

If unit tests show 40% coverage and functional tests show 50% coverage, all you can really say is that you have at least 40% coverage (not 45%, not 50%, not 90%).

vars/kibanaCoverage.groovy Outdated Show resolved Hide resolved
vars/kibanaCoverage.groovy Outdated Show resolved Hide resolved
vars/kibanaCoverage.groovy Outdated Show resolved Hide resolved
@tylersmalley
Copy link
Contributor

tylersmalley commented May 12, 2020

We have sync'd with @LeeDr and are fine if you move forward given QA will be owning the project. Given that, would you mind adding an exclusion for src/dev/code_coverage in the CODEOWNERS to prevent Ops from getting pings?

Also, oddly enough, I am not seeing code coverage for anything in src/dev including code coverage within the reports.

@wayneseymour
Copy link
Member Author

We have sync'd with @LeeDr and are fine if you move forward given QA will be owning the project. Given that, would you mind adding an exclusion for src/dev/code_coverage in the CODEOWNERS to prevent Ops from getting pings?

Also, oddly enough, I am not seeing code coverage for anything in src/dev including code coverage within the reports.

Why kind sir, art thou suggestion we "eat our own dog food"?! :)
I'll fixup

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@wayneseymour wayneseymour merged commit 4853f24 into master May 13, 2020
@wayneseymour wayneseymour deleted the ingest-code-coverage-WITH-MASTER branch May 13, 2020 21:09
@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label May 15, 2020
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 62336 or prevent reminders by adding the backport:skip label.

1 similar comment
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 62336 or prevent reminders by adding the backport:skip label.

@LeeDr LeeDr added the backport:skip This commit does not require backporting label May 18, 2020
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label May 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes test-coverage issues & PRs for improving code test coverage v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants