Move test of \a regex character to optional/format/regex

notEthan commented 3 months ago

This was validating "\a" as a schema pattern described by the meta-schema as format: regex. This moves it to directly test the pattern against a format: regex schema.

Julian commented 2 months ago

This doesn't really seem correct to me. The regex file is often run by implementations using some other regex dialect, and in that other dialect \a very well may be a valid regular expression. The file these were in is the file meant to represent thing specific to ECMA regexes.

karenetheridge commented 2 months ago

We already put format tests in optional/ because it's understood that full specification adherence is spotty. If the ECMA spec says that this string is a valid (or invalid) pattern, then it belongs in this file (optional/format/regex.json).

The test definitely does not belong where it is currently because formats are never invalid using the draft2020-12 metaschema dialect, because formats are only annotations there.

notEthan commented 2 months ago

Reasons I think this test belongs where this PR moves it:

It's validating a property of a schema (pattern) against the meta-schema (specifying "format": "regex") but there's no reason the meta-schema needs to be involved in this, validating the pattern "\\a" directly against a "format": "regex" schema has the same effect.
The rest of the tests in ecmascript-regex.json are testing strings against a pattern, this is the only one testing a regex pattern against format: regex.
It shouldn't be doing any validation at all, in this context. The 2020-12 meta-schema uses the format-annotation vocabulary, not format-assertion - this doesn't mean it can't do validation, per:

Implementations MAY still treat "format" as an assertion ... The implementation MUST provide options to enable and disable such evaluation and MUST be disabled by default. (ref)

But there's nothing about this test that indicates that a non-default option to enable validation should be set. The tests in optional/format/regex.json (which also rely on validation behavior) also don't have any indication of that - which I think is another issue that warrants correction, possibly as part of https://github.com/json-schema-org/JSON-Schema-Test-Suite/pull/708 - but at least they all consistently expect format validation, and this test having the same expectation makes sense in that context.

Discussed a few days ago with @karenetheridge on slack, for reference, though I think I've made the same points here. https://json-schema.slack.com/archives/C8CQ81GKF/p1713142486362639

gregsdennis commented 2 months ago

there's no reason the meta-schema needs to be involved in this

We've recently decided that meta-schema tests are not really in scope for this suite.

validating the pattern "\\a" directly against a "format": "regex" schema has the same effect [and related]

The spec provides allowance to validate format, however the main portion of this suite is testing the requirements of the spec. Optional behaviors, which includes impromptu validation of format, need to go in the optional/ folder until we decide to merge that other break-out PR.

What I would be open to is creating a set of required format tests where the schemas have a custom dialect that includes the format-assertion vocab. However, even this has some variability in support: the spec says that implementations are to provide a "best effort" validation. (I stated it that way on purpose to alleviate the burden from maintainers.)

I agree with the changes proposed by this PR.

Julian commented 2 months ago

Those sound like good reasons to fix/modify the test, and not good reasons to move the test to me, but do as y'all wish.

notEthan commented 2 months ago

This does modify the test, as well as moving it to a more appropriate location. I'm not sure what else would be more of an improvement.

Julian commented 2 months ago

It moves it to a strictly worse location, because it moves it from a file that is "tests for ECMA dialect regexes" to one that is not and is meant to be run across multiple possible regular expression dialects.

notEthan commented 2 months ago

I can leave the test where it is - that part doesn't matter to my test harness, anyway - but do you agree that testing with "data": "\\a" against a schema with "format": "regex" - dropping the meta-schema in between - is correct?

I do still think it has more in common with optional/format/regex.json than optional/ecmascript-regex.json but you didn't really respond to the reasons I said - that's fine, if the test being \a's role in ECMA dialect outweighs what I said, I'm good accepting that. Let me know if you agree with the change to the test content and I will move it back where it was.

Julian commented 2 months ago

but do you agree that testing with "data": "\a" against a schema with "format": "regex" - dropping the meta-schema in between - is correct?

Yes.

but you didn't really respond to the reasons I said

Happy to do this, but then can you point me at which ones you mean here, I'm happy to respond, but none of the ones I saw had anything to do with where the test was located, which is exactly why I commented with what I did.

Let me know if you agree with the change to the test content and I will move it back where it was.

Yes I agree.

Julian commented 2 months ago

I mean I guess just to directly comment on each one since those are the only ones here:

It's validating a property of a schema (pattern) against the meta-schema (specifying "format": "regex") but there's no reason the meta-schema needs to be involved in this, validating the pattern "\a" directly against a "format": "regex" schema has the same effect.

This certainly doesn't seem to have anything to do with which file it belongs in to me.

The rest of the tests in ecmascript-regex.json are testing strings against a pattern, this is the only one testing a regex pattern against format: regex.

This seems like we simply haven't had any tests of an invalid regex, and now we do.

It shouldn't be doing any validation at all, in this context. The 2020-12 meta-schema uses the format-annotation vocabulary, not format-assertion - this doesn't mean it can't do validation, per:

This again seems to have nothing to do with what file it's in and simply is commenting on the test being wrong in 2020.

But there's nothing about this test that indicates that a non-default option to enable validation should be set. The tests in optional/format/regex.json (which also rely on validation behavior) also don't have any indication of that - which I think is another issue that warrants correction, possibly as part of https://github.com/json-schema-org/JSON-Schema-Test-Suite/pull/708 - but at least they all consistently expect format validation, and this test having the same expectation makes sense in that context.

(This again doesn't have to do with what file the test is in though since you mention it, this is precisely why this test was fine in versions pre-2019 -- and why it's really a simple documentation issue for 2019 + 2020 at which point they'd be valid even where they are. But I'm honestly tired of that discussion, which is why it's easier to just say "sure change it".)

notEthan commented 1 month ago

Thanks, Julian. I moved it back and rebased.

Julian commented 1 month ago

I didn't do anything but quibble really, no need to thank me :) but this lgtm now too.

notEthan commented 1 month ago

I think you're right that it was fine where it is, and appreciate the time you took saying why.

Julian commented 1 month ago

Ok I'm gonna merge given it looks like others were already ok with this but if for whatever reason anyone thinks it was better before or something we can address.

Thanks again!

karenetheridge commented 1 month ago

I just came across this change again while testing my implementation.

I will repeat my objections earlier, that the location of this test is wrong: it is using the format keyword as an assertion, but neither specifying the use of the format-assertion vocabulary using a custom $schema URI, nor is the test under optional/format which is where all the other "treat the format keyword as an assertion" tests live.

We've repeatedly acknowledged that the use of optional/ is a bit vague for specifying different expected behaviour, but the convention is that format-assertion tests should be in optional/format. Deviating from this requires that an implementation's test suite needs to special-case other files under optional/ (or even specific tests in those files) in a way that they haven't before.

This convention is documented: see item 2 at https://github.com/json-schema-org/JSON-Schema-Test-Suite/blob/main/README.md#additional-assumptions.

gregsdennis commented 1 month ago

@karenetheridge I'm confused. You're arguing that the tests should be in optional/ because we've decided on a convention that asserting format is optional behavior (barring a custom meta-schema). But the final result of the PR is that the tests are in optional/.

What are you objecting to?

big-andy-coates commented 3 weeks ago

What are you objecting to?

According to item 2 at https://github.com/json-schema-org/JSON-Schema-Test-Suite/blob/main/README.md#additional-assumptions, only tests under optional/format should have format validation turned on. So having the test under optional doesn't help.

How about: https://github.com/json-schema-org/JSON-Schema-Test-Suite/pull/751

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

Move test of \a regex character to optional/format/regex #740