Added StoryValidator #362
Added StoryValidator #362
Conversation
nhtruong
requested review from
dblock,
harshavamsi,
sachetalva,
Xtansia,
VachaShah,
Tokesh and
aabeshov
as code owners
Changes AnalysisCommit SHA: f1d6d85 API ChangesSummaryNO CHANGES ReportThe full API changes report is available at: https://github.com/opensearch-project/opensearch-api-specification/actions/runs/9686292855/artifacts/1642073848 API Coverage
|
There was a problem hiding this comment.
Looks good, I have some questions.
I have a general concern for lack of unit tests. I understand that we're testing the end-to-end flow, but for any new given class like StoryValidator I'd expect to see a StoryValidator.test.ts that checks the basics at the very least.
| @@ -12,8 +11,7 @@ prologues: | |||
| director: Bennett Miller | |||
| title: Moneyball | |||
| year: 2011 | |||
| response: | |||
| status: 201 | |||
| status: [201] | |||
There was a problem hiding this comment.
In prologues and epilogues, we don't evaluate any spec but rather simply run the HTTP requests to setup and tear down. By default, the framework accepts status: [200, 201] as okay and other codes will make the prologue or epilogue return an error. So in this case in particular, we could have omitted status. Specifying status is usually for when cleanup where we want to delete a resource created in one of the chapters but we want to account for 404 where the resource was not created successfully. In this case, it's usually status: [200, 404]
There was a problem hiding this comment.
Also added unit tests for StoryValidator
tools/src/tester/ResultLogger.ts
Outdated
| @@ -29,15 +29,17 @@ export class ConsoleResultLogger implements ResultLogger { | |||
| } | |||
|
|
|||
| log (evaluation: StoryEvaluation): void { | |||
| const with_padding = [Result.FAILED, Result.ERROR].includes(evaluation.result) || this._verbose | |||
| if(with_padding) console.log() | |||
There was a problem hiding this comment.
Lack of space after if looks suspicious, missing linter?
There was a problem hiding this comment.
Same issue with the previous PR: the rule was moved into a plugin.
I've resolved it.
| @@ -32,7 +32,7 @@ export default class SchemaValidator { | |||
| validate (schema: OpenAPIV3.SchemaObject, data: any): Evaluation { | |||
| const validate = this.ajv.compile(schema) | |||
| const valid = validate(data) | |||
| if (! valid) { | |||
| if (!valid) { | |||
There was a problem hiding this comment.
Same issue with the previous PR: the rule was moved into a plugin.
I've resolved it.
| private readonly validate_schema: ValidateFunction | ||
|
|
||
| constructor() { | ||
| this.ajv = new Ajv2019({ allErrors: true, strict: false }) |
There was a problem hiding this comment.
We use plain ajv elsewhere, why is this using a different one?
There was a problem hiding this comment.
It's because of the use of unevaluatedProperties, which is a new feature in JSON Schema draft 2019-09. We use this in test_story.schema to enable additionalProperties: false for allOf objects. This was how I caught this infraction.
There was a problem hiding this comment.
Should we upgrade all schema checks?
Validating stories before running them. Signed-off-by: Theo Truong <[email protected]>
nhtruong
force-pushed
the
validate_stories
branch
from
affc950
to
f1d6d85
Compare
Validating stories before running them.
Issues Resolved
closes #354
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.