chore(repository-json-schema): add JSON schema type tests #1323
Conversation
| expect(modelToJsonSchema(NoModelMeta)).to.eql({}); | ||
| const emptyJson = modelToJsonSchema(Empty); | ||
| const noModelMetaJson = modelToJsonSchema(NoModelMeta); | ||
| expect(emptyJson).to.eql({}); |
There was a problem hiding this comment.
This seems redundant. Won't expectValidJsonSchema check if it's {} or not?
There was a problem hiding this comment.
I guess it is redundant. On the other hand though, what if someday they decide {} is not a valid JSON Schema? In that case, we'd want this to test to fail while still maintaining that emptyJson is an empty object and that {} is not a valid JSON Schema
There was a problem hiding this comment.
Unlikely, but I'm ok with the redundancy.
| @@ -0,0 +1,63 @@ | |||
| import {JsonSchema} from '../../src'; | |||
There was a problem hiding this comment.
Move import below the Copyright notice.
| const validate = new Ajv().compile( | ||
| require('ajv/lib/refs/json-schema-draft-06.json'), | ||
| ); | ||
| expect(validate(schema)).to.be.true(); |
There was a problem hiding this comment.
+1 for creating a shared function expectValidJsonSchema.
When the validation fails because the schema is not valid, what information is provided to the user (LB4 framework developer) to help them troubleshoot the problem?
IIUC, the test prints "expected false to be true", which is not helpful at all :(
According to https://www.npmjs.com/package/ajv#validation-errors, validation errors are provided as an array in validate.errors. There is also ajv.errorsText() function that can be used to build a user-friendly message.
I am proposing to rework this check as follows:
const ajv = new Ajv();
const validate = ajv.compile(require('ajv/lib/refs/json-schema-draft-06.json'));
const isValid = validate(schema);
const result = isValid ? 'JSON Schema is valid' : ajv.errorsText(validate.errors);
expect(result).to.equal('JSON Schema is valid');An example test failure:
AssertionError: expected 'data.properties should be object' to be 'JSON Schema is valid'
+ expected - actual
-data.properties should be object
+JSON Schema is valid
| describe('JsonSchema interface', () => { | ||
| /** | ||
| * The classes below are declared as tests for the Interfaces. | ||
| * The TS Compiler will complain if an interface changes with a way |
shimks
force-pushed
the
json-schema-tests
branch
2 times, most recently
from
81abb30 to
71e0375
Compare
| }); | ||
| }); | ||
|
|
||
| function expectValidJsonSchema(schema: JsonSchema) { |
There was a problem hiding this comment.
Should we consider exporting this as a util function from this package?
There was a problem hiding this comment.
🤔 This means that we'd have to have @loopback/testlab and ajv as dependencies for @loopback/repository. Looking at https://bundlephobia.com/result?p=ajv@6.5.0, would you consider ajv to be sizable?
There was a problem hiding this comment.
It's not that big imo but I'm ok not exporting this as a utility function.
There was a problem hiding this comment.
@shimks I think @virkt25 was suggesting to export expectValidJsonSchema from repository-json-schema, not testlab package?
At one hand, a readily-available helper for verifying JSON Schema may be helpful outside of this module tests too. On the other hand, exporting a test helper from a non-test-helper module is tricky as we may need to move dev-dependencies to regular dependencies. As for testlab, it should not become a kitchen-sink for everything testing related.
In the light of what I wrote above, I think we should not be exporting this utility function.
| }); | ||
| }); | ||
|
|
||
| function expectValidJsonSchema(schema: JsonSchema) { |
There was a problem hiding this comment.
@shimks I think @virkt25 was suggesting to export expectValidJsonSchema from repository-json-schema, not testlab package?
At one hand, a readily-available helper for verifying JSON Schema may be helpful outside of this module tests too. On the other hand, exporting a test helper from a non-test-helper module is tricky as we may need to move dev-dependencies to regular dependencies. As for testlab, it should not become a kitchen-sink for everything testing related.
In the light of what I wrote above, I think we should not be exporting this utility function.
Checklist
npm testpasses on your machinepackages/cliwere updatedexamples/*were updated