adding test_check_schema_builtin

[Borda]

adding test for #550

cc: @sirosen @edgarrmondragon

[sirosen]

Hi, thanks for the PR!

I'm still getting up to speed on the situation re #550. But the azure-pipelines hook is already declared with --regex-variant nonunicode.
And I think that must be working correctly since the acceptance tests for Azure are healthy; these pass:

tests/acceptance/test_example_files.py::test_hook_positive_examples[azure-pipelines/marshmallow.yaml]
tests/acceptance/test_example_files.py::test_hook_positive_examples[azure-pipelines/object-defined-by-expression-map.yaml]
tests/acceptance/test_example_files.py::test_hook_positive_examples[azure-pipelines/expression-from-lang-server.yaml]
tests/acceptance/test_example_files.py::test_hook_positive_examples[azure-pipelines/expression-transform.yaml]

I would very strongly prefer not including unrelated changes (e.g., changing something to an ABC) in something like this. If you think something internal should change, please make that as a separate PR and explain your rationale -- class hierarchies and other project structure should not change as mere side-effects of other changes.

---

I'm very unclear on what the new test is testing, since it doesn't seem to be reading the schema catalog data to determine which regex variant to use. Is this just a duplicate of the pre-commit hook which validates the vendored schemas?

check-jsonschema/.pre-commit-config.yaml

Lines 9 to 16 in 121fbf7

	- id: check-metaschema
	name: Validate Vendored Schemas
	files: ^src/check_jsonschema/builtin_schemas/vendor/.*\.json$
	exclude: ^src/check_jsonschema/builtin_schemas/vendor/azure-pipelines\.json$
	- id: check-metaschema
	name: Validate Vendored Schemas (nonunicode regexes)
	files: ^src/check_jsonschema/builtin_schemas/vendor/azure-pipelines\.json$
	args: ["--regex-variant", "nonunicode"]

[Borda]

But the azure-pipelines hook is already declared with --regex-variant nonunicode

yes if you use it as a hook but but you call it with CLI and you are not aware of this specificity (could not find it and the error trace did not help either) you won't know what has happened or why it crashed...

I'm very unclear on what the new test is testing, since it doesn't seem to be reading the schema catalog data to determine which regex variant to use.

it is related to CLI not hooks... so from a user perspective if you use builtin schema the default shall be relevant to the selected schema, not that you need to select another argument...

about this PR, I did not find any validation that all builtin schemas are tested so at first added a test which helped me understand that default is not the right default for all schemas

[Borda]

I would very strongly prefer not including unrelated changes (e.g., changing something to an ABC) in something like this. If you think something internal should change, please make that as a separate PR and explain your rationale -- class hierarchies and other project structure should not change as mere side-effects of other changes.

sure I am happy to revert the abstraction change :)