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

Fix: evaluate output values before schema checking. #536

Merged
merged 1 commit into from
Aug 28, 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
14 changes: 8 additions & 6 deletions tools/src/tester/ChapterEvaluator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -63,8 +63,8 @@ export default class ChapterEvaluator {

async #evaluate(chapter: Chapter, operation: ParsedOperation, story_outputs: StoryOutputs, retries?: number): Promise<ChapterEvaluation> {
const response = await this._chapter_reader.read(chapter, story_outputs)
const params = this.#evaluate_parameters(chapter, operation)
const request = this.#evaluate_request(chapter, operation)
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)
const payload_schema_evaluation = status.result === Result.PASSED ? this.#evaluate_payload_schema(chapter, response, operation) : { result: Result.SKIPPED }
const output_values_evaluation: EvaluationWithOutput = status.result === Result.PASSED ? ChapterOutput.extract_output_values(response, chapter.output) : { evaluation: { result: Result.SKIPPED } }
Expand Down Expand Up @@ -110,21 +110,23 @@ export default class ChapterEvaluator {
return result
}

#evaluate_parameters(chapter: Chapter, operation: ParsedOperation): Record<string, Evaluation> {
return Object.fromEntries(Object.entries(chapter.parameters ?? {}).map(([name, parameter]) => {
#evaluate_parameters(chapter: Chapter, operation: ParsedOperation, story_outputs: StoryOutputs): Record<string, Evaluation> {
const parameters: Record<string, any> = story_outputs.resolve_value(chapter.parameters) ?? {}
return Object.fromEntries(Object.entries(parameters).map(([name, parameter]) => {
const schema = operation.parameters[name]?.schema
if (schema == null) return [name, { result: Result.FAILED, message: `Schema for "${name}" parameter not found.` }]
const evaluation = this._schema_validator.validate(schema, parameter)
return [name, evaluation]
}))
}

#evaluate_request(chapter: Chapter, operation: ParsedOperation): Evaluation {
#evaluate_request(chapter: Chapter, operation: ParsedOperation, story_outputs: StoryOutputs): Evaluation {
if (chapter.request?.payload === undefined) return { result: Result.PASSED }
const content_type = chapter.request.content_type ?? APPLICATION_JSON
const schema = operation.requestBody?.content[content_type]?.schema
if (schema == null) return { result: Result.FAILED, message: `Schema for "${content_type}" request body not found in the spec.` }
return this._schema_validator.validate(schema, chapter.request?.payload ?? {})
const payload = story_outputs.resolve_value(chapter.request?.payload) ?? {}
return this._schema_validator.validate(schema, payload)
}

#evaluate_status(chapter: Chapter, response: ActualResponse): Evaluation {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
display_path: passed.yaml
full_path: tools/tests/tester/fixtures/stories/passed.yaml
display_path: passed/passed.yaml
full_path: tools/tests/tester/fixtures/stories/passed/passed.yaml

result: PASSED
description: This story should pass.
Expand Down
68 changes: 68 additions & 0 deletions tools/tests/tester/fixtures/evals/passed/value_type.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
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}
prologues:
- overall:
result: PASSED
title: GET /
output:
outputs:
boolean: false
epilogues:
- overall:
result: PASSED
title: DELETE /movies
chapters:
- title: This chapter requires a boolean in the parameters.
overall:
result: PASSED
path: GET /_cat/health
operation:
method: GET
path: /_cat/health
request:
parameters:
format:
result: PASSED
v:
result: PASSED
request:
result: PASSED
response:
status:
result: PASSED
payload_body:
result: PASSED
payload_schema:
result: PASSED
output_values:
result: SKIPPED
- title: This chapter requires a boolean in the body.
overall:
result: PASSED
path: PUT /{index}
operation:
method: PUT
path: /{index}
request:
parameters:
index:
result: PASSED
request:
result: PASSED
response:
status:
result: PASSED
payload_body:
result: PASSED
payload_schema:
result: PASSED
output_values:
result: SKIPPED
40 changes: 26 additions & 14 deletions tools/tests/tester/fixtures/specs/excerpt.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ paths:
description: Returns health for the Cat APIs.
parameters:
- $ref: '#/components/parameters/cat.health::query.format'
- $ref: '#/components/parameters/cat.health::query.v'
responses:
'200':
$ref: '#/components/responses/cat.health@200'
Expand All @@ -40,6 +41,20 @@ paths:
'200':
$ref: '#/components/responses/cat.indices@200'
/{index}:
put:
operationId: indices.create.0
x-operation-group: indices.create
x-version-added: '1.0'
description: Creates an index with optional settings and mappings.
externalDocs:
url: https://opensearch.org/docs/latest/api-reference/index-apis/create-index/
parameters:
- $ref: '#/components/parameters/indices.create::path.index'
requestBody:
$ref: '#/components/requestBodies/indices.create'
responses:
'200':
$ref: '#/components/responses/indices.create@200'
delete:
operationId: indices.delete.0
x-operation-group: indices.delete
Expand All @@ -65,20 +80,6 @@ paths:
responses:
'200':
$ref: '#/components/responses/indices.exists@200'
put:
operationId: indices.create.0
x-operation-group: indices.create
x-version-added: '1.0'
description: Creates an index with optional settings and mappings.
externalDocs:
url: https://opensearch.org/docs/latest/api-reference/index-apis/create-index/
parameters:
- $ref: '#/components/parameters/indices.create::path.index'
requestBody:
$ref: '#/components/requestBodies/indices.create'
responses:
'200':
$ref: '#/components/responses/indices.create@200'
components:
requestBodies:
indices.create:
Expand All @@ -90,6 +91,12 @@ components:
settings:
type: object
description: Optional settings for the index.
properties:
shard:
type: object
properties:
check_on_startup:
type: boolean
mappings:
type: object
description: Optional mappings for the index.
Expand Down Expand Up @@ -180,6 +187,11 @@ components:
name: format
schema:
type: string
cat.health::query.v:
in: query
name: v
schema:
type: boolean
cat.help::query.format:
in: query
name: format
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
$schema: ../../../../../json_schemas/test_story.schema.yaml
$schema: ../../../../../../json_schemas/test_story.schema.yaml

