Catch response deserialization errors. #478

dblock · 2024-08-08T15:52:21Z

Description

After doing #476 I ran tests against AOS 2.7 and ran into an error deserializing CBOR. The implementation here will catch deserialization errors and bubble them up instead of failing unexpectedly.

Before:

$ npm run test:spec -- --tests tests/default/cat/health.yaml 

> opensearch_api_tools@1.0.0 test:spec
> ts-node tools/src/tester/test.ts --tests tests/default/cat/health.yaml

OpenSearch 2.7.0

/Users/dblock/source/opensearch-project/opensearch-api-specification/dblock-opensearch-api-specification/node_modules/cbor/lib/decoder.js:469
          throw new Error(`Additional info not implemented: ${ai}`)
                ^
Error: Additional info not implemented: 29
    at Decoder._parse (/Users/dblock/source/opensearch-project/opensearch-api-specification/dblock-opensearch-api-specification/node_modules/cbor/lib/decoder.js:469:17)
    at _parse.next (<anonymous>)
    at Object.decodeFirstSync (/Users/dblock/source/opensearch-project/opensearch-api-specification/dblock-opensearch-api-specification/node_modules/cbor/lib/decoder.js:228:22)
    at ChapterReader.#deserialize_payload (/Users/dblock/source/opensearch-project/opensearch-api-specification/dblock-opensearch-api-specification/tools/src/tester/ChapterReader.ts:112:69)
    at /Users/dblock/source/opensearch-project/opensearch-api-specification/dblock-opensearch-api-specification/tools/src/tester/ChapterReader.ts:53:48
    at processTicksAndRejections (node:internal/process/task_queues:95:5)
    at async ChapterReader.read (/Users/dblock/source/opensearch-project/opensearch-api-specification/dblock-opensearch-api-specification/tools/src/tester/ChapterReader.ts:41:5)
    at async ChapterEvaluator.#evaluate (/Users/dblock/source/opensearch-project/opensearch-api-specification/dblock-opensearch-api-specification/tools/src/tester/ChapterEvaluator.ts:65:22)
    at async ChapterEvaluator.evaluate (/Users/dblock/source/opensearch-project/opensearch-api-specification/dblock-opensearch-api-specification/tools/src/tester/ChapterEvaluator.ts:50:16)
    at async StoryEvaluator.#evaluate_chapters (/Users/dblock/source/opensearch-project/opensearch-api-specification/dblock-opensearch-api-specification/tools/src/tester/StoryEvaluator.ts:89:28)

After:

$ npm run test:spec -- --tests tests/default/cat/health.yaml 

> opensearch_api_tools@1.0.0 test:spec
> ts-node tools/src/tester/test.ts --tests tests/default/cat/health.yaml

OpenSearch 2.7.0


        ERROR   Cat in different formats (format=cbor).
            PASSED  PARAMETERS
                PASSED  format
            PASSED  REQUEST BODY
            ERROR   RESPONSE STATUS (Expected status 200, but failed to read the response. Additional info not implemented: 29)
            SKIPPED RESPONSE PAYLOAD BODY
            SKIPPED RESPONSE PAYLOAD SCHEMA
            SKIPPED RESPONSE OUTPUT VALUES

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

github-actions · 2024-08-08T15:54:29Z

Changes Analysis

Commit SHA: 8b6352f
Comparing To SHA: ecf3921

API Changes

Summary

NO CHANGES

Report

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

API Coverage

	Before	After	Δ
Covered (%)	510 (49.95 %)	510 (49.95 %)	0 (0 %)
Uncovered (%)	511 (50.05 %)	511 (50.05 %)	0 (0 %)
Unknown	24	24	0

github-actions · 2024-08-08T16:00:13Z

Spec Test Coverage Analysis

Total	Tested
534	118 (22.1 %)

tools/src/tester/StoryEvaluator.ts

Signed-off-by: dblock <dblock@amazon.com>

dblock · 2024-08-08T23:12:37Z

tools/tests/tester/fixtures/evals/error/prologue_error.yaml

@@ -16,7 +16,6 @@ prologues:
    overall:
      result: ERROR
      message: no such index [does_not_exists]
-      error: Request failed with status code 404


These aren't actually errors per-se, they get parsed and properly reflected in the failures of the test.

tools/src/tester/ChapterReader.ts

nhtruong · 2024-08-09T14:14:05Z

tools/src/tester/ChapterEvaluator.ts

@@ -122,15 +122,17 @@ export default class ChapterEvaluator {

  #evaluate_status(chapter: Chapter, response: ActualResponse): Evaluation {
    const expected_status = chapter.response?.status ?? 200
-    if (response.status === expected_status) return { result: Result.PASSED }
+    if (response.status === expected_status && response.error === undefined) return { result: Result.PASSED }


Is checking response.error === undefined necessary? From the surface when reading this line, I interpret the business logic as: Even if the status is 403 as the test story expects, the test will still fail regardless because it's an error.

Meanwhile the implementation here says other wise. It guarantees that only either response.status or response.error is set, but not both. That means if response.status === expected_status (which is not null) then response.error === undefined is also true.

It a small thing to nit-pick but keeping the guards simple and close to the high-level logic makes it a lot easier to maintain :)

