From 953fa725a699b5869172eb471252c7936528be77 Mon Sep 17 00:00:00 2001 From: dblock Date: Tue, 14 Jan 2025 15:50:18 -0500 Subject: [PATCH] Warn if file path is invalid. Signed-off-by: dblock --- CHANGELOG.md | 1 + TESTING_GUIDE.md | 10 +++++ json_schemas/test_story.schema.yaml | 4 ++ tests/default/_core/info.yaml | 2 + tools/src/tester/StoryEvaluator.ts | 37 ++++++++++++++++--- tools/src/tester/types/story.types.ts | 4 ++ tools/tests/tester/StoryValidator.test.ts | 5 ++- .../fixtures/evals/error/chapter_error.yaml | 5 ++- .../fixtures/evals/error/output_error.yaml | 2 + .../fixtures/evals/error/prologue_error.yaml | 3 ++ .../fixtures/evals/failed/not_found.yaml | 4 +- .../tester/fixtures/evals/passed/passed.yaml | 3 ++ .../fixtures/evals/passed/value_type.yaml | 4 +- 13 files changed, 73 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3cb728001..815871953 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -52,6 +52,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) - Added `GET /_plugins/_ml/profile`, `GET /_plugins/_ml/profile/models`, `models/{model_id}`, `tasks`, `tasks/{task_id}` ([#787](https://github.com/opensearch-project/opensearch-api-specification/pull/787)) - Added `GET /_plugins/_ml/stats/`, `stats/{stat}`, `{nodeId}/stats/`, `{nodeId}/stats/{stat}` ([#794](https://github.com/opensearch-project/opensearch-api-specification/pull/794)) - Added `GET`, `POST /_plugins/_ml/tasks/_search`, `GET /_plugins/_ml/tools`, `tools/{tool_name}` ([#797](https://github.com/opensearch-project/opensearch-api-specification/pull/797)) +- Added a warning for test file names that don't match the API being tested ([#793](https://github.com/opensearch-project/opensearch-api-specification/pull/793)) ### Removed - Removed unsupported `_common.mapping:SourceField`'s `mode` field and associated `_common.mapping:SourceFieldMode` enum ([#652](https://github.com/opensearch-project/opensearch-api-specification/pull/652)) diff --git a/TESTING_GUIDE.md b/TESTING_GUIDE.md index df2a2526f..8236b235c 100644 --- a/TESTING_GUIDE.md +++ b/TESTING_GUIDE.md @@ -20,6 +20,7 @@ - [Waiting for Tasks](#waiting-for-tasks) - [Warnings](#warnings) - [multiple-paths-detected](#multiple-paths-detected) + - [invalid-path-detected](#invalid-path-detected) - [Suppressing Warnings](#suppressing-warnings) - [Collecting Test Coverage](#collecting-test-coverage) - [Coverage Summary](#coverage-summary) @@ -367,6 +368,15 @@ WARNING Multiple paths detected, please group similar tests together and move pa /{index} ``` +#### invalid-path-detected + +The test file names expect to match the API being tested. Otherwise, a warning will be emitted. + +``` +PASSED _core/rank_eval.yaml (tests/default/_core/rank_eval.yaml) +WARNING Invalid path detected, please move /tests/default/_core/rank_eval.yaml to /rankeval.yaml. +``` + #### Suppressing Warnings The test runner may generate warnings that can be suppressed with `warnings:` at story or chapter level. For example, to suppress the multiple paths detected warning. diff --git a/json_schemas/test_story.schema.yaml b/json_schemas/test_story.schema.yaml index 01b742b51..a0bc7ba23 100644 --- a/json_schemas/test_story.schema.yaml +++ b/json_schemas/test_story.schema.yaml @@ -252,4 +252,8 @@ definitions: type: boolean default: true description: Enable/disable warnings about multiple paths being tested in the same story. + invalid-path-detected: + type: boolean + default: true + description: Enable/disable warnings about file paths that do not match paths tested in the story. additionalProperties: false diff --git a/tests/default/_core/info.yaml b/tests/default/_core/info.yaml index 813ef6ab6..f938996aa 100644 --- a/tests/default/_core/info.yaml +++ b/tests/default/_core/info.yaml @@ -2,6 +2,8 @@ $schema: ../../../json_schemas/test_story.schema.yaml description: Test root endpoint. +warnings: + invalid-path-detected: false distributions: excluded: - amazon-serverless diff --git a/tools/src/tester/StoryEvaluator.ts b/tools/src/tester/StoryEvaluator.ts index a032f9369..9dc7fa11b 100644 --- a/tools/src/tester/StoryEvaluator.ts +++ b/tools/src/tester/StoryEvaluator.ts @@ -18,6 +18,7 @@ import * as semver from '../_utils/semver' import _ from 'lodash' import { ParsedChapter, ParsedStory } from './types/parsed_story.types' import { OutputReference } from './OutputReference' +import * as path from 'path' export default class StoryEvaluator { private readonly _chapter_evaluator: ChapterEvaluator @@ -79,7 +80,7 @@ export default class StoryEvaluator { result: overall_result(prologues.concat(chapters).concat(epilogues).concat(prologues).map(e => e.overall)), } - const warnings = this.#chapter_warnings(story) + const warnings = this.#chapter_warnings(story, full_path) if (warnings !== undefined) { result.warnings = warnings } @@ -87,10 +88,11 @@ export default class StoryEvaluator { return result } - #chapter_warnings(story: ParsedStory): string[] | undefined { - const result = _.compact([ - this.#warning_if_mismatched_chapter_paths(story) - ]) + #chapter_warnings(story: ParsedStory, full_path: string): string[] | undefined { + const result = _.compact(_.flattenDeep([ + [this.#warning_if_mismatched_chapter_paths(story)], + this.#warning_if_invalid_path(story, full_path) + ])) return result.length > 0 ? result : undefined } @@ -103,10 +105,33 @@ export default class StoryEvaluator { const normalized_paths = _.map(paths, (path) => path.replaceAll(/\/\{[^}]+}/g, '').replaceAll('//', '/')) 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` + return `Multiple paths detected, please group similar tests together and move paths not being tested to prologues or epilogues.\n ${_.join(_.uniq(paths), "\n ")}` } } + #warning_if_invalid_path(story: ParsedStory, full_path: string): string[] | undefined { + if (story.warnings?.['invalid-path-detected'] === false) return + const paths = _.compact(_.map(story.chapters, (chapter) => { + if (chapter.warnings?.['multiple-paths-detected'] === false) return // not the path being tested + return chapter.path + })) + const normalized_paths = _.uniq(_.map(paths, (path) => + path + .replaceAll('/_plugins/', '') + .replaceAll(/\/\{[^}]+}/g, '') + .replaceAll('//', '/') + .replaceAll('_', '') + + '.yaml' + )) + + return _.compact(_.map(normalized_paths, (normalized_path) => { + if (!full_path.endsWith(normalized_path)) { + const relative_path = path.relative('.', full_path) + return `Invalid path detected, please move /${relative_path} to ${normalized_path}.` + } + })) + } + async #evaluate_chapters(chapters: ParsedChapter[], has_errors: boolean, dry_run: boolean, story_outputs: StoryOutputs, version?: string, distribution?: string): Promise { const evaluations: ChapterEvaluation[] = [] for (const chapter of chapters) { diff --git a/tools/src/tester/types/story.types.ts b/tools/src/tester/types/story.types.ts index 2e48e38af..c0c40cbbc 100644 --- a/tools/src/tester/types/story.types.ts +++ b/tools/src/tester/types/story.types.ts @@ -202,6 +202,10 @@ export interface Warnings { * Enable/disable warnings about multiple paths being tested in the same story. */ 'multiple-paths-detected'?: boolean; + /** + * Enable/disable warnings about file paths that do not match paths tested in the story. + */ + 'invalid-path-detected'?: boolean; } /** * This interface was referenced by `Story`'s JSON-Schema diff --git a/tools/tests/tester/StoryValidator.test.ts b/tools/tests/tester/StoryValidator.test.ts index aa28fdcc0..a6850e845 100644 --- a/tools/tests/tester/StoryValidator.test.ts +++ b/tools/tests/tester/StoryValidator.test.ts @@ -31,10 +31,11 @@ describe('StoryValidator', () => { const evaluation = validate('tools/tests/tester/fixtures/invalid_story.yaml') expect(evaluation?.result).toBe('ERROR') expect(evaluation?.message).toBe("Invalid Story: " + - "data/epilogues/0 contains unsupported properties: response --- " + "data/chapters/0 MUST contain the missing properties: method --- " + "data/chapters/1/method MUST be equal to one of the allowed values: GET, PUT, POST, DELETE, PATCH, HEAD, OPTIONS --- " + - "data/chapters/1/method must be array --- data/chapters/1/method must match exactly one schema in oneOf") + "data/chapters/1/method must be array --- " + + "data/chapters/1/method must match exactly one schema in oneOf" + ) }) test('invalid description', () => { diff --git a/tools/tests/tester/fixtures/evals/error/chapter_error.yaml b/tools/tests/tester/fixtures/evals/error/chapter_error.yaml index 331667e99..08e4288a0 100644 --- a/tools/tests/tester/fixtures/evals/error/chapter_error.yaml +++ b/tools/tests/tester/fixtures/evals/error/chapter_error.yaml @@ -1,3 +1,4 @@ +# eslint-disable yml/sort-sequence-values display_path: error/chapter_error.yaml full_path: tools/tests/tester/fixtures/stories/error/chapter_error.yaml @@ -5,11 +6,13 @@ result: ERROR description: This story should failed due to missing info in the spec. warnings: - - | + - >- Multiple paths detected, please group similar tests together and move paths not being tested to prologues or epilogues. /{index}/settings /{index} /_cat/indices + - Invalid path detected, please move /tools/tests/tester/fixtures/stories/error/chapter_error.yaml to /settings.yaml. + - Invalid path detected, please move /tools/tests/tester/fixtures/stories/error/chapter_error.yaml to /cat/indices.yaml. prologues: - title: PUT /books overall: diff --git a/tools/tests/tester/fixtures/evals/error/output_error.yaml b/tools/tests/tester/fixtures/evals/error/output_error.yaml index 5a7e03bb3..3175978d2 100644 --- a/tools/tests/tester/fixtures/evals/error/output_error.yaml +++ b/tools/tests/tester/fixtures/evals/error/output_error.yaml @@ -2,6 +2,8 @@ display_path: error/output_error.yaml full_path: tools/tests/tester/fixtures/stories/error/output_error.yaml result: ERROR +warnings: + - Invalid path detected, please move /tools/tests/tester/fixtures/stories/error/output_error.yaml to /cat/health.yaml. description: This story has an error in the output. prologues: [] epilogues: [] diff --git a/tools/tests/tester/fixtures/evals/error/prologue_error.yaml b/tools/tests/tester/fixtures/evals/error/prologue_error.yaml index c27415bf3..f8cec4a2c 100644 --- a/tools/tests/tester/fixtures/evals/error/prologue_error.yaml +++ b/tools/tests/tester/fixtures/evals/error/prologue_error.yaml @@ -2,6 +2,9 @@ display_path: error/prologue_error.yaml full_path: tools/tests/tester/fixtures/stories/error/prologue_error.yaml result: ERROR +warnings: + - Invalid path detected, please move /tools/tests/tester/fixtures/stories/error/prologue_error.yaml to /cat/health.yaml. + - Invalid path detected, please move /tools/tests/tester/fixtures/stories/error/prologue_error.yaml to /cat/indices.yaml. description: This story should failed due to missing info in the spec. prologues: - title: PUT /books diff --git a/tools/tests/tester/fixtures/evals/failed/not_found.yaml b/tools/tests/tester/fixtures/evals/failed/not_found.yaml index 9a25ad3ee..dca949aec 100644 --- a/tools/tests/tester/fixtures/evals/failed/not_found.yaml +++ b/tools/tests/tester/fixtures/evals/failed/not_found.yaml @@ -1,3 +1,4 @@ +# eslint-disable yml/sort-sequence-values display_path: failed/not_found.yaml full_path: tools/tests/tester/fixtures/stories/failed/not_found.yaml @@ -5,10 +6,11 @@ result: FAILED description: This story should failed due to missing info in the spec. prologues: [] warnings: - - | + - >- Multiple paths detected, please group similar tests together and move paths not being tested to prologues or epilogues. /_cat/does_not_exist /{index} + - Invalid path detected, please move /tools/tests/tester/fixtures/stories/failed/not_found.yaml to /cat/doesnotexist.yaml. chapters: - title: This chapter should fail because the operation is not defined in the spec. overall: diff --git a/tools/tests/tester/fixtures/evals/passed/passed.yaml b/tools/tests/tester/fixtures/evals/passed/passed.yaml index a87c944c9..60165e1f4 100644 --- a/tools/tests/tester/fixtures/evals/passed/passed.yaml +++ b/tools/tests/tester/fixtures/evals/passed/passed.yaml @@ -2,6 +2,9 @@ display_path: passed/passed.yaml full_path: tools/tests/tester/fixtures/stories/passed/passed.yaml result: PASSED +warnings: + - Invalid path detected, please move /tools/tests/tester/fixtures/stories/passed/passed.yaml to /cat.yaml. + - Invalid path detected, please move /tools/tests/tester/fixtures/stories/passed/passed.yaml to /cat/health.yaml. description: This story should pass. prologues: [] chapters: diff --git a/tools/tests/tester/fixtures/evals/passed/value_type.yaml b/tools/tests/tester/fixtures/evals/passed/value_type.yaml index ec5a64be9..c29dca520 100644 --- a/tools/tests/tester/fixtures/evals/passed/value_type.yaml +++ b/tools/tests/tester/fixtures/evals/passed/value_type.yaml @@ -1,13 +1,15 @@ +# eslint-disable yml/sort-sequence-values display_path: passed/value_type.yaml full_path: tools/tests/tester/fixtures/stories/passed/value_type.yaml result: PASSED description: This story has an error trying to reuse a variable of a different type. warnings: - - | + - >- Multiple paths detected, please group similar tests together and move paths not being tested to prologues or epilogues. /_cat/health /{index} + - Invalid path detected, please move /tools/tests/tester/fixtures/stories/passed/value_type.yaml to /cat/health.yaml. prologues: - overall: result: PASSED