-
-
Notifications
You must be signed in to change notification settings - Fork 93
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
chore: add test cases to demonstrate multipleOf errors #398
base: master
Are you sure you want to change the base?
chore: add test cases to demonstrate multipleOf errors #398
Conversation
Codecov Report
@@ Coverage Diff @@
## master #398 +/- ##
=======================================
Coverage 79.52% 79.52%
=======================================
Files 52 52
Lines 4455 4455
=======================================
Hits 3543 3543
Misses 912 912
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Hm, the test is passing |
It's written to pass so that it can be committed - but those tests logically should fail. edit: in other words when the bug is fixed the new tests should start failing as confirmation that the behaviour is correct then line 179 can be changed to is_not_valid as it should be and line 183 can be changed to is_valid as it should be. https://www.wolframalpha.com/input?key=&i=1e-15%2F1e-16 <- is a multiple https://www.wolframalpha.com/input?key=&i=1.3e-16%2F1.3 <- is not a multiple |
Oh, thank you - I completely overlooked it :) I prefer to have this test failing though - so we know that something is not yet properly done. On the other hand, I don't want to overcomplicate things. Maybe, this PR could be a base for porting that implementation from Monako & when it's done we'll have the fixed behavior + regression tests. Also, maybe this test could also be added to the upstream JSON Schema Test Suite? |
I could make it fail, but won't that be annoying if every subsequent unrelated PR has failing tests because of this? Would you like me to update this PR with the test that fails? Could also just cancel this PR, but I think having something making this behaviour clear in the code is nice to have. |
That's a good idea, it will definitely start an interesting conversation. My guess due to the absence of difficult cases in their suite is that the jsonschema people aren't deliberately avoiding dealing with these cases. I would love for them to take a stand on what they believe is correct behaviour (floating point results vs decimal results). |
I was thinking to keep this PR open with failing test cases and then push a fix here as well, and then merge :) What do you think about such a workflow? I'll open an issue / PR there about this :) |
Relevant discussion - json-schema-org/JSON-Schema-Test-Suite#413 |
Yeah, I also started this thread on the slack channel - https://json-schema.slack.com/archives/C5CF75URH/p1669273565065379 For now, the spec doesn't say anything helpful, but for this library we can still choose to do what we think is best for the consumers of this library. I think for most people that is to match decimal results as much as possible. |
c8ee78a
to
8eacf2d
Compare
#397