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

chore: add test cases to demonstrate multipleOf errors #398

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

DanielBauman88
Copy link
Contributor

@codecov
Copy link

codecov bot commented Nov 21, 2022

Codecov Report

Merging #398 (5f0f64c) into master (002edce) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #398   +/-   ##
=======================================
  Coverage   79.52%   79.52%           
=======================================
  Files          52       52           
  Lines        4455     4455           
=======================================
  Hits         3543     3543           
  Misses        912      912           
Impacted Files Coverage Δ
jsonschema/src/keywords/multiple_of.rs 81.03% <ø> (ø)
jsonschema/src/error.rs 57.71% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@Stranger6667
Copy link
Owner

Hm, the test is passing

@DanielBauman88
Copy link
Contributor Author

DanielBauman88 commented Nov 22, 2022

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

@Stranger6667
Copy link
Owner

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?

@DanielBauman88
Copy link
Contributor Author

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.

@DanielBauman88
Copy link
Contributor Author

Also, maybe this test could also be added to the upstream

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).

@Stranger6667
Copy link
Owner

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?

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 :)

@Stranger6667
Copy link
Owner

Relevant discussion - json-schema-org/JSON-Schema-Test-Suite#413

@DanielBauman88
Copy link
Contributor Author

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.

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