description: This story should pass.
warnings:
Expand Down
31 changes: 31 additions & 0 deletions tools/tests/tester/fixtures/stories/passed/value_type.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
$schema: ../../../../../../json_schemas/test_story.schema.yaml

description: This story has an error trying to reuse a variable of a different type.

prologues:
- method: GET
id: root
path: /
output:
boolean: payload.version.build_snapshot
epilogues:
- method: DELETE
path: /movies
status: [200, 404]
chapters:
- synopsis: This chapter requires a boolean in the parameters.
method: GET
path: /_cat/health
parameters:
format: json
v: ${root.boolean}
- synopsis: This chapter requires a boolean in the body.
method: PUT
path: /{index}
parameters:
index: movies
request:
payload:
settings:
shard:
check_on_startup: ${root.boolean}
12 changes: 9 additions & 3 deletions tools/tests/tester/integ/StoryEvaluator.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,14 @@ afterEach(() => {
})

test('passed', async () => {
const actual = await load_actual_evaluation(story_evaluator, 'passed')
const expected = load_expected_evaluation('passed')
const actual = await load_actual_evaluation(story_evaluator, 'passed/passed')
const expected = load_expected_evaluation('passed/passed')
expect(actual).toEqual(expected)
})

test('value_type.yaml', async () => {
const actual = await load_actual_evaluation(story_evaluator, 'passed/value_type')
const expected = load_expected_evaluation('passed/value_type')
expect(actual).toEqual(expected)
})

Expand Down Expand Up @@ -65,7 +71,7 @@ test('skipped/semver', async () => {

test('with an unexpected error deserializing data', async () => {
opensearch_http_client.request = jest.fn().mockRejectedValue(new Error('This was unexpected.'))
const actual = await load_actual_evaluation(story_evaluator, 'passed')
const actual = await load_actual_evaluation(story_evaluator, 'passed/passed')
expect(actual.result).toEqual(Result.ERROR)
expect(actual.chapters && actual.chapters[0]).toEqual({
title: "This PUT /{index} chapter should pass.",
Expand Down
5 changes: 3 additions & 2 deletions tools/tests/tester/integ/TestRunner.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,19 +29,20 @@ test('stories folder', async () => {
}

const expected_evaluations = [
'passed',
'error/chapter_error',
'error/output_error',
'error/prologue_error',
'failed/invalid_data',
'failed/not_found',
'passed/passed',
'passed/value_type',
'skipped/semver',
'skipped/distributions/chapters',
'skipped/distributions/excluded',
'skipped/distributions/included'
].map((fixture) => { return load_expected_evaluation(fixture, true) })

expect(actual_evaluations).toStrictEqual(expected_evaluations)
expect(actual_evaluations).toEqual(expected_evaluations)
Copy link
Member Author

Choose a reason for hiding this comment

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

This is because of this:

   @@ -959,11 +959,11 @@
              "title": "DELETE /movies",
            },
          ],
          "prologues": Array [
            Object {
    -         "output": Object {
    +         "output": ChapterOutput {
                "outputs": Object {
                  "boolean": false,
                },
              },
              "overall": Object {

})

describe('story_files', () => {
Expand Down
Loading