feat: add JSON Schema - #3314
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3314 +/- ##
==========================================
- Coverage 85.58% 85.57% -0.01%
==========================================
Files 160 160
Lines 47927 47933 +6
==========================================
+ Hits 41018 41019 +1
- Misses 6909 6914 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Thanks for doing this! Have you tested this end-to-end? I tried doing this a while ago and ran into tamasfe/taplo#779 which never got accepted upstream. |
|
Ahh, thanks, didn't realize you were the maintainer of Tombi! I'll have a look at this in more detail soon. |
|
If there are any improvements needed on the Tombi side, I’ll take care of them. |
| "required": [ | ||
| "store", | ||
| "profile" | ||
| ], |
There was a problem hiding this comment.
I started reviewing this and trying it out with tombi, but immediately noticed that we have these properties as required. In almost no cases are properties required -- an entirely empty config file is valid, for example, and is equivalent to not having a config file at all.
I think there's a difference here between the schema for the default config (where most things are required) and for custom repo-specific configs (where most things are optional). Could you look into this deeper?
I'll let you address this before continuing the review.
There was a problem hiding this comment.
I was made aware that there are exceptions that cannot be automatically generated from Rust structs.
I have modified NextestConfigDeserialize and StoreConfigImpl to use exceptions and implemented a special generation method for them.
There was a problem hiding this comment.
I have finished fixing the lint errors in nextest.toml for this repository. Could you please review it?
There was a problem hiding this comment.
Thanks. It's going to take me a little while because I have two very high priority things to work on this week.
There was a problem hiding this comment.
It is not urgent. Thank you!
00424fe to
687cc2e
Compare
687cc2e to
f9ab21c
Compare
| @@ -0,0 +1,6 @@ | |||
| [lsp] | |||
| formatting.enabled = false | |||
There was a problem hiding this comment.
The formatting has been disabled as it is not the focus of this PR.
With this configuration, you can verify how the nextet.json schema functions in tombi.
There was a problem hiding this comment.
Thanks! Tested it out a bit -- this is wonderful.
sunshowers
left a comment
There was a problem hiding this comment.
This is really, really great work -- very thorough, thank you. Just a few questions.
| #[cfg_attr(feature = "config-schema", schemars(deny_unknown_fields))] | ||
| #[serde(rename_all = "kebab-case", deny_unknown_fields)] |
There was a problem hiding this comment.
You can rely on schemars's serde attribute inheritance here, I think.
| schemars::json_schema!({ | ||
| "oneOf": [ | ||
| { "type": "integer", "minimum": 0 }, | ||
| { "type": "string", "enum": ["infinite"] } | ||
| ] | ||
| }) |
| serde_ignored = "0.1.14" | ||
| serde_json = "1.0.149" | ||
| serde_path_to_error = "0.1.20" | ||
| schemars = { version = "1.2.1", features = ["indexmap2"] } |
There was a problem hiding this comment.
I think this is fine, but it's funny -- at my workplace (Oxide Computer) we're still stuck on schemars 0.8 due to a few reasons. I was hoping at some point to use some tooling we have at work to also analyze schema evolution over time for nextest (both this config schema and things like the schema for JSON output). I guess if I want to do that, then I'll have to either downgrade this to schemars 0.8 or find a way to move the work tooling to schemars 1.
There was a problem hiding this comment.
Support for schemars v0.8 has been added.
Since the new syntax cannot be used, it is necessary to use the more verbose syntax.
There was a problem hiding this comment.
If you're okay with the latest schemars, I can revert it. How would you like to proceed?
There was a problem hiding this comment.
Yeah I think I'd prefer the latest schemars here.
| # Generate the JSON schema for .config/nextest.toml. | ||
| generate-config-schema: | ||
| cargo run --package nextest-runner --features config-schema --bin generate-config-schema |
There was a problem hiding this comment.
We should add this generate check as a CI step -- I'd probably add this as part of the lint job.
There was a problem hiding this comment.
Could you move the parenthetical to a regular comment?
| #[cfg_attr( | ||
| feature = "config-schema", | ||
| schemars(with = "Option<HashMap<String, CustomProfileImpl>>") | ||
| )] |
There was a problem hiding this comment.
This is pretty non-obvious so I think it needs a comment.
I would either inline a comment here, or add a short comment here and refer to the longer writeup in the helper binary.
| @@ -0,0 +1,6 @@ | |||
| [lsp] | |||
| formatting.enabled = false | |||
There was a problem hiding this comment.
Thanks! Tested it out a bit -- this is wonderful.
| use nextest_runner::config::core::nextest_config_schema; | ||
| use std::fs; | ||
|
|
||
| fn main() -> Result<()> { |
There was a problem hiding this comment.
I feel like there needs to be a writeup somewhere of some of the internal design decisions taken while generating the schema:
- how
deny_unknown_fieldsis meant to play the role ofserde_ignored(which is why you can't rely on schemars's serde inheritance - related, why some fields of type
Tsay that their schema isOption<T> - how types with custom deserializers are handled
All of these make sense, but I'm likely going to forget about them when I look a few months from now :)
Thoughts about putting this writeup in this file? This would be a natural place to look. Maybe if it gets complex enough we can even have a small design doc on the nextest site.
There was a problem hiding this comment.
I moved the comments to NextestConfigDeserialize because having them in generate-config-schema.rs felt too far away from the definitions.
| { | ||
| "$schema": "https://json-schema.org/draft/2020-12/schema", | ||
| "title": "NextestConfigDeserialize", | ||
| "type": "object", | ||
| "properties": { |
There was a problem hiding this comment.
As a followup PR, would you be willing to write something that (1) embeds this schema within nextest, and (2) adds a cargo nextest self config-schema command that dumps out a file? I would prefer we not have to include schemars as part of the release (so your optional feature is fantastic, thanks), but it would still be nice to have access to the schemal.
| #[cfg_attr(feature = "config-schema", derive(schemars::JsonSchema))] | ||
| #[cfg_attr(feature = "config-schema", schemars(deny_unknown_fields))] | ||
| #[serde(rename_all = "kebab-case")] | ||
| struct NextestConfigDeserialize { | ||
| pub(crate) struct NextestConfigDeserialize { |
There was a problem hiding this comment.
Curious, how hard would it be to add a test which ensures the default config passes schema validation?
There was a problem hiding this comment.
Added a test for .config/nextest.toml.
f71ef61 to
77d356a
Compare
…ences - Updated `schemars` dependency in `Cargo.toml` and `Cargo.lock` to version 0.8.22. - Modified JSON schema references in `nextest.json` to use the correct format. - Refactored JSON schema generation in various configuration elements to improve clarity and maintainability.
…uration structs - Enhanced `NextestConfigDeserialize` and `StoreConfigImpl` to indicate optional fields in the JSON schema. - Updated `ArchiveConfig` to reflect that the `include` field should also be optional, improving schema clarity.
- Introduced a new `ConfigSchema` subcommand in `SelfCommand` to output the embedded JSON Schema for `nextest.toml`. - Added a static string `CONFIG_SCHEMA` to hold the JSON Schema content.
|
I’ve gone ahead and fixed all the review comments I could address. |
…ma references" This reverts commit f0c2222.
|
Looks great. I'll address the remaining issues I identified myself before landing this. Thanks again for doing all this work and for maintaining Tombi (I'm excited to finally have JSON Schema support for the nextest config -- Taplo was a bit of a brutal experience) |

Support: #3315
I have created a JSON Schema using schemars so that
nextest.tomlcan be validated with tombi or taplo.