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

Test Framework #299

Merged
merged 1 commit into from
May 29, 2024
Merged

Test Framework #299

merged 1 commit into from
May 29, 2024

Conversation

nhtruong
Copy link
Collaborator

@nhtruong nhtruong commented May 21, 2024

This PR adds a custom test framework for OpenSearch Open API Spec. Each YAML file in the ./tests folder (and its sub-folders) is a test story with 3 main components:

  • prologues: An array of requests made to the OpenSearch cluster to prepare it for the test (e.g. create and index and a few docs to test the search operations)
  • chapters: An array of requests, run sequentially, that actually test the spec:
    • Test if the operation exists in the spec
    • Test the provided parameters against their schemas in the spec
    • Test the provided request body against its schema in the spec
    • Test if the response code matches the expected response code
    • Test the response payload against its schema in the spec
  • epilogues: An array of requests co clean up the test environment (e.g. delete all indices and documents created during the test)

A JSON Schema for the test stories is also provided so that the IDE can help developers with autocomplete and validation when drafting the stories.

Make sure you have an OpenSearch cluster running at http://localhost:9200 before running the following command to initiate the test suite

npm run test:spec

Bellow is the result of running npm run test:spec -- --ignore_passed N
Check start.ts to see the options that can be passed to npm run test:spec command.

image

Remaining work:

  • Grab server URL from environment variable and add authentication.
  • Add a github workflow
  • Update repo docs
  • Add tests fro the framework (heavy lift)
  • Test should fail when a field is found in the response but not defined in the spec

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.

Copy link
Contributor

github-actions bot commented May 21, 2024

API specs implemented for 313/649 (48%) APIs.
Commit 39bb904.

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 overall! Needs READMEs, tests, and all the TODOs.

Left small usage/naming comments.

Try to get something that runs under CI first in.

json_schemas/story.schema.yaml Outdated Show resolved Hide resolved
json_schemas/story.schema.yaml Outdated Show resolved Hide resolved
tests/index_lifecycle.yaml Outdated Show resolved Hide resolved
tests/index_lifecycle.yaml Outdated Show resolved Hide resolved
tools/src/tester/start.ts Outdated Show resolved Hide resolved
json_schemas/story.schema.yaml Outdated Show resolved Hide resolved
json_schemas/story.schema.yaml Outdated Show resolved Hide resolved
json_schemas/story.schema.yaml Outdated Show resolved Hide resolved
tools/src/tester/helpers.ts Outdated Show resolved Hide resolved
tools/src/tester/ChapterEvaluator.ts Outdated Show resolved Hide resolved
tools/src/tester/StoryEvaluator.ts Outdated Show resolved Hide resolved
tools/src/tester/ResultsDisplayer.ts Outdated Show resolved Hide resolved
tools/src/tester/TestsRunner.ts Outdated Show resolved Hide resolved
tools/src/tester/TestsRunner.ts Outdated Show resolved Hide resolved
tools/src/tester/TestsRunner.ts Outdated Show resolved Hide resolved
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.

Tested this out.

tools/src/tester/ChapterReader.ts Outdated Show resolved Hide resolved
tools/src/tester/ChapterReader.ts Outdated Show resolved Hide resolved
tools/src/tester/start.ts Show resolved Hide resolved
tests/index_lifecycle.yaml Show resolved Hide resolved
tests/index_lifecycle.yaml Show resolved Hide resolved
tests/index_lifecycle.yaml Show resolved Hide resolved
Copy link
Contributor

github-actions bot commented May 27, 2024

Changes Analysis

Commit SHA: 078326f
Comparing To SHA: c58908d

API Changes

Summary


├─┬Paths
│ └─┬/{index}
│   └─┬HEAD
│     └─┬Responses
│       └──[➕] codes (23760:7)
└─┬Components
  └──[➕] responses (23760:7)

Document Element Total Changes Breaking Changes
paths 1 0
components 1 0
  • Total Changes: 2
  • Additions: 2

Report

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

API Coverage

Before After Δ
Covered (%) 476 (46.62 %) 476 (46.62 %) 0 (0 %)
Uncovered (%) 545 (53.38 %) 545 (53.38 %) 0 (0 %)
Unknown 24 24 0

Signed-off-by: Theo Truong <theotr@amazon.com>
@nhtruong
Copy link
Collaborator Author

nhtruong commented May 29, 2024

This is ready for the view and merged so people can start adding tests. I'll handle the remaining work in followup PRs:

  • Update repo docs
  • Add tests for the framework (heavy lift)
  • Additional Feature: Test should fail when a field is found in the response but not defined in the spec

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 LGTM! @Xtansia any objections to merging as is?

Copy link
Collaborator

@Xtansia Xtansia left a comment

Choose a reason for hiding this comment

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

There's still some parts I think could be improved, but not against merging as is to iterate on it

Comment on lines +9 to +11
.addOption(new Option('--spec, --spec_path <path>', 'path to the root folder of the multi-file spec').default('./spec'))
.addOption(new Option('--tests, --tests_path <path>', 'path to the root folder of the tests').default('./tests'))
.addOption(new Option('--tab_size <size>', 'tab size for displayed results').default('4'))
Copy link
Collaborator

Choose a reason for hiding this comment

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

CLI args are generally kebab-case. Additionally I'd say it's more commonly referred to as a tab width than a tab size.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

THanks for the callout. Will update them in the next PR

Copy link
Member

Choose a reason for hiding this comment

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

#301 with tests

export function overall_result (evaluations: Evaluation[]): Result {
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
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this correct? If I have 24 results, 23 passed but 1 skipped, then the overall result is skipped?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is for evaluating components results. A component is skipped when a previous component results in an error (say if a chapter could not be executed for validation, all subsequent chapters will be skipped). However that would result in ERROR for overall. So for now line 6 is actually redundant.

Hypothetically when we allow the user to skip some component evaluation (say skip evaluating the response body of a new endpoint still being developed), I think we should still mark the parent component as skipped. The print out will show all subcomponents so you can tell which subcomponent causes the SKIPPED status for the parent component. So:

  • If you see ERROR as the result for a component, at least one of its subcomponent has ERROR
  • otherwise, If you see FAILED as the result for a component, at least one of its subcomponent has FAILED
  • otherwise, If you see SKIPPED as the result for a component, at least one of its subcomponent has SKIPPED
  • else, all components have been vested and passed the test.

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