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

Support outputs on tests #324

Merged
merged 20 commits into from
Jun 11, 2024

Conversation

miguel-vila
Copy link
Contributor

Adds a way to describe outputs in chapters, that can be used for later requests in the params or in the request bodies.

Added some unit tests and an integration test that uses this feature.

Signed-off-by: miguel-vila <miguelvilag@gmail.com>
Signed-off-by: miguel-vila <miguelvilag@gmail.com>
Signed-off-by: miguel-vila <miguelvilag@gmail.com>
Signed-off-by: miguel-vila <miguelvilag@gmail.com>
Signed-off-by: miguel-vila <miguelvilag@gmail.com>
Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

This looks good! Rebase, add some to the documentation (DEVELOPER_GUIDE on writing tests probably).

I suggest wrapping up the chapter output into a class and refactoring all the helper methods as class methods, it will make things a lot cleaner/better organized.

if (evaluations.some(e => e.result === Result.ERROR)) return Result.ERROR
if (evaluations.some(e => e.result === Result.FAILED)) return Result.FAILED
if (evaluations.some(e => e.result === Result.SKIPPED)) return Result.SKIPPED
return Result.PASSED
}

export function extract_output_values(response: ActualResponse, chapterOutput?: Output): EvaluationWithOutput {
Copy link
Member

Choose a reason for hiding this comment

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

Refactor into ChapterOutput, I think it should be constructed from ActualResponse, be a method on ActualResponse, or something like that.

@@ -1,5 +1,7 @@
/* eslint-disable */

import { ChapterOutput } from "./eval.types";

Copy link
Collaborator

Choose a reason for hiding this comment

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

This file is not supposed to be edited by hand. It's generated from running npx ts-node tools/src/tester/_generate_story_types.ts.

The goal is maintaining parity between the JSON Schemas defined in test_story.schema.yaml and the TS types defined here.

@nhtruong
Copy link
Collaborator

nhtruong commented Jun 7, 2024

The test framework actually went through a refactor after you pulled from main to code this. Would you mind rebasing? Sorry about that.

Signed-off-by: miguel-vila <miguelvilag@gmail.com>
Signed-off-by: miguel-vila <miguelvilag@gmail.com>
Signed-off-by: miguel-vila <miguelvilag@gmail.com>
Signed-off-by: miguel-vila <miguelvilag@gmail.com>
Signed-off-by: miguel-vila <miguelvilag@gmail.com>
import { Agent } from 'https'
import { StoryOutputs } from './types/eval.types'
import { string } from 'yaml/dist/schema/common/string'
import { resolve_params, resolve_value } from './helpers'
Copy link
Collaborator

Choose a reason for hiding this comment

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

If resolve_params and resolve_value are only meant to be used in this class, shouldn't they be defined here instead?

if (evaluations.some(e => e.result === Result.ERROR)) return Result.ERROR
if (evaluations.some(e => e.result === Result.FAILED)) return Result.FAILED
if (evaluations.some(e => e.result === Result.SKIPPED)) return Result.SKIPPED
return Result.PASSED
}

export function extract_output_values(response: ActualResponse, chapterOutput?: Output): EvaluationWithOutput | undefined {
Copy link
Collaborator

Choose a reason for hiding this comment

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

chapterOutput -> chapter_output

}
}
return evaluations
}

async evaluate_supplemental_chapter(chapter: SupplementalChapter, story_outputs: StoryOutputs): Promise<ChapterEvaluation> {
Copy link
Collaborator

@nhtruong nhtruong Jun 10, 2024

Choose a reason for hiding this comment

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

Having both evaluate_supplemental_chapters and #evaluate_supplemental_chapters is confusing especially when they call each other. I think this calls for a SupplementalChapterEvaluation class similar to ChapterEvaluation now. They will most likely have a lot in common and we can DRY that as well.

import ChapterEvaluator from './ChapterEvaluator'
import type ChapterReader from './ChapterReader'
import SharedResources from './SharedResources'
import { overall_result } from './helpers'
import { check_story_variables, extract_output_values, overall_result } from './helpers'
Copy link
Collaborator

Choose a reason for hiding this comment

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

If check_story_variables is only used here, it should be defined here instead.

chapters: [],
message: variables_error
}
}
Copy link
Collaborator

@nhtruong nhtruong Jun 10, 2024

Choose a reason for hiding this comment

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

These errors should be assigned to the chapters that throw them so the viewer can pinpoint where to fix.

@@ -41,7 +41,7 @@ export default class ResultsDisplayer {

#display_story (): void {
const result = this.evaluation.result
const message = this.evaluation.full_path
const message = this.evaluation.message || this.evaluation.full_path
Copy link
Collaborator

Choose a reason for hiding this comment

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

With this we wont need this change anymore.

Copy link

codecov bot commented Jun 11, 2024

Codecov Report

Attention: Patch coverage is 59.51220% with 83 lines in your changes missing coverage. Please review.

Project coverage is 63.22%. Comparing base (ba3f5b2) to head (1ca41b6).

Current head 1ca41b6 differs from pull request most recent head b8ccfca

Please upload reports for the commit b8ccfca to get more accurate results.

Files Patch % Lines
tools/src/tester/SupplementalChapterEvaluator.ts 0.00% 30 Missing ⚠️
tools/src/tester/StoryEvaluator.ts 60.27% 29 Missing ⚠️
tools/src/tester/ChapterEvaluator.ts 0.00% 7 Missing ⚠️
tools/src/tester/types/eval.types.ts 57.14% 6 Missing ⚠️
tools/src/tester/ChapterReader.ts 0.00% 4 Missing ⚠️
tools/src/tester/helpers.ts 85.00% 3 Missing ⚠️
tools/src/tester/start.ts 0.00% 3 Missing ⚠️
tools/src/tester/ResultLogger.ts 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main     #324       +/-   ##
===========================================
- Coverage   78.88%   63.22%   -15.67%     
===========================================
  Files          42       45        +3     
  Lines        1184     1365      +181     
  Branches      261      316       +55     
===========================================
- Hits          934      863       -71     
- Misses        250      502      +252     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: miguel-vila <miguelvilag@gmail.com>
Signed-off-by: miguel-vila <miguelvilag@gmail.com>
Copy link
Contributor

github-actions bot commented Jun 11, 2024

Changes Analysis

Commit SHA: b8ccfca
Comparing To SHA: ba3f5b2

API Changes

Summary


├─┬Paths
│ ├──[➕] path (3659:3)
│ ├──[➕] path (3751:3)
│ ├──[➕] path (3675:3)
│ ├──[➕] path (3704:3)
│ ├──[➕] path (3736:3)
│ └──[➕] path (3720:3)
└─┬Components
  ├──[➕] requestBodies (22315:7)
  ├──[➕] requestBodies (22258:7)
  ├──[➕] requestBodies (22286:7)
  ├──[➕] responses (24467:7)
  ├──[➕] responses (24452:7)
  ├──[➕] responses (24485:7)
  ├──[➕] responses (24398:7)
  ├──[➕] responses (24408:7)
  ├──[➕] responses (24431:7)
  ├──[➕] responses (24403:7)
  ├──[➕] parameters (18613:7)
  ├──[➕] parameters (18619:7)
  ├──[➕] parameters (18607:7)
  ├──[➕] parameters (18601:7)
  ├──[➕] schemas (44160:7)
  ├──[➕] schemas (44172:7)
  └──[➕] schemas (44184:7)

Document Element Total Changes Breaking Changes
paths 6 0
components 17 0
  • Total Changes: 23
  • Additions: 23

Report

The full API changes report is available at: https://github.com/opensearch-project/opensearch-api-specification/actions/runs/9467793824/artifacts/1590084965

API Coverage

Before After Δ
Covered (%) 476 (46.62 %) 483 (47.31 %) 7 (0.69 %)
Uncovered (%) 545 (53.38 %) 538 (52.69 %) -7 (-0.69 %)
Unknown 24 24 0

Signed-off-by: miguel-vila <miguelvilag@gmail.com>
Signed-off-by: miguel-vila <miguelvilag@gmail.com>
Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

@miguel-vila Let's get this merged? Iterate to green and say when you think you're done with the changes?

I don't love it that we're adding a ton of things to helpers, that's an OO anti-pattern. Consider refactoring those. Can also be refactired later.

* @param output
* @returns
*/
static create_dummy_from_output (output: Output): ChapterOutput {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: this seems unnecessary, do new ChapterOutput where it's called?

const title = `${chapter.method} ${chapter.path}`
const response = await this._chapter_reader.read(chapter, story_outputs)
const status = chapter.status ?? [200, 201]
const output_values = extract_output_values(response)
Copy link
Member

Choose a reason for hiding this comment

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

This should read response.output_values, it's very business-logic specific to the reponse.

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 params = this.#evaluate_parameters(chapter, operation)
const request_body = this.#evaluate_request_body(chapter, operation)
const status = this.#evaluate_status(chapter, response)
const payload = status.result === Result.PASSED ? this.#evaluate_payload(response, operation) : { result: Result.SKIPPED }
const output_values = extract_output_values(response, chapter.output)
Copy link
Member

Choose a reason for hiding this comment

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

extract_output_values should either be a private method on ChapterEvaluator or maybe this works better as chapter.output.extract_output_values_from_response(response)?

Signed-off-by: miguel-vila <miguelvilag@gmail.com>
Signed-off-by: miguel-vila <miguelvilag@gmail.com>
Signed-off-by: miguel-vila <miguelvilag@gmail.com>
@@ -78,15 +77,15 @@ test('extract_output_values', () => {
d: 'payload.a.arr[0].d',
e: 'payload.a.arr[1].e'
}
expect(extract_output_values(response, output1)).toEqual(passed_output({
expect(ChapterOutput.extract_output_values(response, output1)).toEqual(passed_output({
Copy link
Member

Choose a reason for hiding this comment

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

A ChapterOutput is something that is the result of an Output (a schema) and a response (values). I think the OO way should be closer to this:

new ChapterOutput(output, values)

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

@nhtruong take a look, I am good without all the refactors, can be done later

@nhtruong nhtruong merged commit 53bdbd9 into opensearch-project:main Jun 11, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants