Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a specification property to test cases + tests #699

Open
Julian opened this issue Oct 31, 2023 · 12 comments
Open

Add a specification property to test cases + tests #699

Julian opened this issue Oct 31, 2023 · 12 comments
Labels
enhancement An enhancement to the tooling or structure of the suite (as opposed to a new test). good first issue An issue that is a good candidate for new contributors

Comments

@Julian
Copy link
Member

Julian commented Oct 31, 2023

There isn't a fixed spot in tests right now which indicates which portion, section, or text of the specification it covers. The suite's main purpose is of course to faithfully represent the specification, so essentially all tests should have some direct basis in the specification, but we leave that for someone interested to have to re-locate, even though this work should have been done before adding each individual test (by at least 2 people presumably -- the test author as well as the reviewer). Right now we occasionally use comments to indicate this information, but having a specific spot for it could open up possibilities, but we should earmark a spot in each test where we include this information.

Considerations:

  • Not all portions of the specification are "deeply structured" -- meaning the best we may be able to say about a test is "it's in section 8.2" but that may have text which covers 20 or more tests because it is intricate. It seems here we should simply do the best we can, perhaps filing issues upstream in the spec when encountering particularly deficient structure.
  • The specification, even within a specific version, is occasionally republished. This may cause section information to go out of date.
  • There are other specifications besides the core JSON Schema specification which we reference. While historically we tried to stay away from becoming a full test suite for specifications beyond JSON Schema (even if JSON Schema links or relies on them), there are places where we indeed test lots of edge cases (e.g. leap second tests). We should choose some mechanism which allows us to represent this information as well -- i.e. for such tests, ideally we'd represent both: 1) where in the JSON Schema specification does it say to follow some other specification or RFC, 2) where in that other RFC is this behavior specified
  • Think about what the schema of this property should look like -- more structured is better, as it will allow for "unforeseen" uses -- e.g. us being able to calculate what fraction of the specification we cover with the suite itself.
  • The test suite schema will need to be modified to include this key. It will likely need to be and stay optional, but we should attempt to add it everywhere we can.
  • Please do not send a massive PR doing this for the whole suite, but rather send small, perhaps single file PRs to add it to tests.
@Julian Julian added enhancement An enhancement to the tooling or structure of the suite (as opposed to a new test). good first issue An issue that is a good candidate for new contributors labels Oct 31, 2023
Julian added a commit to python-jsonschema/referencing-suite that referenced this issue Nov 8, 2023
@Era-cell
Copy link
Contributor

Era-cell commented Apr 2, 2024

Hi @Julian I am interested, and I think this will useful for upcoming project. I have some doubts:
For cases like escaped pointer:

"description": "refs with quote",

Where should the re-location be done?

and for cases like $ref to else :

"description": "ref to else",

Should we point the location for both ref and else?

@gregsdennis
Copy link
Member

Where should the re-location be done?

"Re-locate" here means "locate again;" as opposed to "relocate" which means "move". English is dumb.

The point is that the reader of the test has to find the requirement in the specification text, when we would have known where the requirement was when writing the test and could have just pointed to it from the beginning.

@Era-cell
Copy link
Contributor

Era-cell commented Apr 2, 2024

Where should the re-location be done?

"Re-locate" here means "locate again;" as opposed to "relocate" which means "move". English is dumb.

The point is that the reader of the test has to find the requirement in the specification text, when we would have known where the requirement was when writing the test and could have just pointed to it from the beginning.

I intended to say where should we point to, for the first case. If I had used the word "refer to" that would have caused ambiguity with $ref. So😅
And yeah, we can use the urls of the spec and rfc right? Inorder to point the location where particular test case belongs to...

Eg,

{
"specification":{
    "jsonschema spec":"https://json-schema.org/draft/2020-12/json-schema-core#name-direct-references-with-ref",
    "rfc spec":"url for rfc spec location"
    }
}

@Era-cell
Copy link
Contributor

Era-cell commented Apr 3, 2024

