search

json-schema-org / JSON-Schema-Test-Suite

A language agnostic test suite for the JSON Schema specifications
MIT License
603 stars 205 forks source link

`draft-next` drop tests asserting empty fragments in `$id` are valid #716

Closed notEthan closed 5 months ago

notEthan commented 5 months ago

Per https://github.com/json-schema-org/json-schema-spec/pull/1291

notEthan commented 5 months ago

also mentioned in https://github.com/marksparkza/jschon/issues/69 but seems to have not led to any action in the test suite

notEthan commented 5 months ago

True, that is an option. I question the value of it - it's really just a test of the pattern keyword applied to $id, and pattern is already well-covered with its own tests. But that's true both before and after this change. I'm neutral on it, and will restore the tests with valid: false if others prefer that.

jdesrosiers commented 5 months ago

That's a good point. The point of this test suite isn't to test the meta-schema.

gregsdennis commented 5 months ago

The point of this test suite isn't to test the meta-schema.

But it is the point of the suite to verify an implementation adheres to the spec.

I agree that whether the meta-schema enforces that $id doesn't have a fragment is not subject to test. However, I still think it's the responsibility of the implementation to verify this requirement.

jdesrosiers commented 5 months ago

I still think it's the responsibility of the implementation to verify this requirement.

I agree, but I don't think we have the capability to check for that using the test suite. We don't have the ability to test for InvalidSchema type of failures. This test validates a schema against the meta-schema as a kind of workaround for that problem. It doesn't actually check that the implementation is capable of verifying the requirement. It's just testing the meta-schema.

notEthan commented 5 months ago

I'm not sure where that brings us as far as either this PR or the rest of the id.json tests that boil down to pattern matching. Should they stay?

jdesrosiers commented 5 months ago

I'm not sure where that brings us as far as either this PR or the rest of the id.json tests that boil down to pattern matching. Should they stay?

I'd say that they should probably be removed. It might be interesting to have a suite that specifically tests the meta-schema, but that's not what this is. @Julian, @gregsdennis, what do you think?

Julian commented 5 months ago

Indifferent personally -- the argument for having them (and #354 and #244 in general) is just to help start to collect these tests instead of making no progress there, but I'm not sure we've made meaningful progress there regardless, so it's not much harm removing them either.

(Tl;dr weak +0 for leaving+inverting but very weak)

gregsdennis commented 5 months ago

I don't think we have the capability to check for that using the test suite. We don't have the ability to test for InvalidSchema type of failures.

This is a good point.

If an invalid $id means an invalid schema, and this suite requires valid schemas, then we don't have a way to test this aside from the meta-schema, which this suite isn't intended for.

So.... yeah, I say drop them.

jdesrosiers commented 5 months ago

the argument for having them (and https://github.com/json-schema-org/JSON-Schema-Test-Suite/pull/354 and https://github.com/json-schema-org/JSON-Schema-Test-Suite/issues/244 in general) is just to help start to collect these tests instead of making no progress there

This is a great point that I hadn't thought of. But, I also agree that there's not terribly much there and what is there is pretty basic and unlikely to be missed if we recreate this later when we have the capability to set invalid schemas.

So, if no one minds me making the call... @notEthan, would you mind either creating an new PR or updating this one that removes all the meta-schema-based $id tests?

notEthan commented 5 months ago

Closing, obviated by https://github.com/json-schema-org/JSON-Schema-Test-Suite/pull/718