draft-04/06: items/additionalItems tests #161
Conversation
tests/draft4/additionalItems.json
Outdated
| @@ -40,6 +40,11 @@ | |||
| }, | |||
| "tests": [ | |||
| { | |||
| "description": "fewer items is valid", | |||
There was a problem hiding this comment.
The spirit of this is redundant with the test below it for "no additional items present", but if you feel like it's worth having separate tests for "less than" than "equal" it might be nice to reword the descriptions in that way specifically.
There was a problem hiding this comment.
The test below tests array with the same number of items. This tests for array with FEWER items. While it should pass and may seem redundant, some validators fail this test.
There was a problem hiding this comment.
@Julian could you please suggest the description?
There was a problem hiding this comment.
Maybe "fewer number of items present" and "equal number of items present" is good enough.
It's mostly the generality of the description below that seems out of place to me now, since it says "no additional items present" but that applies mostly equally to this test case.
tests/draft4/items.json
Outdated
| @@ -19,6 +19,14 @@ | |||
| "description": "ignores non-arrays", | |||
| "data": {"foo" : "bar"}, | |||
| "valid": true | |||
| }, | |||
| { | |||
| "description": "not array is valid", | |||
There was a problem hiding this comment.
This one is redundant with "ignores non-arrays" above it for what I can tell, unless you're testing something different here, but not seeing it?
tests/draft4/items.json
Outdated
| @@ -19,6 +19,14 @@ | |||
| "description": "ignores non-arrays", | |||
| "data": {"foo" : "bar"}, | |||
| "valid": true | |||
| }, | |||
| { | |||
| "description": "not array is valid", | |||
There was a problem hiding this comment.
That is relevant for Javascript validators (almost half of all validators) that do not differentiate between arrays and pseudo-array (objects that look like and can be iterated as arrays). Objects should pass even if a similar array would fail. Quite a few validators fail this test.
There was a problem hiding this comment.
Got it, can you update the description to mention that specifically? This seems even more likely to be JS-specific given the presence of the "length" key, so maybe this one should specifically say something like "Javascript pseudo-arrays are not valid", and it seems likely that that edge case belongs elsewhere as well then possibly.
And if we're already going there, maybe having integer-keyed maps entirely is another reasonable test (i.e. the same as what you have, just without the length key.).
tests/draft4/items.json
Outdated
| @@ -55,6 +63,15 @@ | |||
| "description": "empty array", | |||
| "data": [ ], | |||
| "valid": true | |||
| }, | |||
| { | |||
| "description": "not array is valid", | |||
There was a problem hiding this comment.
see comment above about pseudo arrays
|
Perhaps this test should go in optional/ then?
I've got mixed feelings on how many tests we should be writing for the
specific purpose of breaking common implementations, instead a mere
negative case.
…On Mon, Feb 6, 2017 at 2:02 AM, Evgeny Poberezkin ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In tests/draft4/items.json
<#161>:
> @@ -19,6 +19,14 @@
"description": "ignores non-arrays",
"data": {"foo" : "bar"},
"valid": true
+ },
+ {
+ "description": "not array is valid",
That is relevant for Javascript validators (almost half of all validators)
that do not differentiate between arrays and pseudo-array (objects that
look like arrays). Objects should pass even if array would fail. Quite a
few validators fail this test.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#161>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAatDdtzMLj9-4od4UFs8hO0bSOqfBaYks5rZuG6gaJpZM4Lwkf9>
.
|
|
@awwright think it got lost which test you're specifically referring to here with that comment |
|
@Julian thank you for the feedback, I've pushed the changes. |
|
@awwright are you ok with that? I don't think pseudo-array tests should be in optional because even though they specifically target JavaScript validators, these tests MUST pass for all validators, irrespective of the language, so they are not really "optional". Currently in this folder we have tests that validators SHOULD (or MAY?) pass, not MUST pass. |
No description provided.