add validator and test for date format #75
Conversation
There was a problem hiding this comment.
This testing strategy isn't going to work in the long run. You used the JSON Schema Test Suite as a reference to manually create tests. It needs to programmatically generate the tests from the test suite. The reason is that when the test suite changes, this tests will get out of sync.
See, https://github.com/hyperjump-io/json-schema/blob/main/draft-2020-12/json-schema-test-suite.spec.ts as an example. You should be able to copy that test runner and make minor changes to point to the right tests and enable format validation.
This will force you to test from the public API. Instead of testing the date function, you would be testing a schema with a format keyword. This is a good thing. Testing from the public API makes your tests more resilient to refactoring by decoupling the test from the internal details of how it was implemented.
You did a fantastic job at writing great test descriptions. Most people do a terrible job at that. I feel bad that I'm asking you not to use them.
The draft-2020-12 folder isn't the right place for the format code because it will end up being used in all of the dialects, not just 2020-12. I think putting it in lib makes sense. Also, I don't think it needs to be in an "optional" folder.
Right now, you have the implementation in a file called date.js, but it looks like the intention is for this to encompass all format validators. You aren't exporting the date validation function you export a structure containing a collection of validators. Let's give that file a name that matches what it does.
There was a problem hiding this comment.
This test runner is running all the required tests as well as the format tests. The required tests are already being run from json-schema-test-suite.spec.ts. You don't need to run them again.
Also, you don't need to modify the original runTestSuite function for this purpose other than to change the path of testSuiteFilePath to point to the optional tests. You don't need to add anything or change anything else other than maybe changing the describe to add - Optional Format Tests. That was nice change. Otherwise, the test runner should just work without modification.
The path you chose points specifically to date.json. You should use the folder (/optional/format) so it finds all the tests. You can use the skip feature to filter out the tests for formats you haven't implemented yet. You can skip a whole file using the pattern |draft2020-12|filename.json (example: |draft2020-12|uri.json). At this point, you'd want all files to be skipped except date.json.
|
When I use the test suite method for testing date formats, some tests fail unexpectedly. Do you have any idea why? in this I am skipping other format just till reach correct version, only testing date formats, please check |
|
You haven't hooked your date validator up to the format keyword. You'll need to check the "validateFormats" config you created, then if enabled call the date validator you implemented. Right now, you're just getting the default behavior, which is to always return |
|
I noticed that you merged
Using merge to update a branch can cause problems. Fortunately, it's easy to fix this mistake if we take care of it early. Just do a rebase now and it will clean up the merge commit and get things back to the way they should be. Please do that before pushing any more commits. |
There was a problem hiding this comment.
Have another look at #75 (review). You removed the extra test runner, but you're still using your custom test runner. You don't need to do that. Just use the original. It works. I promise.
| }; | ||
|
|
||
| const skip = new Set<string>([ | ||
| "|draft2020-12|optional/format/.*" |
There was a problem hiding this comment.
See my previous comment for the correct syntax. It should be just the file name without the path and wildcards aren't supported. You need to put an entry for each file you want to skip.
| const addRemotes = (dialectId: string, filePath = `${testSuitePath}/remotes`, url = "") => { | ||
| fs.readdirSync(filePath, { withFileTypes: true }) | ||
| .forEach((entry) => { | ||
| if (entry.isFile() && entry.name.endsWith(".json")) { | ||
| const remote = JSON.parse(fs.readFileSync(`${filePath}/${entry.name}`, "utf8")) as SchemaObject; | ||
| if (!remote.$schema || toAbsoluteIri(remote.$schema as string) === dialectId) { | ||
| registerSchema(remote, `http://localhost:1234${url}/${entry.name}`, dialectId); | ||
| } | ||
| } else if (entry.isDirectory()) { | ||
| addRemotes(dialectId, `${filePath}/${entry.name}`, `${url}/${entry.name}`); | ||
| } | ||
| }); | ||
| }; |
There was a problem hiding this comment.
You won't need remotes for format tests. So, you can leave this part out.
| const isValidDate = formatValidators.date(test.data); | ||
|
|
||
| if (test.valid) { | ||
| expect(isValidDate).toBe(true); | ||
| } else { | ||
| expect(isValidDate).toBe(false); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Don't test formatValidators directly. It's an implementation detail. The test above this is testing a schema with the format keyword that will call formatValidators. You want to test behaviors, not implementation details.
|
Closing due to lack of activity. |
related to Issue #72