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

Add support for multiple test verbs and fix /_render/template. #723

Closed
wants to merge 4 commits into from
Closed
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
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
- Added schema for `/_plugins/_knn/stats`, `/_plugins/_knn/models/{model_id}`, `_train` and `_search` ([#704](https://github.com/opensearch-project/opensearch-api-specification/pull/704))
- Added `retry` support in `prologues` and `epilogues` ([#713](https://github.com/opensearch-project/opensearch-api-specification/pull/713))
- Added response schema for `DELETE /_plugins/_rollup/jobs/{id}`, `POST /_plugins/_rollup/jobs/{id}/_start` and `_stop` ([#716](https://github.com/opensearch-project/opensearch-api-specification/pull/716))
- Added support for multiple test verbs ([#723](https://github.com/opensearch-project/opensearch-api-specification/pull/723))

### 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))
Expand Down Expand Up @@ -62,6 +63,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
- Fixed content-type of `GET /_plugins/_observability/_local/stats` ([#711](https://github.com/opensearch-project/opensearch-api-specification/pull/711))
- Fixed `tenant` in `ObservabilityObject` request body to not be required ([#711](https://github.com/opensearch-project/opensearch-api-specification/pull/711))
- Fixed response code in `PUT /_plugins/_rollup/jobs/{id}` ([#716](https://github.com/opensearch-project/opensearch-api-specification/pull/716))
- Fixed response schema for `/_render/template` and `/_render/template/{id}` ([#723](https://github.com/opensearch-project/opensearch-api-specification/pull/723))

### Changed
- Changed `tasks._common:TaskInfo` and `tasks._common:TaskGroup` to be composed of a `tasks._common:TaskInfoBase` ([#683](https://github.com/opensearch-project/opensearch-api-specification/pull/683))
Expand Down
13 changes: 13 additions & 0 deletions TESTING_GUIDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
- [FAILED Cat with a json response (from security-analytics).](#failed--cat-with-a-json-response-from-security-analytics)
- [Writing Spec Tests](#writing-spec-tests)
- [Simple Test Story](#simple-test-story)
- [Testing Multiple Verbs](#testing-multiple-verbs)
- [Using Output from Previous Chapters](#using-output-from-previous-chapters)
- [Managing Versions](#managing-versions)
- [Managing Distributions](#managing-distributions)
Expand Down Expand Up @@ -182,6 +183,18 @@ chapters:
index: books
```

### Testing Multiple Verbs

Some APIs allow multiple verbs for the same effect. Specify multiple verbs as follows and the test tool will execute both.

```yaml
- synopsis: Use POST and PUT interchangeably.
path: /{index}
method:
- POST
- PUT
```

Copy link
Collaborator

@nhtruong nhtruong Dec 10, 2024

Choose a reason for hiding this comment

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

This is a great shorthand for writing the same chapter multiple times with different methods. I think we should treat it as such: Multiple chapters that virtually identical but the method. And then when the story is read, we will turn that 1 chapter of Create index "books" into two chapter:

  • Create Index "book" [With POST]
  • Create Index "book" [With PUT]

Then you will only have to modify the story parser to turn that 1 chapter into 2 and the rest of the framework can stay intact. We can add a line in this guide saying that the chapter will be split into 2 so they know what to expect in the Result page.

Arguably, a case like this should be treat as 2 different stories because rarely are our non-PUT requests idempotent (even when you delete a resource on OS twice, you get 404 the second time). So you could not actually test this on PUT/POST /books because executing PUT /books after POST /books will result in index named books already exists error. This will open a can of worm of matrix argument when we have multiple of such chapters in an original story.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay let me see if I can do a parser-based implementation instead ...

### Using Output from Previous Chapters

Consider the following chapters in [ml/model_groups](tests/plugins/ml/ml/model_groups.yaml) test story:
Expand Down
13 changes: 10 additions & 3 deletions json_schemas/test_story.schema.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,11 @@ definitions:
items:
type: integer

HttpMethod:
type: string
# eslint-disable-next-line yml/sort-sequence-values
enum: [GET, PUT, POST, DELETE, PATCH, HEAD, OPTIONS]

ChapterRequest:
type: object
properties:
Expand All @@ -76,9 +81,11 @@ definitions:
path:
type: string
method:
type: string
# eslint-disable-next-line yml/sort-sequence-values
enum: [GET, PUT, POST, DELETE, PATCH, HEAD, OPTIONS]
oneOf:
- type: array
items:
$ref: '#/definitions/HttpMethod'
- $ref: '#/definitions/HttpMethod'
parameters:
type: object
additionalProperties:
Expand Down
7 changes: 3 additions & 4 deletions spec/namespaces/_core.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -2482,16 +2482,15 @@ components:
schema:
type: object
properties:
file:
type: string
id:
$ref: '../schemas/_common.yaml#/components/schemas/Id'
params:
description: |-
Key-value pairs used to replace Mustache variables in the template.
The key is the variable name.
The value is the variable value.
type: object
additionalProperties:
type: object
additionalProperties: true
source:
description: |-
An inline search template.
Expand Down
62 changes: 62 additions & 0 deletions tests/default/_core/render/template.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
$schema: ../../../../json_schemas/test_story.schema.yaml

description: Test rendering a search template as a search query.

prologues:
- path: /_scripts/movie_template
method: POST
request:
content_type: application/json
payload:
script:
lang: mustache
source: >
{
"query": {
"match": {
"{{field}}": "{{value}}"
}
}
}
epilogues:
- path: /_scripts/movie_template
method: DELETE
status: [200, 404]
chapters:
- synopsis: Render the movie template (request payload).
path: /_render/template
method:
- GET
- POST
request:
payload:
id: movie_template
params:
field: director
value: Quentin Tarantino
response:
status: 200
payload:
template_output:
query:
match:
director: Quentin Tarantino
- synopsis: Render the movie template (path).
path: /_render/template/{id}
method:
- GET
- POST
parameters:
id: movie_template
request:
payload:
params:
field: director
value: Christopher Nolan
response:
status: 200
payload:
template_output:
query:
match:
director: Christopher Nolan
3 changes: 3 additions & 0 deletions tests/default/search/template.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@ epilogues:
- path: /movies
method: DELETE
status: [200, 404]
- path: /movies
method: DELETE
status: [200, 404]
prologues:
- path: /_bulk
method: POST
Expand Down
16 changes: 8 additions & 8 deletions tools/src/tester/ChapterEvaluator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,19 +35,19 @@ export default class ChapterEvaluator {
this.logger = logger
}

async evaluate(chapter: Chapter, skip: boolean, story_outputs: StoryOutputs): Promise<ChapterEvaluation> {
async evaluate(chapter: Chapter, method: string, skip: boolean, story_outputs: StoryOutputs): Promise<ChapterEvaluation> {
if (skip) return { title: chapter.synopsis, overall: { result: Result.SKIPPED } }

const operation = this._operation_locator.locate_operation(chapter)
if (operation == null) return { title: chapter.synopsis, overall: { result: Result.FAILED, message: `Operation "${chapter.method.toUpperCase()} ${chapter.path}" not found in the spec.` } }
const operation = this._operation_locator.locate_operation(chapter, method)
if (operation == null) return { title: chapter.synopsis, overall: { result: Result.FAILED, message: `Operation "${method.toUpperCase()} ${chapter.path}" not found in the spec.` } }

var tries = chapter.retry && chapter.retry?.count > 0 ? chapter.retry.count + 1 : 1
var retry = 0

var result: ChapterEvaluation

do {
result = await this.#evaluate(chapter, operation, story_outputs, ++retry > 1 ? retry - 1 : undefined)
result = await this.#evaluate(chapter, method, operation, story_outputs, ++retry > 1 ? retry - 1 : undefined)

if (result.overall.result === Result.PASSED || result.overall.result === Result.SKIPPED) {
return result
Expand All @@ -61,8 +61,8 @@ export default class ChapterEvaluator {
return result
}

async #evaluate(chapter: Chapter, operation: ParsedOperation, story_outputs: StoryOutputs, retries?: number): Promise<ChapterEvaluation> {
const response = await this._chapter_reader.read(chapter, story_outputs)
async #evaluate(chapter: Chapter, method: string, operation: ParsedOperation, story_outputs: StoryOutputs, retries?: number): Promise<ChapterEvaluation> {
const response = await this._chapter_reader.read(chapter, method, story_outputs)
const params = this.#evaluate_parameters(chapter, operation, story_outputs)
const request = this.#evaluate_request(chapter, operation, story_outputs)
const status = this.#evaluate_status(chapter, response)
Expand All @@ -85,10 +85,10 @@ export default class ChapterEvaluator {
var result: ChapterEvaluation = {
title: chapter.synopsis,
operation: {
method: chapter.method,
method,
path: chapter.path
},
path: `${chapter.method} ${chapter.path}`,
path: `${method} ${chapter.path}`,
overall: { result: overall_result(evaluations) },
request: { parameters: params, request },
response: {
Expand Down
6 changes: 3 additions & 3 deletions tools/src/tester/ChapterReader.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ export default class ChapterReader {
this.logger = logger
}

async read (chapter: ChapterRequest, story_outputs: StoryOutputs): Promise<ActualResponse> {
async read (chapter: ChapterRequest, method: string, story_outputs: StoryOutputs): Promise<ActualResponse> {
const response: Record<string, any> = {}
const resolved_params = story_outputs.resolve_params(chapter.parameters ?? {})
const [url_path, params] = this.#parse_url(chapter.path, resolved_params)
Expand All @@ -37,10 +37,10 @@ export default class ChapterReader {
story_outputs.resolve_value(chapter.request.payload),
content_type
) : undefined
this.logger.info(`=> ${chapter.method} ${url_path} (${to_json(params)}) [${content_type}] ${_.compact([to_json(headers), to_json(request_data)]).join(' | ')}`)
this.logger.info(`=> ${method} ${url_path} (${to_json(params)}) [${content_type}] ${_.compact([to_json(headers), to_json(request_data)]).join(' | ')}`)
await this._client.request({
url: url_path,
method: chapter.method,
method,
headers: { 'Content-Type' : content_type, ...headers },
params,
data: request_data,
Expand Down
6 changes: 3 additions & 3 deletions tools/src/tester/OperationLocator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,12 @@ export default class OperationLocator {
this.spec = spec
}

locate_operation (chapter: Chapter): ParsedOperation | undefined {
locate_operation (chapter: Chapter, method: string): ParsedOperation | undefined {
const path = chapter.path
const method = chapter.method.toLowerCase() as OpenAPIV3.HttpMethods
const request_method = method.toLowerCase() as OpenAPIV3.HttpMethods
const cache_key = path + method
if (this.cached_operations[cache_key] != null) return this.cached_operations[cache_key]
const operation = this.spec.paths[path]?.[method]
const operation = this.spec.paths[path]?.[request_method]
if (operation == null) return undefined
this.#deref(operation)
const parameters = _.keyBy(operation.parameters ?? [], 'name')
Expand Down
68 changes: 39 additions & 29 deletions tools/src/tester/StoryEvaluator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -108,25 +108,27 @@ export default class StoryEvaluator {
async #evaluate_chapters(chapters: Chapter[], has_errors: boolean, dry_run: boolean, story_outputs: StoryOutputs, version?: string, distribution?: string): Promise<ChapterEvaluation[]> {
const evaluations: ChapterEvaluation[] = []
for (const chapter of chapters) {
if (dry_run) {
const title = chapter.synopsis || `${chapter.method} ${chapter.path}`
evaluations.push({ title, overall: { result: Result.SKIPPED, message: 'Dry Run' } })
} else if (distribution != undefined && chapter.distributions?.included !== undefined && chapter.distributions?.included.length > 0 && !chapter.distributions.included.includes(distribution)) {
const title = chapter.synopsis || `${chapter.method} ${chapter.path}`
evaluations.push({ title, overall: { result: Result.SKIPPED, message: `Skipped because distribution ${distribution} is not ${chapter.distributions.included.length > 1 ? 'one of ' : ''}${chapter.distributions.included.join(', ')}.` } })
} else if (distribution != undefined && chapter.distributions?.excluded !== undefined && chapter.distributions?.excluded.length > 0 && chapter.distributions.excluded.includes(distribution)) {
const title = chapter.synopsis || `${chapter.method} ${chapter.path}`
evaluations.push({ title, overall: { result: Result.SKIPPED, message: `Skipped because distribution ${distribution} is ${chapter.distributions.excluded.length > 1 ? 'one of ' : ''}${chapter.distributions.excluded.join(', ')}.` } })
} else if (version != undefined && chapter.version !== undefined && !semver.satisfies(version, chapter.version)) {
const title = chapter.synopsis || `${chapter.method} ${chapter.path}`
evaluations.push({ title, overall: { result: Result.SKIPPED, message: `Skipped because version ${version} does not satisfy ${chapter.version}.` } })
} else {
const evaluation = await this._chapter_evaluator.evaluate(chapter, has_errors, story_outputs)
has_errors = has_errors || evaluation.overall.result === Result.ERROR
if (evaluation.output !== undefined && chapter.id !== undefined) {
story_outputs.set_chapter_output(chapter.id, evaluation.output)
for (const method of StoryEvaluator.#chapter_methods(chapter.method)) {
if (dry_run) {
const title = chapter.synopsis || `${method} ${chapter.path}`
evaluations.push({ title, overall: { result: Result.SKIPPED, message: 'Dry Run' } })
} else if (distribution != undefined && chapter.distributions?.included !== undefined && chapter.distributions?.included.length > 0 && !chapter.distributions.included.includes(distribution)) {
const title = chapter.synopsis || `${method} ${chapter.path}`
evaluations.push({ title, overall: { result: Result.SKIPPED, message: `Skipped because distribution ${distribution} is not ${chapter.distributions.included.length > 1 ? 'one of ' : ''}${chapter.distributions.included.join(', ')}.` } })
} else if (distribution != undefined && chapter.distributions?.excluded !== undefined && chapter.distributions?.excluded.length > 0 && chapter.distributions.excluded.includes(distribution)) {
const title = chapter.synopsis || `${method} ${chapter.path}`
evaluations.push({ title, overall: { result: Result.SKIPPED, message: `Skipped because distribution ${distribution} is ${chapter.distributions.excluded.length > 1 ? 'one of ' : ''}${chapter.distributions.excluded.join(', ')}.` } })
} else if (version != undefined && chapter.version !== undefined && !semver.satisfies(version, chapter.version)) {
const title = chapter.synopsis || `${method} ${chapter.path}`
evaluations.push({ title, overall: { result: Result.SKIPPED, message: `Skipped because version ${version} does not satisfy ${chapter.version}.` } })
} else {
const evaluation = await this._chapter_evaluator.evaluate(chapter, method, has_errors, story_outputs)
has_errors = has_errors || evaluation.overall.result === Result.ERROR
if (evaluation.output !== undefined && chapter.id !== undefined) {
story_outputs.set_chapter_output(chapter.id, evaluation.output)
}
evaluations.push(evaluation)
}
evaluations.push(evaluation)
}
}
return evaluations
Expand All @@ -136,16 +138,18 @@ export default class StoryEvaluator {
let has_errors = false
const evaluations: ChapterEvaluation[] = []
for (const chapter of chapters) {
const title = `${chapter.method} ${chapter.path}`
if (dry_run) {
evaluations.push({ title, overall: { result: Result.SKIPPED, message: 'Dry Run' } })
} else {
const evaluation = await this._supplemental_chapter_evaluator.evaluate(chapter, story_outputs)
has_errors = has_errors || (evaluation.overall.result !== Result.PASSED && evaluation.overall.result !== Result.SKIPPED)
if (evaluation.output !== undefined && chapter.id !== undefined) {
story_outputs.set_chapter_output(chapter.id, evaluation.output)
for (const method of StoryEvaluator.#chapter_methods(chapter.method)) {
const title = `${method} ${chapter.path}`
if (dry_run) {
evaluations.push({ title, overall: { result: Result.SKIPPED, message: 'Dry Run' } })
} else {
const evaluation = await this._supplemental_chapter_evaluator.evaluate(chapter, method, story_outputs)
has_errors = has_errors || (evaluation.overall.result !== Result.PASSED && evaluation.overall.result !== Result.SKIPPED)
if (evaluation.output !== undefined && chapter.id !== undefined) {
story_outputs.set_chapter_output(chapter.id, evaluation.output)
}
evaluations.push(evaluation)
}
evaluations.push(evaluation)
}
}
return { evaluations, has_errors }
Expand Down Expand Up @@ -179,7 +183,8 @@ export default class StoryEvaluator {
}

static #check_chapter_variables(chapter: ChapterRequest, story_outputs: StoryOutputs): ChapterEvaluation {
const title = `${chapter.method} ${chapter.path}`
const methods = StoryEvaluator.#chapter_methods(chapter.method)
const title = `${methods.join(', ')} ${chapter.path}`
const error = StoryEvaluator.#check_used_variables(chapter, story_outputs)
if (error !== undefined) {
return error
Expand All @@ -202,8 +207,9 @@ export default class StoryEvaluator {
* @returns
*/
static #check_used_variables(chapter: ChapterRequest, story_outputs: StoryOutputs): ChapterEvaluation | undefined {
const methods = StoryEvaluator.#chapter_methods(chapter.method)
const variables = new Set<OutputReference>()
const title = `${chapter.method} ${chapter.path}`
const title = `${methods.join(', ')} ${chapter.path}`
StoryEvaluator.#extract_params_variables(chapter.parameters ?? {}, variables)
StoryEvaluator.#extract_request_variables(chapter.request?.payload ?? {}, variables)
for (const { chapter_id, output_name } of variables) {
Expand Down Expand Up @@ -258,4 +264,8 @@ export default class StoryEvaluator {
static #failed_evaluation(title: string, message: string): ChapterEvaluation {
return { title, overall: { result: Result.FAILED, message } }
}

static #chapter_methods(methods: string[] | string): string[] {
return [...(Array.isArray(methods) ? methods : [methods])]
}
}
10 changes: 5 additions & 5 deletions tools/src/tester/SupplementalChapterEvaluator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,14 +26,14 @@ export default class SupplementalChapterEvaluator {
this.logger = logger
}

async evaluate(chapter: SupplementalChapter, story_outputs: StoryOutputs): Promise<ChapterEvaluation> {
const title = `${chapter.method} ${chapter.path}`
async evaluate(chapter: SupplementalChapter, method: string, story_outputs: StoryOutputs): Promise<ChapterEvaluation> {
const title = `${method} ${chapter.path}`

let tries = chapter.retry && chapter.retry?.count > 0 ? chapter.retry.count + 1 : 1
let chapter_evaluation: EvaluationWithOutput

do {
chapter_evaluation = await this.#evaluate(chapter, story_outputs)
chapter_evaluation = await this.#evaluate(chapter, method, story_outputs)
if (chapter_evaluation.evaluation.result === Result.PASSED) break
if (--tries == 0) break
this.logger.info(`Failed, retrying, ${tries == 1 ? '1 retry left' : `${tries} retries left`} ...`)
Expand All @@ -45,8 +45,8 @@ export default class SupplementalChapterEvaluator {
return result
}

async #evaluate(chapter: SupplementalChapter, story_outputs: StoryOutputs): Promise<EvaluationWithOutput> {
const response = await this._chapter_reader.read(chapter, story_outputs)
async #evaluate(chapter: SupplementalChapter, method: string, story_outputs: StoryOutputs): Promise<EvaluationWithOutput> {
const response = await this._chapter_reader.read(chapter, method, story_outputs)
const output_values_evaluation = ChapterOutput.extract_output_values(response, chapter.output)
if (output_values_evaluation.output) this.logger.info(`$ ${to_json(output_values_evaluation.output)}`)

Expand Down
Loading
Loading