From 3b02ba3af21935c0c17c2e7e307c766697f42e93 Mon Sep 17 00:00:00 2001 From: "Daniel (dB.) Doubrovkine" Date: Thu, 15 Aug 2024 15:08:02 -0400 Subject: [PATCH] Added detailed test coverage report. (#513) * Added detailed test coverage report. Signed-off-by: dblock --- .../pr-test-coverage-analysis.template.md | 6 +-- CHANGELOG.md | 1 + TESTING_GUIDE.md | 32 +++++++++++++ tools/src/tester/ChapterEvaluator.ts | 4 ++ tools/src/tester/ResultLogger.ts | 28 ++++++++++- tools/src/tester/StoryEvaluator.ts | 4 +- tools/src/tester/TestResults.ts | 46 ++++++++++++++----- tools/src/tester/test.ts | 2 + tools/src/tester/types/eval.types.ts | 6 +++ tools/src/tester/types/test.types.ts | 4 +- tools/tests/tester/ResultLogger.test.ts | 45 +++++++++++++++++- tools/tests/tester/TestResults.test.ts | 40 ++++++++++++---- .../fixtures/evals/error/chapter_error.yaml | 3 ++ .../fixtures/evals/error/output_error.yaml | 3 ++ .../fixtures/evals/failed/invalid_data.yaml | 15 ++++++ .../fixtures/evals/failed/not_found.yaml | 9 ++++ tools/tests/tester/fixtures/evals/passed.yaml | 30 ++++++++++++ .../tests/tester/integ/StoryEvaluator.test.ts | 4 ++ 18 files changed, 253 insertions(+), 29 deletions(-) diff --git a/.github/pr-comment-templates/pr-test-coverage-analysis.template.md b/.github/pr-comment-templates/pr-test-coverage-analysis.template.md index 27f0fe7b5..212dd595b 100644 --- a/.github/pr-comment-templates/pr-test-coverage-analysis.template.md +++ b/.github/pr-comment-templates/pr-test-coverage-analysis.template.md @@ -1,8 +1,8 @@ ## Spec Test Coverage Analysis {{with .test_coverage}} -| Total | Tested | -|-------------------|----------------------------------------------------------| -| {{.paths_count}} | {{.evaluated_paths_count}} ({{.evaluated_paths_pct}} %) | +| Total | Tested | +|------------------------------|---------------------------------------------------------------| +| {{.total_operations_count}} | {{.evaluated_operations_count}} ({{.evaluated_paths_pct}} %) | {{end}} \ No newline at end of file diff --git a/CHANGELOG.md b/CHANGELOG.md index b8fd3cefc..ebfbabd1c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -77,6 +77,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) - Added `read_time`, `write_time`, `queue_size` and `io_time_in_millis` to `IoStatDevice` ([#483](https://github.com/opensearch-project/opensearch-api-specification/pull/483)) - Added `total_rejections_breakup` to `ShardIndexingPressureStats` ([#483](https://github.com/opensearch-project/opensearch-api-specification/pull/483)) - Added `cancelled_task_percentage` and `current_cancellation_eligible_tasks_count` to `ShardSearchBackpressureTaskCancellationStats` ([#483](https://github.com/opensearch-project/opensearch-api-specification/pull/483)) +- Added detailed test coverage report ([#513](https://github.com/opensearch-project/opensearch-api-specification/pull/513)) ### Changed diff --git a/TESTING_GUIDE.md b/TESTING_GUIDE.md index 898b90be3..9aa53fca2 100644 --- a/TESTING_GUIDE.md +++ b/TESTING_GUIDE.md @@ -16,6 +16,9 @@ - [Warnings](#warnings) - [multiple-paths-detected](#multiple-paths-detected) - [Suppressing Warnings](#suppressing-warnings) + - [Collecting Test Coverage](#collecting-test-coverage) + - [Coverage Summary](#coverage-summary) + - [Coverage Report](#coverage-report) # Spec Testing Guide @@ -318,3 +321,32 @@ The test runner may generate warnings that can be suppressed with `warnings:` at parameters: index: movies ``` + +## Collecting Test Coverage + +### Coverage Summary + +The test tool can generate a test coverage summary using `--coverage ` with the number of evaluated verb + path combinations, a total number of paths and the % of paths tested. + +```json +{ + "evaluated_operations_count": 214, + "operations_count": 550, + "evaluated_paths_pct": 38.91 +} +``` + +The report is then used by the [test-spec.yml](.github/workflows/test-spec.yml) workflow, uploaded with every run, combined across various versions of OpenSearch, and reported as a comment on each pull request. + +### Coverage Report + +The test tool can display detailed and hierarchal test coverage with `--coverage-report`. This is useful to identify untested paths. For example, the [put_alias.yaml](tests/default/indices/aliases/put_alias.yaml) test exercises `PUT /_alias/{name}`, but not the other verbs. The report produces the following output with the missing ones. + +``` +/_alias (4) + GET /_alias + /{name} (3) + GET /_alias/{name} + POST /_alias/{name} + HEAD /_alias/{name} +``` \ No newline at end of file diff --git a/tools/src/tester/ChapterEvaluator.ts b/tools/src/tester/ChapterEvaluator.ts index dee177ddc..6feb7d0a0 100644 --- a/tools/src/tester/ChapterEvaluator.ts +++ b/tools/src/tester/ChapterEvaluator.ts @@ -84,6 +84,10 @@ export default class ChapterEvaluator { var result: ChapterEvaluation = { title: chapter.synopsis, + operation: { + method: chapter.method, + path: chapter.path + }, path: `${chapter.method} ${chapter.path}`, overall: { result: overall_result(evaluations) }, request: { parameters: params, request }, diff --git a/tools/src/tester/ResultLogger.ts b/tools/src/tester/ResultLogger.ts index d2ef339c2..0d6d2c929 100644 --- a/tools/src/tester/ResultLogger.ts +++ b/tools/src/tester/ResultLogger.ts @@ -7,10 +7,11 @@ * compatible open source license. */ -import { type ChapterEvaluation, type Evaluation, Result, type StoryEvaluation } from './types/eval.types' +import { type ChapterEvaluation, type Evaluation, Operation, Result, type StoryEvaluation } from './types/eval.types' import { overall_result } from './helpers' import * as ansi from './Ansi' import TestResults from './TestResults' +import _ from 'lodash' export interface ResultLogger { log: (evaluation: StoryEvaluation) => void @@ -43,7 +44,30 @@ export class ConsoleResultLogger implements ResultLogger { log_coverage(results: TestResults): void { console.log() - console.log(`Tested ${results.evaluated_paths_count()}/${results.spec_paths_count()} paths.`) + console.log(`Tested ${results.evaluated_operations().length}/${results.operations().length} paths.`) + } + + log_coverage_report(results: TestResults): void { + console.log() + console.log(`${results.unevaluated_operations().length} paths remaining.`) + const groups = _.groupBy(results.unevaluated_operations(), (operation) => operation.path.split('/')[1]) + Object.entries(groups).forEach(([root, operations]) => { + this.#log_coverage_group(root, operations) + }); + } + + #log_coverage_group(key: string, operations: Operation[], index: number = 2): void { + if (operations.length == 0 || key == undefined) return + console.log(`${' '.repeat(index)}/${key} (${operations.length})`) + const current_level_operations = operations.filter((operation) => operation.path.split('/').length == index) + current_level_operations.forEach((operation) => { + console.log(`${' '.repeat(index + 2)}${operation.method} ${operation.path}`) + }) + const next_level_operations = operations.filter((operation) => operation.path.split('/').length > index) + const subgroups = _.groupBy(next_level_operations, (operation) => operation.path.split('/')[index]) + Object.entries(subgroups).forEach(([root, operations]) => { + this.#log_coverage_group(root, operations, index + 1) + }); } #log_story ({ result, full_path, display_path, message, warnings }: StoryEvaluation): void { diff --git a/tools/src/tester/StoryEvaluator.ts b/tools/src/tester/StoryEvaluator.ts index fa1e46918..e464f3f60 100644 --- a/tools/src/tester/StoryEvaluator.ts +++ b/tools/src/tester/StoryEvaluator.ts @@ -99,8 +99,8 @@ export default class StoryEvaluator { return chapter.path })) const normalized_paths = _.map(paths, (path) => path.replaceAll(/\/\{[^}]+}/g, '').replaceAll('//', '/')) - const paths_counts: Record = Object.assign((_.values(_.groupBy(normalized_paths)).map(p => { return { [p[0]] : p.length } }))) - if (paths_counts.length > 1) { + const operations_counts: Record = Object.assign((_.values(_.groupBy(normalized_paths)).map(p => { return { [p[0]] : p.length } }))) + if (operations_counts.length > 1) { return `Multiple paths detected, please group similar tests together and move paths not being tested to prologues or epilogues.\n ${_.join(_.uniq(paths), "\n ")}\n` } } diff --git a/tools/src/tester/TestResults.ts b/tools/src/tester/TestResults.ts index c4ae26b78..792699e20 100644 --- a/tools/src/tester/TestResults.ts +++ b/tools/src/tester/TestResults.ts @@ -9,35 +9,59 @@ import _ from "lodash"; import MergedOpenApiSpec from "./MergedOpenApiSpec"; -import { StoryEvaluations } from "./types/eval.types"; +import { Operation, StoryEvaluations } from "./types/eval.types"; import { SpecTestCoverage } from "./types/test.types"; import { write_json } from "../helpers"; export default class TestResults { protected _spec: MergedOpenApiSpec protected _evaluations: StoryEvaluations + protected _evaluated_operations?: Operation[] + protected _unevaluated_operations?: Operation[] + protected _operations?: Operation[] constructor(spec: MergedOpenApiSpec, evaluations: StoryEvaluations) { this._spec = spec this._evaluations = evaluations } - evaluated_paths_count(): number { - return _.uniq(_.compact(_.flatten(_.map(this._evaluations.evaluations, (evaluation) => - _.map(evaluation.chapters, (chapter) => chapter.path) - )))).length + evaluated_operations(): Operation[] { + if (this._evaluated_operations !== undefined) return this._evaluated_operations + this._evaluated_operations = _.uniq(_.compact(_.flatMap(this._evaluations.evaluations, (evaluation) => + _.map(evaluation.chapters, (chapter) => chapter.operation) + ))) + return this._evaluated_operations } - spec_paths_count(): number { - return Object.values(this._spec.paths()).reduce((acc, methods) => acc + methods.length, 0); + unevaluated_operations(): Operation[] { + if (this._unevaluated_operations !== undefined) return this._unevaluated_operations + this._unevaluated_operations = this.operations().filter((operation) => + !_.find(this.evaluated_operations(), + (op) => + operation.method == op.method && + operation.path == op.path + ) + ) + return this._unevaluated_operations + } + + operations(): Operation[] { + if (this._operations !== undefined) return this._operations + this._operations = _.uniq(Object.entries(this._spec.paths()).flatMap(([path, path_item]) => { + return Object.values(path_item).map((method) => { + return { method: method.toUpperCase(), path } + }) + })) + + return this._operations } test_coverage(): SpecTestCoverage { return { - evaluated_paths_count: this.evaluated_paths_count(), - paths_count: this.spec_paths_count(), - evaluated_paths_pct: this.spec_paths_count() > 0 ? Math.round( - this.evaluated_paths_count() / this.spec_paths_count() * 100 * 100 + evaluated_operations_count: this.evaluated_operations().length, + total_operations_count: this.operations().length, + evaluated_paths_pct: this.operations().length > 0 ? Math.round( + this.evaluated_operations().length / this.operations().length * 100 * 100 ) / 100 : 0, } } diff --git a/tools/src/tester/test.ts b/tools/src/tester/test.ts index da025a34b..e781e343c 100644 --- a/tools/src/tester/test.ts +++ b/tools/src/tester/test.ts @@ -60,6 +60,7 @@ const command = new Command() .addOption(AWS_REGION_OPTION) .addOption(AWS_SERVICE_OPTION) .addOption(new Option('--coverage ', 'path to write test coverage results to')) + .addOption(new Option('--coverage-report', 'display a detailed test coverage report')) .allowExcessArguments(false) .parse() @@ -82,6 +83,7 @@ runner.run(opts.testsPath, spec.api_version(), opts.opensearchDistribution, opts const test_results = new TestResults(spec, results) result_logger.log_coverage(test_results) + if (opts.coverageReport) result_logger.log_coverage_report(test_results) if (opts.coverage !== undefined) { console.log(`Writing ${opts.coverage} ...`) test_results.write_coverage(opts.coverage) diff --git a/tools/src/tester/types/eval.types.ts b/tools/src/tester/types/eval.types.ts index a95104c50..eacb2c62a 100644 --- a/tools/src/tester/types/eval.types.ts +++ b/tools/src/tester/types/eval.types.ts @@ -17,6 +17,11 @@ export interface StoryFile { story: Story } +export interface Operation { + method: string + path: string +} + export interface StoryEvaluation { result: Result display_path: string @@ -36,6 +41,7 @@ export interface StoryEvaluations { export interface ChapterEvaluation { title: string, overall: Evaluation, + operation?: Operation, path?: string, request?: { parameters?: Record diff --git a/tools/src/tester/types/test.types.ts b/tools/src/tester/types/test.types.ts index c0e14717a..174f207c0 100644 --- a/tools/src/tester/types/test.types.ts +++ b/tools/src/tester/types/test.types.ts @@ -8,7 +8,7 @@ */ export interface SpecTestCoverage { - paths_count: number - evaluated_paths_count: number, + total_operations_count: number + evaluated_operations_count: number, evaluated_paths_pct: number } diff --git a/tools/tests/tester/ResultLogger.test.ts b/tools/tests/tester/ResultLogger.test.ts index f21e45af1..512619cd9 100644 --- a/tools/tests/tester/ResultLogger.test.ts +++ b/tools/tests/tester/ResultLogger.test.ts @@ -66,7 +66,11 @@ describe('ConsoleResultLogger', () => { { title: 'title', overall: { result: Result.PASSED }, - path: 'path' + path: 'path', + operation: { + method: 'GET', + path: '/_nodes/{id}' + } } ] }] }) @@ -79,6 +83,45 @@ describe('ConsoleResultLogger', () => { ]) }) + test('log_coverage_report', () => { + const spec = new MergedOpenApiSpec('tools/tests/tester/fixtures/specs/complete') + const test_results = new TestResults(spec, { evaluations: [{ + result: Result.PASSED, + display_path: 'path', + full_path: 'path', + description: 'description', + chapters: [ + { + title: 'title', + overall: { result: Result.PASSED }, + path: 'GET /_nodes/{id}', + operation: { + method: 'GET', + path: '/_nodes/{id}' + } + } + ] + }] }) + + logger.log_coverage_report(test_results) + + expect(log.mock.calls).not.toContain(["GET /_nodes/{id}"]) + expect(log.mock.calls).toEqual([ + [], + ["5 paths remaining."], + [" /_nodes (1)"], + [" /{id} (1)"], + [" POST /_nodes/{id}"], + [" /cluster_manager (2)"], + [" GET /cluster_manager"], + [" POST /cluster_manager"], + [" /index (1)"], + [" GET /index"], + [" /nodes (1)"], + [" GET /nodes"] + ]) + }) + test('with retries', () => { logger.log({ result: Result.PASSED, diff --git a/tools/tests/tester/TestResults.test.ts b/tools/tests/tester/TestResults.test.ts index ac06a6350..d4abe8472 100644 --- a/tools/tests/tester/TestResults.test.ts +++ b/tools/tests/tester/TestResults.test.ts @@ -17,16 +17,20 @@ describe('TestResults', () => { const evaluations = [{ result: Result.PASSED, - display_path: 'path', + display_path: 'PUT /{index}', full_path: 'full_path', description: 'description', message: 'message', chapters: [{ title: 'title', + operation: { + method: 'PUT', + path: '/{index}' + }, overall: { result: Result.PASSED }, - path: 'path' + path: 'PUT /{index}' }], epilogues: [], prologues: [] @@ -34,21 +38,41 @@ describe('TestResults', () => { const test_results = new TestResults(spec, { evaluations }) - test('evaluated_paths_count', () => { - expect(test_results.evaluated_paths_count()).toEqual(1) + test('unevaluated_operations', () => { + expect(test_results.unevaluated_operations()).toEqual([ + { method: "GET", path: "/_nodes/{id}" }, + { method: "POST", path: "/_nodes/{id}" }, + { method: "GET", path: "/cluster_manager" }, + { method: "POST", path: "/cluster_manager" }, + { method: "GET", path: "/index" }, + { method: "GET", path: "/nodes" } + ]) + }) + + test('evaluated_operations', () => { + expect(test_results.evaluated_operations()).toStrictEqual([ + { method: 'PUT', path: '/{index}' } + ]) }) - test('spec_paths_count', () => { - expect(test_results.spec_paths_count()).toEqual(6) + test('operations', () => { + expect(test_results.operations()).toEqual([ + { method: "GET", path: "/_nodes/{id}" }, + { method: "POST", path: "/_nodes/{id}" }, + { method: "GET", path: "/cluster_manager" }, + { method: "POST", path: "/cluster_manager" }, + { method: "GET", path: "/index" }, + { method: "GET", path: "/nodes" } + ]) }) test('write_coverage', () => { const filename = 'coverage.json' test_results.write_coverage(filename) expect(JSON.parse(fs.readFileSync(filename, 'utf8'))).toEqual({ - evaluated_paths_count: 1, + evaluated_operations_count: 1, evaluated_paths_pct: 16.67, - paths_count: 6 + total_operations_count: 6 }) fs.unlinkSync(filename) }) diff --git a/tools/tests/tester/fixtures/evals/error/chapter_error.yaml b/tools/tests/tester/fixtures/evals/error/chapter_error.yaml index fab1f0cc4..331667e99 100644 --- a/tools/tests/tester/fixtures/evals/error/chapter_error.yaml +++ b/tools/tests/tester/fixtures/evals/error/chapter_error.yaml @@ -24,6 +24,9 @@ chapters: overall: result: ERROR path: DELETE /{index} + operation: + method: DELETE + path: /{index} request: parameters: {} request: diff --git a/tools/tests/tester/fixtures/evals/error/output_error.yaml b/tools/tests/tester/fixtures/evals/error/output_error.yaml index 27ac940d3..5a7e03bb3 100644 --- a/tools/tests/tester/fixtures/evals/error/output_error.yaml +++ b/tools/tests/tester/fixtures/evals/error/output_error.yaml @@ -11,6 +11,9 @@ chapters: overall: result: ERROR path: GET /_cat/health + operation: + method: GET + path: /_cat/health retries: 3 request: parameters: diff --git a/tools/tests/tester/fixtures/evals/failed/invalid_data.yaml b/tools/tests/tester/fixtures/evals/failed/invalid_data.yaml index 3f59fe0e8..14716a69b 100644 --- a/tools/tests/tester/fixtures/evals/failed/invalid_data.yaml +++ b/tools/tests/tester/fixtures/evals/failed/invalid_data.yaml @@ -9,6 +9,9 @@ chapters: overall: result: FAILED path: PUT /{index} + operation: + method: PUT + path: /{index} request: parameters: index: @@ -29,6 +32,9 @@ chapters: overall: result: FAILED path: PUT /{index} + operation: + method: PUT + path: /{index} request: parameters: index: @@ -49,6 +55,9 @@ chapters: overall: result: FAILED path: GET /_cat/indices/{index} + operation: + method: GET + path: /_cat/indices/{index} request: parameters: format: @@ -71,6 +80,9 @@ chapters: overall: result: FAILED path: DELETE /{index} + operation: + method: DELETE + path: /{index} request: parameters: index: @@ -92,6 +104,9 @@ chapters: overall: result: ERROR path: PUT /{index} + operation: + method: PUT + path: /{index} request: parameters: index: diff --git a/tools/tests/tester/fixtures/evals/failed/not_found.yaml b/tools/tests/tester/fixtures/evals/failed/not_found.yaml index ecbd88992..9a25ad3ee 100644 --- a/tools/tests/tester/fixtures/evals/failed/not_found.yaml +++ b/tools/tests/tester/fixtures/evals/failed/not_found.yaml @@ -18,6 +18,9 @@ chapters: overall: result: FAILED path: PUT /{index} + operation: + method: PUT + path: /{index} request: parameters: index: @@ -40,6 +43,9 @@ chapters: overall: result: FAILED path: HEAD /{index} + operation: + method: HEAD + path: /{index} request: parameters: index: @@ -60,6 +66,9 @@ chapters: overall: result: FAILED path: DELETE /{index} + operation: + method: DELETE + path: /{index} request: parameters: index: diff --git a/tools/tests/tester/fixtures/evals/passed.yaml b/tools/tests/tester/fixtures/evals/passed.yaml index abe502379..e20d6fc4a 100644 --- a/tools/tests/tester/fixtures/evals/passed.yaml +++ b/tools/tests/tester/fixtures/evals/passed.yaml @@ -9,6 +9,9 @@ chapters: overall: result: PASSED path: PUT /{index} + operation: + method: PUT + path: /{index} request: parameters: index: @@ -28,6 +31,9 @@ chapters: overall: result: PASSED path: GET /_cat + operation: + method: GET + path: /_cat request: parameters: {} request: @@ -45,6 +51,9 @@ chapters: overall: result: PASSED path: GET /_cat + operation: + method: GET + path: /_cat request: parameters: {} request: @@ -62,6 +71,9 @@ chapters: overall: result: PASSED path: GET /_cat/health + operation: + method: GET + path: /_cat/health request: parameters: format: @@ -81,6 +93,9 @@ chapters: overall: result: PASSED path: GET /_cat/health + operation: + method: GET + path: /_cat/health request: parameters: format: @@ -100,6 +115,9 @@ chapters: overall: result: PASSED path: GET /_cat/health + operation: + method: GET + path: /_cat/health request: parameters: format: @@ -119,6 +137,9 @@ chapters: overall: result: PASSED path: GET /_cat/health + operation: + method: GET + path: /_cat/health request: parameters: format: @@ -138,6 +159,9 @@ chapters: overall: result: PASSED path: GET /_cat/health + operation: + method: GET + path: /_cat/health request: parameters: format: @@ -157,6 +181,9 @@ chapters: overall: result: PASSED path: GET /_cat/health + operation: + method: GET + path: /_cat/health request: parameters: format: @@ -180,6 +207,9 @@ chapters: overall: result: PASSED path: GET /_cat/health + operation: + method: GET + path: /_cat/health request: parameters: format: diff --git a/tools/tests/tester/integ/StoryEvaluator.test.ts b/tools/tests/tester/integ/StoryEvaluator.test.ts index ac5fc60ef..f3c6bc15e 100644 --- a/tools/tests/tester/integ/StoryEvaluator.test.ts +++ b/tools/tests/tester/integ/StoryEvaluator.test.ts @@ -70,6 +70,10 @@ test('with an unexpected error deserializing data', async () => { expect(actual.chapters && actual.chapters[0]).toEqual({ title: "This PUT /{index} chapter should pass.", path: 'PUT /{index}', + operation: { + method: 'PUT', + path: '/{index}' + }, overall: { result: Result.ERROR },