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

Added detailed test coverage report. #513

Merged
merged 2 commits into from
Aug 15, 2024

Conversation

dblock
Copy link
Member

@dblock dblock commented Aug 15, 2024

Description

Added a new --coverage-report option that displays a hierarchy of paths that are not tested. This is helpful to see if an entire API is tested vs. what's missing.

For example, the /_alias API is missing 4 tests and /_plugins are missing 46, including 11 in /_plugins/_knn.

  /_alias (4)
    GET /_alias
   /{name} (3)
     GET /_alias/{name}
     POST /_alias/{name}
     HEAD /_alias/{name}
/_plugins (46)
   /_knn (11)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: dblock <dblock@amazon.com>
Copy link
Contributor

github-actions bot commented Aug 15, 2024

Changes Analysis

Commit SHA: 74b4b99
Comparing To SHA: df34927

API Changes

Summary

NO CHANGES

Report

The full API changes report is available at: https://github.com/opensearch-project/opensearch-api-specification/actions/runs/10408668486/artifacts/1816987687

API Coverage

Before After Δ
Covered (%) 524 (51.32 %) 524 (51.32 %) 0 (0 %)
Uncovered (%) 497 (48.68 %) 497 (48.68 %) 0 (0 %)
Unknown 26 26 0

Copy link
Contributor

Spec Test Coverage Analysis

Total Tested
550 216 (39.27 %)

TESTING_GUIDE.md Outdated
```json
{
"evaluated_paths_count": 214,
"paths_count": 550,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: "paths_count" is ambiguous. Can we do total_paths_count or exposed_paths_count?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possibly, but this is existing code, so let's not scope creep it.

The spec test coverage is defined as follows.

export interface SpecTestCoverage {
  paths_count: number
  evaluated_paths_count: number,
  evaluated_paths_pct: number
}

In this context the paths count represents the spec, so adding spec_ would duplicate these semantics.

log_coverage_report(results: TestResults): void {
console.log()
console.log(`${results.unevaluated_paths().length} paths remaining.`)
const groups = _.groupBy(results.unevaluated_paths(), (path) => path.split(' ', 2)[1].split('/')[1])
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wondering where the space ' ' character came from because an URL path doesn't contain any space character. Then I saw this. The combination of an http verb and an url path constitutes an API operation. I think we should name these entities unevaluated_operations and such to avoid confusion.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, that's a good suggestion, I'll do a bulk rename.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've refactored the path parsing into an Operation object, too.

Signed-off-by: dblock <dblock@amazon.com>
@nhtruong nhtruong merged commit 3b02ba3 into opensearch-project:main Aug 15, 2024
17 checks passed
@dblock dblock deleted the test-coverage-report branch August 15, 2024 19:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants