Skip to content

description of additionalItems test case is insufficient / misleading #768

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

Open
gregsdennis opened this issue Apr 8, 2025 · 3 comments
Open

Comments

@gregsdennis
Copy link
Member

Test:

{
"description": "additionalItems does not look in applicators, valid case",
"schema": {
"allOf": [
{ "items": [ { "type": "integer" } ] }
],
"additionalItems": { "type": "boolean" }
},
"tests": [
{
"description": "items defined in allOf are not examined",
"data": [ 1, null ],
"valid": true
}
]
},

While it's true that additionalItems can't see into applicators, the reason this passes validation is actually because the default value for items is an empty schema, which evaluates all items, leaving none for additionalItems to operate on.

We already have a test for default items, though, which kind of makes this one redundant.

We also have an invalid test immediately below the test in question that could easily have a valid test data case added to it. This test properly verifies that the additionalItems is failing on the second element.

@jdesrosiers
Copy link
Member

Looks like that discussion was had at the time, but it was merged anyway #344. After thinking about it for a while, I don't think there's a way to write this test that makes sense.

@gregsdennis
Copy link
Member Author

I don't think there's a way to write this test that makes sense.

As I mentioned, the test that immediately follows it has a "failing validation" case, and it's pretty easy to add a "passing validation" case to that test. Then we just remove this one.

@jdesrosiers
Copy link
Member

The only way to make a passing test is a one element array, but then it's not testing additionalItems in any way. For the second item in the array, additionalItems is saying it must be a boolean and /allOf/items/1 is saying it must be a string. There's no value that can be a string and a boolean at the same time, so there is no meaningful passing test for this case.

I think just removing the ", valid case" test is the right move. I think the ", invalid case" test effectively covers what it intends to cover without a passing test even if that was possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants