Support outputs on tests #324

miguel-vila · 2024-06-06T23:59:36Z

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 <[email protected]>

dblock

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.

dblock · 2024-06-07T12:08:33Z

tools/src/tester/helpers.ts

  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 {


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

nhtruong · 2024-06-07T14:46:39Z

tools/src/tester/types/story.types.ts

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

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


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 · 2024-06-07T14:58:45Z

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 <[email protected]>

nhtruong · 2024-06-10T21:25:42Z

tools/src/tester/ChapterReader.ts

 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'


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

nhtruong · 2024-06-10T21:43:01Z

tools/src/tester/helpers.ts

  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 {


chapterOutput -> chapter_output

nhtruong · 2024-06-10T21:58:09Z

tools/src/tester/StoryEvaluator.ts

      }
    }
    return evaluations
  }
+
+  async evaluate_supplemental_chapter(chapter: SupplementalChapter, story_outputs: StoryOutputs): Promise<ChapterEvaluation> {


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.

nhtruong · 2024-06-10T22:03:20Z

tools/src/tester/StoryEvaluator.ts

 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'


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

nhtruong · 2024-06-10T22:16:36Z

tools/src/tester/StoryEvaluator.ts

+        chapters: [],
+        message: variables_error
+      }
+    }


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

nhtruong · 2024-06-10T22:17:26Z

tools/src/tester/ResultsDisplayer.ts

@@ -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


With this we wont need this change anymore.

…tests

codecov · 2024-06-11T00:43:30Z

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 <[email protected]>

github-actions · 2024-06-11T01:01:56Z

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 <[email protected]>

dblock

@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.

dblock · 2024-06-11T12:09:49Z

tools/src/tester/ChapterOutput.ts

+   * @param output
+   * @returns
+   */
+  static create_dummy_from_output (output: Output): ChapterOutput {


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

dblock · 2024-06-11T12:12:50Z

tools/src/tester/SupplementalChapterEvaluator.ts

+        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)


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

dblock · 2024-06-11T12:16:11Z

tools/src/tester/ChapterEvaluator.ts

    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)


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 <[email protected]>

dblock · 2024-06-11T14:51:38Z

tools/tests/tester/start.test.ts

@@ -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({


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)

dblock

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

miguel-vila added 5 commits June 6, 2024 13:25

Support outputs in tests

0c3faf6

Signed-off-by: miguel-vila <[email protected]>

add variables checks

e392472

Signed-off-by: miguel-vila <[email protected]>

remove comment

fe0b421

Signed-off-by: miguel-vila <[email protected]>

remove index and logs

55b7163

Signed-off-by: miguel-vila <[email protected]>

refactor

8dd3d33

Signed-off-by: miguel-vila <[email protected]>

miguel-vila requested review from dblock, harshavamsi, sachetalva, nhtruong, Xtansia, VachaShah, Tokesh and aabeshov as code owners June 6, 2024 23:59

dblock reviewed Jun 7, 2024

View reviewed changes

nhtruong reviewed Jun 7, 2024

View reviewed changes

miguel-vila added 6 commits June 7, 2024 11:35

Merge tag 'main' into support-outputs-in-tests

b0340fe

fix lints

7620ad2

Signed-off-by: miguel-vila <[email protected]>

move definitions to schema file

77926e7

Signed-off-by: miguel-vila <[email protected]>

factor out in classes

8f2160a

Signed-off-by: miguel-vila <[email protected]>

finish refactor

6cf3eff

Signed-off-by: miguel-vila <[email protected]>

add tests for variable checking

99e3663

Signed-off-by: miguel-vila <[email protected]>

nhtruong requested changes Jun 10, 2024

View reviewed changes

nhtruong reviewed Jun 10, 2024

View reviewed changes

miguel-vila added 2 commits June 10, 2024 19:40

Merge remote-tracking branch 'upstream/main' into support-outputs-in-…

dd15ece

…tests

Merge remote-tracking branch 'upstream/main' into support-outputs-in-…

fc915f7

…tests

miguel-vila added 2 commits June 10, 2024 19:49

factor out supplemental chapter ev

68f46bd

Signed-off-by: miguel-vila <[email protected]>

fix spec

b05a452

Signed-off-by: miguel-vila <[email protected]>

miguel-vila added 2 commits June 10, 2024 20:04

separate story output tests

422a429

Signed-off-by: miguel-vila <[email protected]>

refactor tests so they return which chapter

1ca41b6

Signed-off-by: miguel-vila <[email protected]>

dblock requested changes Jun 11, 2024

View reviewed changes

miguel-vila added 3 commits June 11, 2024 08:41

fix spec

8b7345a

Signed-off-by: miguel-vila <[email protected]>

add changelog entry

e159bbe

Signed-off-by: miguel-vila <[email protected]>

move extract logic to ChapterOutput

b8ccfca

Signed-off-by: miguel-vila <[email protected]>

dblock reviewed Jun 11, 2024

View reviewed changes

dblock approved these changes Jun 11, 2024

View reviewed changes

nhtruong approved these changes Jun 11, 2024

View reviewed changes

nhtruong merged commit 53bdbd9 into opensearch-project:main Jun 11, 2024
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support outputs on tests #324

Support outputs on tests #324

miguel-vila commented Jun 6, 2024

dblock left a comment

dblock Jun 7, 2024

nhtruong Jun 7, 2024

nhtruong commented Jun 7, 2024 •

edited

Loading

nhtruong Jun 10, 2024

nhtruong Jun 10, 2024

nhtruong Jun 10, 2024 •

edited

Loading

nhtruong Jun 10, 2024

nhtruong Jun 10, 2024 •

edited

Loading

nhtruong Jun 10, 2024

codecov bot commented Jun 11, 2024 •

edited

Loading

github-actions bot commented Jun 11, 2024 •

edited

Loading

dblock left a comment

dblock Jun 11, 2024

dblock Jun 11, 2024

dblock Jun 11, 2024

dblock Jun 11, 2024

dblock left a comment

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

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

Support outputs on tests #324

Support outputs on tests #324

Conversation

miguel-vila commented Jun 6, 2024

dblock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nhtruong commented Jun 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nhtruong Jun 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nhtruong Jun 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jun 11, 2024 • edited Loading

Codecov Report

github-actions bot commented Jun 11, 2024 • edited Loading

Changes Analysis

API Changes

Summary

Report

API Coverage

dblock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dblock left a comment

Choose a reason for hiding this comment

nhtruong commented Jun 7, 2024 •

edited

Loading

nhtruong Jun 10, 2024 •

edited

Loading

nhtruong Jun 10, 2024 •

edited

Loading

codecov bot commented Jun 11, 2024 •

edited

Loading

github-actions bot commented Jun 11, 2024 •

edited

Loading