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 StoryValidator #362

Merged
merged 1 commit into from
Jun 26, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
- Added `cancel_after_time_interval` and `phase_took` in `_search` ([#353](https://github.com/opensearch-project/opensearch-api-specification/pull/353))
- Added support for testing `application/x-ndjson` payloads ([#355](https://github.com/opensearch-project/opensearch-api-specification/pull/355))
- Added SPECIFICATION_TESTING.md [#359](https://github.com/opensearch-project/opensearch-api-specification/pull/359)
- Added StoryValidator to validate stories before running them ([#354](https://github.com/opensearch-project/opensearch-api-specification/issues/354))

### Changed

Expand Down
2 changes: 2 additions & 0 deletions eslint.config.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ export default [
...pluginComments.configs.recommended.rules,
...pluginTs.configs["recommended-type-checked"].rules,
'@stylistic/object-curly-spacing': ["error", "always"],
'@stylistic/keyword-spacing': ["error", { "before": true, "after": true }],
'@stylistic/space-unary-ops': ["error", { "words": true, "nonwords": false }],
'@typescript-eslint/no-unsafe-member-access': 'off',
'@typescript-eslint/no-explicit-any': 'off',
'@typescript-eslint/no-unsafe-assignment': 'off',
Expand Down
11 changes: 4 additions & 7 deletions json_schemas/test_story.schema.yaml
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
$schema: http://json-schema.org/draft-07/schema#
$schema: https://json-schema.org/draft/2019-09/schema

type: object
properties:
Expand All @@ -18,13 +18,12 @@ properties:
type: array
items:
$ref: '#/definitions/Chapter'
minItems: 1
required: [description, chapters]
required: [$schema, description, chapters]
additionalProperties: false

definitions:
Chapter:
type: object
unevaluatedProperties: false
allOf:
- $ref: '#/definitions/ChapterRequest'
- properties:
Expand All @@ -34,7 +33,6 @@ definitions:
response:
$ref: '#/definitions/ExpectedResponse'
required: [synopsis]
additionalProperties: false

ReadChapter:
allOf:
Expand All @@ -47,6 +45,7 @@ definitions:
additionalProperties: false

SupplementalChapter:
unevaluatedProperties: false
allOf:
- $ref: '#/definitions/ChapterRequest'
- type: object
Expand All @@ -57,7 +56,6 @@ definitions:
default: [200, 201]
items:
type: integer
additionalProperties: false

ChapterRequest:
type: object
Expand All @@ -79,7 +77,6 @@ definitions:
output:
$ref: '#/definitions/Output'
required: [path, method]
additionalProperties: false

Output:
description: |
Expand Down
4 changes: 2 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,10 @@
"lint--fix": "eslint . --fix --report-unused-disable-directives",
"merge": "ts-node tools/src/merger/merge.ts",
"test": "npm run test:unit && npm run test:integ",
"jest": "jest",
"test:unit": "jest --testMatch='**/*.test.ts' --testPathIgnorePatterns=/integ/",
"test:integ": "jest --testMatch='**/integ/*.test.ts' --runInBand",
"test:spec": "ts-node tools/src/tester/test.ts"
"test:spec": "ts-node tools/src/tester/test.ts",
"test:spec--insecure": "ts-node tools/src/tester/test.ts --opensearch-insecure"
},
"dependencies": {
"@apidevtools/swagger-parser": "^10.1.0",
Expand Down
1 change: 0 additions & 1 deletion tests/_core/bulk.yaml
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
$schema: ../../json_schemas/test_story.schema.yaml

skip: false
description: Test bulk endpoint.
epilogues:
- path: /books,movies
Expand Down
4 changes: 1 addition & 3 deletions tests/_core/search.yaml
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
$schema: ../../json_schemas/test_story.schema.yaml

skip: false
description: Test search endpoint.
prologues:
- path: /movies/_doc
Expand All @@ -12,8 +11,7 @@ prologues:
director: Bennett Miller
title: Moneyball
year: 2011
response:
status: 201
status: [201]
Copy link
Member

Choose a reason for hiding this comment

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

Why would this return an array?

Copy link
Collaborator Author

@nhtruong nhtruong Jun 26, 2024

Choose a reason for hiding this comment

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

In prologues and epilogues, we don't evaluate any spec but rather simply run the HTTP requests to setup and tear down. By default, the framework accepts status: [200, 201] as okay and other codes will make the prologue or epilogue return an error. So in this case in particular, we could have omitted status. Specifying status is usually for when cleanup where we want to delete a resource created in one of the chapters but we want to account for 404 where the resource was not created successfully. In this case, it's usually status: [200, 404]

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also added unit tests for StoryValidator

epilogues:
- path: /movies
method: DELETE
Expand Down
1 change: 0 additions & 1 deletion tests/cat/index.yaml
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
$schema: ../../json_schemas/test_story.schema.yaml

skip: false
description: Test cat endpoints.
chapters:
- synopsis: Cat with a json response.
Expand Down
2 changes: 1 addition & 1 deletion tools/src/linter/components/NamespaceFile.ts
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ export default class NamespaceFile extends FileValidator {
const current_keys = _.keys(p)
sort_by_keys(p as Record<string, any>, HTTP_METHODS)
const sorted_keys = _.keys(p)
if(!_.isEqual(current_keys, sorted_keys)) {
if (!_.isEqual(current_keys, sorted_keys)) {
return this.error(
`Operations must be sorted. Expected ${_.join(sorted_keys, ', ')}.`,
path)
Expand Down
2 changes: 1 addition & 1 deletion tools/src/linter/components/SupersededOperationsFile.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ export default class SupersededOperationsFile extends FileValidator {
return _.entries(this.superseded_ops()).map(([path, p]) => {
const current_keys = p.operations
const sorted_keys = sort_array_by_keys(p.operations as string[], HTTP_METHODS)
if(!_.isEqual(current_keys, sorted_keys)) {
if (!_.isEqual(current_keys, sorted_keys)) {
return this.error(
`Operations must be sorted. Expected ${_.join(sorted_keys, ', ')}.`,
path)
Expand Down
2 changes: 1 addition & 1 deletion tools/src/tester/ChapterEvaluator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ export default class ChapterEvaluator {
if (expected_payload == null) return { result: Result.PASSED }
const delta = atomizeChangeset(diff(expected_payload, response.payload))
const messages: string[] = _.compact(delta.map((value, _index, _array) => {
switch(value.type) {
switch (value.type) {
case Operation.UPDATE:
return `expected ${value.path.replace('$.', '')}='${value.oldValue}', got '${value.value}'`
case Operation.REMOVE:
Expand Down
2 changes: 1 addition & 1 deletion tools/src/tester/ChapterReader.ts
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ export default class ChapterReader {

#serialize_payload(payload: any, content_type: string): any {
if (payload === undefined) return undefined
switch(content_type) {
switch (content_type) {
case 'application/x-ndjson': return to_ndjson(payload as any[])
default: return payload
}
Expand Down
8 changes: 5 additions & 3 deletions tools/src/tester/ResultLogger.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,15 +29,17 @@ export class ConsoleResultLogger implements ResultLogger {
}

log (evaluation: StoryEvaluation): void {
const with_padding = [Result.FAILED, Result.ERROR].includes(evaluation.result) || this._verbose
if (with_padding) console.log()
this.#log_story(evaluation)
this.#log_chapters(evaluation.prologues ?? [], 'PROLOGUES')
this.#log_chapters(evaluation.chapters ?? [], 'CHAPTERS')
this.#log_chapters(evaluation.epilogues ?? [], 'EPILOGUES')
console.log('\n')
if (with_padding) console.log()
}

#log_story ({ result, full_path, description, display_path, message }: StoryEvaluation): void {
this.#log_evaluation({ result, message: message ?? full_path }, ansi.cyan(ansi.b(description ?? display_path)))
#log_story ({ result, full_path, display_path, message }: StoryEvaluation): void {
this.#log_evaluation({ result, message: message ?? full_path }, ansi.cyan(ansi.b(display_path)))
}

#log_chapters (evaluations: ChapterEvaluation[], title: string): void {
Expand Down
2 changes: 1 addition & 1 deletion tools/src/tester/SchemaValidator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ export default class SchemaValidator {
validate (schema: OpenAPIV3.SchemaObject, data: any): Evaluation {
const validate = this.ajv.compile(schema)
const valid = validate(data)
if (! valid) {
if (!valid) {
Copy link
Member

Choose a reason for hiding this comment

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

Missing lint?

Copy link
Collaborator Author

@nhtruong nhtruong Jun 26, 2024

Choose a reason for hiding this comment

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

Same issue with the previous PR: the rule was moved into a plugin.
I've resolved it.

this.logger.info(`# ${to_json(schema)}`)
this.logger.info(`* ${to_json(data)}`)
this.logger.info(`& ${to_json(validate.errors)}`)
Expand Down
29 changes: 12 additions & 17 deletions tools/src/tester/StoryEvaluator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,19 +8,13 @@
*/

import { ChapterRequest, Parameter, type Chapter, type Story, type SupplementalChapter } from './types/story.types'
import { type ChapterEvaluation, Result, type StoryEvaluation, OutputReference } from './types/eval.types'
import { type StoryFile, type ChapterEvaluation, Result, type StoryEvaluation, OutputReference } from './types/eval.types'
import type ChapterEvaluator from './ChapterEvaluator'
import { overall_result } from './helpers'
import { StoryOutputs } from './StoryOutputs'
import SupplementalChapterEvaluator from './SupplementalChapterEvaluator'
import { ChapterOutput } from './ChapterOutput'

export interface StoryFile {
display_path: string
full_path: string
story: Story
}

export default class StoryEvaluator {
private readonly _chapter_evaluator: ChapterEvaluator
private readonly _supplemental_chapter_evaluator: SupplementalChapterEvaluator
Expand Down Expand Up @@ -87,16 +81,17 @@ export default class StoryEvaluator {
return { evaluations, has_errors }
}

// TODO: Refactor and move this logic into StoryValidator
static check_story_variables(story: Story, display_path: string, full_path: string): StoryEvaluation | undefined {
const story_outputs = new StoryOutputs()
const prologues = (story.prologues ?? []).map((prologue) => {
return StoryEvaluator.#check_episode_variables(prologue, story_outputs)
return StoryEvaluator.#check_chapter_variables(prologue, story_outputs)
})
const chapters = (story.chapters ?? []).map((chapter) => {
return StoryEvaluator.#check_episode_variables(chapter, story_outputs)
return StoryEvaluator.#check_chapter_variables(chapter, story_outputs)
})
const epilogues = (story.epilogues ?? []).map((epilogue) => {
return StoryEvaluator.#check_episode_variables(epilogue, story_outputs)
return StoryEvaluator.#check_chapter_variables(epilogue, story_outputs)
})
const evals = prologues.concat(chapters).concat(epilogues)
if (overall_result(evals.map(e => e.overall)) === Result.FAILED) {
Expand All @@ -113,17 +108,17 @@ export default class StoryEvaluator {
}
}

static #check_episode_variables(episode: ChapterRequest, story_outputs: StoryOutputs): ChapterEvaluation {
const title = `${episode.method} ${episode.path}`
const error = StoryEvaluator.#check_used_variables(episode, story_outputs)
static #check_chapter_variables(chapter: ChapterRequest, story_outputs: StoryOutputs): ChapterEvaluation {
const title = `${chapter.method} ${chapter.path}`
const error = StoryEvaluator.#check_used_variables(chapter, story_outputs)
if (error !== undefined) {
return error
}
if (episode.id === undefined && episode.output !== undefined) {
return this.#failed_evaluation(title, 'An episode must have an id to store its output')
if (chapter.id === undefined && chapter.output !== undefined) {
return this.#failed_evaluation(title, 'A chapter must have an id to store its output')
}
if (episode.id !== undefined && episode.output !== undefined) {
story_outputs.set_chapter_output(episode.id, new ChapterOutput(episode.output))
if (chapter.id !== undefined && chapter.output !== undefined) {
story_outputs.set_chapter_output(chapter.id, new ChapterOutput(chapter.output))
}
return { title, overall: { result: Result.PASSED } }
}
Expand Down
55 changes: 55 additions & 0 deletions tools/src/tester/StoryValidator.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
/*
* Copyright OpenSearch Contributors
* SPDX-License-Identifier: Apache-2.0
*
* The OpenSearch Contributors require contributions made to
* this file be licensed under the Apache-2.0 license or a
* compatible open source license.
*/

import { Result, StoryEvaluation, StoryFile } from "./types/eval.types";
import * as path from "path";
import { Ajv2019, ValidateFunction } from 'ajv/dist/2019'
import addFormats from 'ajv-formats'
import { read_yaml } from "../helpers";

export default class StoryValidator {
private static readonly SCHEMA_FILE = path.resolve("./json_schemas/test_story.schema.yaml")
private readonly ajv: Ajv2019
private readonly validate_schema: ValidateFunction

constructor() {
this.ajv = new Ajv2019({ allErrors: true, strict: false })
Copy link
Member

Choose a reason for hiding this comment

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

We use plain ajv elsewhere, why is this using a different one?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's because of the use of unevaluatedProperties, which is a new feature in JSON Schema draft 2019-09. We use this in test_story.schema to enable additionalProperties: false for allOf objects. This was how I caught this infraction.

Copy link
Member

Choose a reason for hiding this comment

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

Should we upgrade all schema checks?

addFormats(this.ajv)
const schema = read_yaml(StoryValidator.SCHEMA_FILE)
this.validate_schema = this.ajv.compile(schema)
}

validate(story_file: StoryFile): StoryEvaluation | undefined {
const schema_file_error = this.#validate_schema_path(story_file)
if (schema_file_error != null) return schema_file_error
const schema_error = this.#validate_schema(story_file)
if (schema_error != null) return schema_error
}

#validate_schema_path(story_file: StoryFile): StoryEvaluation | undefined {
const actual_path = story_file.story.$schema
const expected_path = path.relative(path.dirname(story_file.full_path), StoryValidator.SCHEMA_FILE)
if (actual_path !== expected_path) return this.#invalid(story_file, `Expected $schema to be ${expected_path}`)
}

#validate_schema(story_file: StoryFile): StoryEvaluation | undefined {
const valid = this.validate_schema(story_file.story)
if (!valid) return this.#invalid(story_file, this.ajv.errorsText(this.validate_schema.errors))
}

#invalid({ story, display_path, full_path }: StoryFile, message: string): StoryEvaluation {
return {
display_path,
full_path,
description: story.description,
result: Result.ERROR,
message: `Invalid Story: ${message}`,
}
}
}
9 changes: 6 additions & 3 deletions tools/src/tester/TestRunner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,19 +8,22 @@
*/

import type StoryEvaluator from './StoryEvaluator'
import { type StoryFile } from './StoryEvaluator'
import { type StoryFile } from './types/eval.types'
import fs from 'fs'
import { type Story } from './types/story.types'
import { read_yaml } from '../helpers'
import { Result, type StoryEvaluation } from './types/eval.types'
import { type ResultLogger } from './ResultLogger'
import { basename, resolve } from 'path'
import type StoryValidator from "./StoryValidator";

export default class TestRunner {
private readonly _story_validator: StoryValidator
private readonly _story_evaluator: StoryEvaluator
private readonly _result_logger: ResultLogger

constructor (story_evaluator: StoryEvaluator, result_logger: ResultLogger) {
constructor (story_validator: StoryValidator, story_evaluator: StoryEvaluator, result_logger: ResultLogger) {
this._story_validator = story_validator
this._story_evaluator = story_evaluator
this._result_logger = result_logger
}
Expand All @@ -30,7 +33,7 @@ export default class TestRunner {
const story_files = this.#sort_story_files(this.#collect_story_files(resolve(story_path), '', ''))
const evaluations: StoryEvaluation[] = []
for (const story_file of story_files) {
const evaluation = await this._story_evaluator.evaluate(story_file, dry_run)
const evaluation = this._story_validator.validate(story_file) ?? await this._story_evaluator.evaluate(story_file, dry_run)
evaluations.push(evaluation)
this._result_logger.log(evaluation)
if ([Result.ERROR, Result.FAILED].includes(evaluation.result)) failed = true
Expand Down
4 changes: 3 additions & 1 deletion tools/src/tester/test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import { ConsoleResultLogger } from './ResultLogger'
import * as process from 'node:process'
import SupplementalChapterEvaluator from './SupplementalChapterEvaluator'
import MergedOpenApiSpec from './MergedOpenApiSpec'
import StoryValidator from "./StoryValidator";

const command = new Command()
.description('Run test stories against the OpenSearch spec.')
Expand Down Expand Up @@ -54,9 +55,10 @@ const http_client = new OpenSearchHttpClient(get_opensearch_opts_from_cli(opts))
const chapter_reader = new ChapterReader(http_client, logger)
const chapter_evaluator = new ChapterEvaluator(new OperationLocator(spec), chapter_reader, new SchemaValidator(spec, logger))
const supplemental_chapter_evaluator = new SupplementalChapterEvaluator(chapter_reader)
const story_validator = new StoryValidator()
const story_evaluator = new StoryEvaluator(chapter_evaluator, supplemental_chapter_evaluator)
const result_logger = new ConsoleResultLogger(opts.tabWidth, opts.verbose)
const runner = new TestRunner(story_evaluator, result_logger)
const runner = new TestRunner(story_validator, story_evaluator, result_logger)

runner.run(opts.testsPath, opts.dryRun)
.then(
Expand Down
9 changes: 7 additions & 2 deletions tools/src/tester/types/eval.types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,16 +9,21 @@

import { type ChapterOutput } from '../ChapterOutput'
import { StoryOutputs } from '../StoryOutputs'
import type { Story } from "./story.types";

export type LibraryEvaluation = StoryEvaluation[]
export interface StoryFile {
display_path: string
full_path: string
story: Story
}

export interface StoryEvaluation {
result: Result
display_path: string
full_path: string
description: string
message?: string
chapters: ChapterEvaluation[]
chapters?: ChapterEvaluation[]
epilogues?: ChapterEvaluation[]
prologues?: ChapterEvaluation[]
}
Expand Down
5 changes: 1 addition & 4 deletions tools/src/tester/types/story.types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -57,13 +57,10 @@ export type ReadChapter = Chapter & {
};

export interface Story {
$schema?: string;
$schema: string;
description: string;
prologues?: SupplementalChapter[];
epilogues?: SupplementalChapter[];
/**
* @minItems 1
*/
chapters: Chapter[];
}
/**
Expand Down
Loading
Loading