Skip to content

Conversation

@414owen
Copy link

@414owen 414owen commented Nov 11, 2025

Unless I'm missing something, it looks like there's an additional property in the with no additional properties test case of the unevaluatedProperties with adjacent bool additionalProperties suite.

The case with additional properties is the next one.

@414owen 414owen requested a review from a team as a code owner November 11, 2025 15:19
Copy link
Member

@jdesrosiers jdesrosiers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for catching that! Can you make this change in the 2019-09 and v1 tests as well.

@414owen
Copy link
Author

414owen commented Nov 11, 2025

I think this is the only draft with this test?

$ command grep -nire 'unevaluatedProperties with adjacent bool additionalProperties' 
tests/draft2020-12/unevaluatedProperties.json:130:        "description": "unevaluatedProperties with adjacent bool additionalProperties",

It looks like 2019 has unevaluatedProperties with adjacent additionalProperties, and 2020 has the same with bool and non-bool additionalProperties. The other tests look fine.

@jdesrosiers
Copy link
Member

Oh, that's weird. There's no reason there should be different tests in those three versions. It looks like someone broke that test in 2020-12, but didn't make the change in the other versions.

We need to make all three the same. I think the best approach is to use the 2019-09 version of the schema/tests and the 2020-12 version of the description. Then we need to copy the "unevaluatedProperties with adjacent non-bool additionalProperties" test to 2019-09 and v1.

This fixes the disparaty between then 2019, 2020, and v1 drafts,
and adds the property constraint back into the 2020 schema, so
that the first test case matches its description.
@414owen 414owen force-pushed the os/fix-unevaluatedproperties-test-case branch from 6b8885f to e0e431f Compare November 12, 2025 14:25
@414owen
Copy link
Author

414owen commented Nov 12, 2025

@jdesrosiers Alright, I've updated all three drafts to match, with the bool and non-bool versions.

Note that the non-bool version doesn't actually test anything about the non-bool nature of the additionalProperties constraint.

Copy link
Member

@jdesrosiers jdesrosiers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I wasn't expecting you to change the non-bool test, just copy it over to the other versions. But, this way is better. The test for an invalid additional property in the original version was testing additionalProperties, not unevaluatedProperties.

Thanks for helping clean this up. Since we're changing an existing test, I'm going to leave this open for a little while to give more people a chance to review.

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

Successfully merging this pull request may close these issues.

2 participants