Skip to content

Switch to @hyperjump/json-schema-coverage for schema test coverage #4762

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

Merged
merged 6 commits into from
Jul 14, 2025

Conversation

jdesrosiers
Copy link
Contributor

Since our last update to the JSON Schema test coverage script, I've been working on a more complete solution and it's finally ready to share! I was able to generate istanbul coverage documents with the information collected from the @hyperjump/json-schema EvaluationPlugin we introduced in the last update. That allows us to generate familiar looking reports. I also figured out how to integrate it with Vitest, so it's super easy to use. I even included support for YAML out-of-the-box. See, @hyperjump/json-schema-coverage for more details and examples.

WARNING: Because this does a more thorough job than the current solution, it will no longer report 100% coverage. More tests will need to be added. The biggest difference is that I added branch coverage support. Instead of just checking that the keyword was covered, it now also tracks whether both the true and false result of a keyword are covered.

WARNING: In the current version, we added a hack to ignore #/$defs/schema in schema.yaml because it isn't possible to cover that location if we're testing against schema-base.yaml. This new package doesn't have a solution for ignoring things. We could add a test that uses schema.yaml to cover that location, or look into some way to ignore coverage in the new solution.

NOTE: I think the dev branch is the right place to make this PR, but please let me know if that's wrong. The PR template says main, but the coverage tests don't exist on that branch.


  • schema changes are included in this pull request
  • schema changes are needed for this pull request but not done yet
  • no schema changes are needed for this pull request

@jdesrosiers jdesrosiers requested review from a team as code owners July 9, 2025 17:29
@jdesrosiers
Copy link
Contributor Author

Here's an example of what we'll get for the 3.1 branch.

 % Coverage report from @hyperjump/json-schema-coverage/vitest/coverage-provider                     
------------------|---------|----------|---------|---------|-------------------                      
File              | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                       
------------------|---------|----------|---------|---------|-------------------                      
All files         |   99.42 |    56.34 |   92.58 |   99.42 |                                         
 dialect.yaml     |     100 |      100 |     100 |     100 |                                         
 meta.yaml        |   98.14 |    53.12 |    82.6 |   98.14 | 18                                      
 schema-base.yaml |     100 |       50 |     100 |     100 | 13                                      
 schema.yaml      |   99.49 |    56.57 |   93.05 |   99.49 | 630,726-729,945                         
------------------|---------|----------|---------|---------|-------------------                      

image

Copy link
Contributor

@ralfhandl ralfhandl left a comment

Choose a reason for hiding this comment

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

@jdesrosiers Really nice!

Unfortunately we'd need some additions to use it, see comments.

@jdesrosiers
Copy link
Contributor Author

It looks like defining a vocabulary isn't quite working, so this isn't working on the v3.2-dev branch yet.

@ralfhandl
Copy link
Contributor

It does work with the "old" solution, both for validation pass/fail and for coverage.

@jdesrosiers jdesrosiers force-pushed the json-schema-coverage branch from bd3cdf3 to 96f62b8 Compare July 13, 2025 03:19
@jdesrosiers
Copy link
Contributor Author

I fixed the problem with registering the vocabulary, so this now works for all branches including v3.2.

I also rebased to fix the conflict.

As far as I know, the only thing missing at this point is adding additional tests to cover the parts the new solution catches that the old one didn't. I'll leave that part to someone else.

@ralfhandl
Copy link
Contributor

Unfortunately this does not run on Windows machines, it fails with

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯ Unhandled Error ⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯
RetrievalError: Unable to load resource 'c:\git\OpenAPI-Specification\src\schemas\validation\dialect.yaml'.
 ❯ get node_modules/@hyperjump/browser/lib/browser/browser.js:29:13
 ❯ getSchema node_modules/@hyperjump/json-schema/lib/schema.js:33:24
 ❯ FileCoverageMapService.addFromFile node_modules/@hyperjump/json-schema-coverage/src/coverage-map-service.js:43:26
 ❯ FileCoverageMapService.addFromFile node_modules/@hyperjump/json-schema-coverage/src/vitest/file-coverage-map-service.js:57:35
 ❯ Object.setup node_modules/@hyperjump/json-schema-coverage/src/vitest/build-coverage-maps.js:55:29
 ❯ TestProject._initializeGlobalSetup node_modules/vitest/dist/chunks/cli-api.BkDphVBG.js:7074:21
 ❯ Vitest.initializeGlobalSetup node_modules/vitest/dist/chunks/cli-api.BkDphVBG.js:9724:35
 ❯ node_modules/vitest/dist/chunks/cli-api.BkDphVBG.js:9636:5
 ❯ Vitest.runFiles node_modules/vitest/dist/chunks/cli-api.BkDphVBG.js:9666:10  
 ❯ Vitest.start node_modules/vitest/dist/chunks/cli-api.BkDphVBG.js:9554:18     

Caused by: UnsupportedUriSchemeError: The 'c:' URI scheme is not supported. Use the 'addUriSchemePlugin' function to add support for 'c:' URIs.
 ❯ retrieve node_modules/@hyperjump/browser/lib/uri-schemes/uri-schemes.js:19:11
 ❯ get node_modules/@hyperjump/browser/lib/browser/browser.js:23:30
 ❯ getSchema node_modules/@hyperjump/json-schema/lib/schema.js:33:24
 ❯ FileCoverageMapService.addFromFile node_modules/@hyperjump/json-schema-coverage/src/coverage-map-service.js:43:26
 ❯ FileCoverageMapService.addFromFile node_modules/@hyperjump/json-schema-coverage/src/vitest/file-coverage-map-service.js:57:35
 ❯ Object.setup node_modules/@hyperjump/json-schema-coverage/src/vitest/build-coverage-maps.js:55:29
 ❯ TestProject._initializeGlobalSetup node_modules/vitest/dist/chunks/cli-api.BkDphVBG.js:7074:21
 ❯ Vitest.initializeGlobalSetup node_modules/vitest/dist/chunks/cli-api.BkDphVBG.js:9724:35
 ❯ node_modules/vitest/dist/chunks/cli-api.BkDphVBG.js:9636:5
 ❯ Vitest.runFiles node_modules/vitest/dist/chunks/cli-api.BkDphVBG.js:9666:10  

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯
Serialized Error: { scheme: 'c' }

The previous implementation worked fine on Windows and Ubuntu. Maybe something is missing when resolving relative paths into file URLs, it should actually try to load resource

file:///C:/git/OpenAPI-Specification/src/schemas/validation/dialect.yaml

which it would find.

@jdesrosiers
Copy link
Contributor Author

jdesrosiers commented Jul 13, 2025

Unfortunately this does not run on Windows machines

I always forget about Windows! I made some fixes and made sure the tests pass with Github actions on windows. I don't have a windows machine to test locally, so let me know if there are any more issues.

@jdesrosiers jdesrosiers force-pushed the json-schema-coverage branch from f0c16c7 to 059a4b0 Compare July 13, 2025 19:37
@ralfhandl
Copy link
Contributor

I don't have a windows machine to test locally, so let me know if there are any more issues.

Works like a charm, and you even got rid of the dependency to nyc 😄

@ralfhandl ralfhandl merged commit cf80708 into OAI:dev Jul 14, 2025
2 checks passed
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.

2 participants