config-schema: add test asserting schema defaults match default config #6361
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The schema said that the default was "never" which is actually not one of the allowed enum values.
…iles The previous implementation of `assert_no_forgotten_test_files` hard-coded the name of the `runner` integration test and required all other source files to appear in matching `mod` declarations. Thus, this approach cannot handle multiple integration tests. However, additional integration tests may be desirable - to support tests using a custom test harness (see upcoming commits) - to balance the trade-off between test run time and compile time as the test suite grows in the future. The new implementation first uses `taplo` to parse the `[[test]]` sections of the manifest to identify integration test main modules, and then searches in those for `mod` declarations. This is then compared to the list of source files in the tests directory. Like the previous implementation, the new one does not attempt to recurse into submodules or to handle directory-style modules; just like before it only treats source files without a module declaration as an error and relies on the compiler to complain about the other way around. When `taplo` is not installed, the check is skipped unless it is running in CI where we require `taplo` to be available.
The `datatest-stable` crate allows to dynamically instantiate test cases based on available files. This is applied to `test_config_schema` to create one test case per config file. As case in point, the test case for `hints.toml` was missing previously, hence the total number of tests is up one. This will become useful when adding more config examples to somewhat exhaust the schema. `datatest-stable` uses a custom test harness and thus cannot be used in the same integration test binary that all of the other test modules run in. However, if data-driven tests are to be used for other applications, they can share in the same binary, so the module structure is already set up to mirror the central "runner" approach.
Adds a bunch of additional sample config toml files. Via the `datatest_runner`, these each correspond to a test case to check that the toml is correctly (in-)validated according to the schema. The `valid/*.toml` files typically define multiple related config options at once. Where there's some overlap with the default configs in `cli/src/config`, the aim was to choose different allowed values, e.g. hex colors, file size in bytes (numeric), etc. The `invalid/*.toml` files typically only define a single offending property such as to not obscure individual false negatives. All of the "invalid" files are still valid toml as the aim is not to test the `toml_edit` crate or Taplo. The sample files all contain a Taplo schema directive. This allows them to be validated against the schema on the fly by Taplo's LSP and derived IDE plugins to speed up editing and immediately highlight offending options. Closes #5695.
These are two more instances where the default values were wrong and in
fact not even consistent with the schema itself.
I've found these by running
```sh
jq -r 'paths(type == "object" and has("default")) as $p | getpath($p).default | tojson as $v | $p | map("\"\(select(. != "properties"))\"") | join(".") as $k | "\($k) = \($v)"' cli/src/config-schema.json | taplo check --schema=file://$PWD/cli/src/config-schema.json -
```
which uses `jq` to filter the default values from the schema definition
to create a rudimentary TOML file containing all the defaults according
to the schema and then uses `taplo` the validate this TOML against the
schema.
This approach could be developed further to also parse the intermediate
TOML file and compare the result with the default config (from parsing
an empty config). That would not only test for self-consistency of the
schema's proclaimed defaults but also for consistency with the actual
defaults as assumed by jj.
While `ui.pager` can be a string which will be tokenized on whitespace, and argument token array, or a command/env table, the `command` key within that table currently must be an array. The schema previously explicitly also allowed it to be a string but that does not actually work, as exemplified by running: ```sh $ jj --config-file cli/tests/sample-configs/invalid/ui.pager_command-env_string.toml config list Config error: Invalid type or value for ui.pager Caused by: data did not match any variant of untagged enum CommandNameAndArgs ``` `CommandNameAndArgs` should potentially be changed to allow strings. For now, the schema has been updated to reflect the status quo. A new sample toml has been added to the `invalid` directory to cover this; prior to updating the schema, this new test case failed. Once the behavior is changed to allow string, the file merely needs to be moved to `valid`.
Anytime an external tool is referenced in the config, the command can be
provided as a string or as a token array. In the latter case, the array
must not be empty; at least the command name must be provided.
The schema didn't previously object to an empty array, though; this has
now been rectified. I've added more sample configs to cover this case.
Those same configs can also be used to illustrate that this is indeed
jj's current behavior:
$ jj --config-file cli/tests/sample-configs/invalid/ui.pager_empty_array.toml show
Config error: Invalid type or value for ui.pager
Caused by: data did not match any variant of untagged enum CommandNameAndArgs
$ jj --config-file cli/tests/sample-configs/invalid/ui.pager.command_empty_array.toml show
Config error: Invalid type or value for ui.pager
Caused by: data did not match any variant of untagged enum CommandNameAndArgs
$ jj --config-file cli/tests/sample-configs/invalid/ui.editor_empty_array.toml config edit --user
Config error: Invalid type or value for ui.editor
Caused by: data did not match any variant of untagged enum CommandNameAndArgs
$ jj --config-file cli/tests/sample-configs/invalid/ui.diff-editor_empty_array.toml split
Error: Failed to load tool configuration
Caused by:
1: Invalid type or value for ui.diff-editor
2: data did not match any variant of untagged enum CommandNameAndArgs
$ jj --config-file cli/tests/sample-configs/invalid/ui.merge-editor_empty_array.toml resolve
Error: Failed to load tool configuration
Caused by:
1: Invalid type or value for ui.merge-editor
2: data did not match any variant of untagged enum CommandNameAndArgs
$ jj --config-file cli/tests/sample-configs/invalid/ui.diff.tool_empty_array.toml diff
Config error: Invalid type or value for ui.diff.tool
Caused by: data did not match any variant of untagged enum CommandNameAndArgs
$ jj --config-file cli/tests/sample-configs/invalid/fix.tools.command_empty_array.toml fix
Config error: Invalid type or value for fix.tools.black
Caused by: data did not match any variant of untagged enum CommandNameAndArgs
in `command`
As a notable exception, `ui.default-command` *is* allowed to be an empty
array. In that case, `jj` will print a usage message. This is also
covered by a valid sample config.
…ol config Not sure if that is intentional or should rather be considered a bug, but currently the "structured" option for specifying an external tool requires that both the command "command" and the "env" keys are specified. The "command" key makes sense; for the "env" key it came as a surprise, but it can be argued that this form should only be used when environment variables need to be specified and the plain string or array form should be used otherwise. Either way, the schema did not accurately reflect the current behavior; now it does. Two sample configs have been added as schema test cases.
The `CommandNameAndArgs` struct is used in multiple places to specify external tools. Previously, the schema only allowed for this in `ui.pager`. This commit adds a few sample configs which define variables editors and fix tools as commands with env vars. The schema has also been updated to make these valid.
A pattern has emerged where a integration tests check for the availability of an external tool (`git`, `taplo`, `gpg`, ...) and skip the test (by simply passing it) when it is not available. To check this, the program is run with the `--version` flag. Some tests require that the program be available at least when running in CI, by calling `ensure_running_outside_ci` conditionally on the outcome. The decision is up to each test, though, the utility merely returns a `bool`.
…s()"` The default value became definitely outdated when "branches" were renamed to "bookmarks" in 0.20 (and broke in 0.28). The actual default of `revset-aliases."immutable_heads()"` is in fact `builtin_immutable_heads()`, now. However, if one were to want override to default `immutable_heads()`, `builtin_immutable_heads()` would likely not be a helpful starting point. Therefore, it may make sense to keep this "resolved" default value in the schema.
Extracts the `"default"` values from the schema and creates a synthetic TOML file holding all the defaults according to the schema. This is done through some `jq` magic and is not perfect but rather a best effort. If `jq` is not available, the test is skipped; in CI `jq` is required. The test then run `jj config get` in the test env for each key in that defaults file and compares the resulting value with the schema default. For a few keys, there are actually no defaults known to `jj config get`, because they are hard-coded or dynamic. These exceptions are intercepted and explained in the test.
jgreitemann
force-pushed
the
push-kqnrrtyxolrm
branch
from
ba915db to
38597b0
Compare
Because the datatests don't use the default libtest harness, datatests and normal tests cannot be mixed in the same module or even binary. `test_config_schema` is renamed, both to reflect that it is unlike the other `test_*` modules, but also to make the name available for "normal" tests related to the config schema.
While the existing test checks that the schema defaults are consistent with the output of `jj config get` (with default config), it would not find type errors like the ones fixed in e9da94e. This add another test case which validates the synthetic TOML containing the default values according to the schema against the schema itself.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a somewhat failed attempt at a stacked PR, building on PR #6316. (I created the other PR from a fork, but that means that it cannot be used as "base" for a stacked PR.) As per recommendation on Discord, I'm still creating this PR now, but only
threefive commits are actually new:The remaining commits are already in PR #6316 and any comments on them should go in there.
Checklist
If applicable:
CHANGELOG.md