If the server responded with the expected status (200), but data failed to deserialize, an exception will be thrown and you will end up with a matching status, but no response.error.

Without:

FAILED Cat in different formats (format=smile). PASSED PARAMETERS PASSED format PASSED REQUEST BODY PASSED RESPONSE STATUS FAILED RESPONSE PAYLOAD BODY (missing $root='[object Object]') FAILED RESPONSE PAYLOAD SCHEMA (data must be array) SKIPPED RESPONSE OUTPUT VALUES

With:

ERROR Cat in different formats (format=cbor). PASSED PARAMETERS PASSED format PASSED REQUEST BODY ERROR RESPONSE STATUS (Received 200: application/cbor. Additional info not implemented: 29) SKIPPED RESPONSE PAYLOAD BODY SKIPPED RESPONSE PAYLOAD SCHEMA SKIPPED RESPONSE OUTPUT VALUES

In this snippet, response.status and response.error are assigned in different code paths (i.e. mutual exclusive), how do you get them both at the same time?

It's a just a small nitpick tho. If it does happens like you describe then it's all good :)

Is that catch re-throwing into itself?! it totally happens

Signed-off-by: dblock <dblock@amazon.com>

dblock requested review from harshavamsi, sachetalva, nhtruong, Xtansia, VachaShah, Tokesh and aabeshov as code owners August 8, 2024 15:52

dblock added the skip-changelog No need to update CHANGELOG. label Aug 8, 2024

nhtruong reviewed Aug 8, 2024

View reviewed changes

tools/src/tester/StoryEvaluator.ts Outdated Show resolved Hide resolved

dblock added 2 commits August 8, 2024 17:16

Catch unexpected errors.

d4e9e07

Signed-off-by: dblock <dblock@amazon.com>

Eat the error instead of catching it.

3b178c1

Signed-off-by: dblock <dblock@amazon.com>

dblock force-pushed the unexpected-error branch from 3d93780 to 3b178c1 Compare August 8, 2024 22:40

dblock marked this pull request as draft August 8, 2024 22:40

dblock marked this pull request as ready for review August 8, 2024 22:55

dblock force-pushed the unexpected-error branch from bc9c062 to 7911c58 Compare August 8, 2024 22:57

dblock requested a review from nhtruong August 8, 2024 23:00

dblock changed the title ~~Catch unexpected errors.~~ Catch response deserialization errors. Aug 8, 2024

dblock commented Aug 8, 2024

View reviewed changes

nhtruong reviewed Aug 9, 2024

View reviewed changes

tools/src/tester/ChapterReader.ts Outdated Show resolved Hide resolved

nhtruong reviewed Aug 9, 2024

View reviewed changes

Distinguish errors and errors.

8b6352f

Signed-off-by: dblock <dblock@amazon.com>

dblock force-pushed the unexpected-error branch from 7911c58 to 8b6352f Compare August 9, 2024 17:14

nhtruong approved these changes Aug 12, 2024

View reviewed changes

dblock merged commit 3828838 into opensearch-project:main Aug 12, 2024
16 checks passed

dblock deleted the unexpected-error branch August 12, 2024 17:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Catch response deserialization errors. #478

Catch response deserialization errors. #478

dblock commented Aug 8, 2024 •

edited

Loading

github-actions bot commented Aug 8, 2024 •

edited

Loading

github-actions bot commented Aug 8, 2024 •

edited

Loading

dblock Aug 8, 2024

nhtruong Aug 9, 2024

dblock Aug 9, 2024 •

edited

Loading

nhtruong Aug 12, 2024

dblock Aug 12, 2024 •

edited

Loading

Catch response deserialization errors. #478

Catch response deserialization errors. #478

Conversation

dblock commented Aug 8, 2024 • edited Loading

Description

github-actions bot commented Aug 8, 2024 • edited Loading

Changes Analysis

API Changes

Summary

Report

API Coverage

github-actions bot commented Aug 8, 2024 • edited Loading

Spec Test Coverage Analysis

dblock Aug 8, 2024

Choose a reason for hiding this comment

nhtruong Aug 9, 2024

Choose a reason for hiding this comment

dblock Aug 9, 2024 • edited Loading

Choose a reason for hiding this comment

nhtruong Aug 12, 2024

Choose a reason for hiding this comment

dblock Aug 12, 2024 • edited Loading

Choose a reason for hiding this comment

dblock commented Aug 8, 2024 •

edited

Loading

github-actions bot commented Aug 8, 2024 •

edited

Loading

github-actions bot commented Aug 8, 2024 •

edited

Loading

dblock Aug 9, 2024 •

edited

Loading

dblock Aug 12, 2024 •

edited

Loading