@Julian @gregsdennis I have some doubts:

  • "Right now we occasionally use comments to indicate this information" - can I get any usage example of these $comments.
  • can the relocation be done to the "reference" which contains examples and explanation OR its just the spec where the re-location to be done
  • Can the re-location be done to an example fit for showing the test-case, but the example is meant for other understanding. Ex:https://json-schema.org/draft/2020-12/draft-bhutton-json-schema-01#section-12.4-4 this is meant for "output" format, but has additionalProperties: false
    help me in one file enhancement .. 🙏

@Julian
Copy link
Member Author

Julian commented Apr 4, 2024

can I get any usage example of these $comments.

I'm afraid I don't understand your other questions, particularly what you mean by "relocation".

The spot that goes there should be the "canonical place in the specification which defines the behavior". If there doesn't seem to be one, that should be raised as an issue.

@Era-cell
Copy link
Contributor

Era-cell commented Apr 4, 2024

Hey! I previously thought that we should be using URLs which will be redirecting to location where test case belongs.
Canonical place is good, I understood that.
I'll just reframe unclear questions:

  • Can I provide the canonical location for an example which belongs to an other section - like ("output" format ) section.
    For the "additionalProperties" test - case.
  • Can I provide a location in the "Reference" / Specification/ any Release Note. which also explain some cases.
  • And for the test case which has explanation in RFC but not in jsonschema spec(also no explicit mention of that RFC in jsonschema spec) will be considered to raise an issue?

@Julian
Copy link
Member Author

Julian commented Apr 4, 2024

Can I provide the canonical location for an example which belongs to an other section - like ("output" format ) section.
For the "additionalProperties" test - case.

No, an example is never a canonical location for something. "Canonical" means "the authoritative place where it is defined". The specification never uses an example to define something, an example is simply to illustrate something.

And for the test case which has explanation in RFC but not in jsonschema spec(also no explicit mention of that RFC in jsonschema spec) will be considered to raise an issue?

No, it's expected that those are defined in their respective RFCs. The JSON Schema spec will however contain a spot where it mentions which RFC it is referencing, and that's interesting to include.

@Era-cell
Copy link
Contributor

Era-cell commented Apr 4, 2024

No, it's expected that those are defined in their respective RFCs. The JSON Schema spec will however contain a spot where it mentions which RFC it is referencing, and that's interesting to include.

Actually for the test-case: "additionalProperties with non-ascii characters"
jsonschema spec doesn't say about property names.
But rfc 8259 tells about object names for any json document. In that case--> should I give the reference to that RFC or should I raise an issue?

@Julian
Copy link
Member Author

Julian commented Apr 4, 2024

Linking to simply the definition of additionalProperties (i.e. https://json-schema.org/draft/2020-12/json-schema-core#additionalProperties) seems like the right thing there, as there's no special behavior whatsoever about non-ASCII characters. Yes you're right the JSON spec does say that non-ASCII characters are allowed, but given that every single test in the repo will also somehow depend on the JSON Spec it seems like quite a bit of work to link to the JSON RFC, work that I don't think seems very useful at first glance.

@Era-cell
Copy link
Contributor

Era-cell commented Apr 4, 2024

Linking to simply the definition of additionalProperties (i.e. https://json-schema.org/draft/2020-12/json-schema-core#additionalProperties) seems like the right thing there, as there's no special behavior whatsoever about non-ASCII characters. Yes you're right the JSON spec does say that non-ASCII characters are allowed, but given that every single test in the repo will also somehow depend on the JSON Spec it seems like quite a bit of work to link to the JSON RFC, work that I don't think seems very useful at first glance.

Thanks @Julian , this task should be much easier then.
Can I be assigned or just make PRs? I you can review my updated PR that would be great

@Julian
Copy link
Member Author

Julian commented Apr 4, 2024

Just sending PRs I think is fine, as just one PR is certainly not going to cover everything, so presumably more than one person can work on it at once. I'll have a look, though I saw both Greg and Karen had left some comments there, their review is certainly as good as mine, but I'll have a look as well.

@Julian
Copy link
Member Author

Julian commented Apr 6, 2024

This work was started in #726 so we now have an initial schema here to work on top of!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An enhancement to the tooling or structure of the suite (as opposed to a new test). good first issue An issue that is a good candidate for new contributors
Projects
None yet
Development

No branches or pull requests

3 participants