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

Add property based test. #63

Closed
wants to merge 6 commits into from

Conversation

mithrandi
Copy link
Contributor

This PR adds a property-based test using Hypothesis, that tests that a patch from a randomly generated source value to a randomly generated destination value is valid (yields the destination value when applied to the source value). This would have been able to find the bug in #55, among other things.

The test is currently failing due to an issue with jsonpointer that I'm not entirely sure how to fix. The first problem is that jsonpointer expects pointers to be urlencoded, which I don't think is correct (my reading of the RFC is that urlencoding should only be used when the pointer is part of a URL, so nothing special to JSON pointer itself). The second issue is that JsonPointer.from_parts([u'%00']) ends up generating a pointer of /\u0000 due to the parts being urldecoded; this is tangled up with the first issue.

I'm happy to do the work to resolve these issues, but I'm not entirely sure what direction to go in, so would appreciate some guidance here. I think the correct thing for jsonpointer to do is to not do any url encoding/decoding at all, but of course this represents a fairly large change in the API.

JSON doesn't actually allow these (despite the json module in Python happily
serializing / parsing 'nan'), and they make assertEqual fail since they're not
equal to anything, including themselves.
@coveralls
Copy link

coveralls commented Jul 6, 2017

Coverage Status

Coverage decreased (-74.8%) to 21.662% when pulling 9758934 on mithrandi:property-tests into 18887df on stefankoegl:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-74.8%) to 21.662% when pulling 9758934 on mithrandi:property-tests into 18887df on stefankoegl:master.

@coveralls
Copy link

coveralls commented Jul 6, 2017

Coverage Status

Coverage decreased (-74.8%) to 21.662% when pulling 24d3196 on mithrandi:property-tests into 18887df on stefankoegl:master.

@coveralls
Copy link

coveralls commented Jul 6, 2017

Coverage Status

Coverage remained the same at 96.439% when pulling 313e1b3 on mithrandi:property-tests into 18887df on stefankoegl:master.

@coveralls
Copy link

coveralls commented Jul 6, 2017

Coverage Status

Coverage remained the same at 96.439% when pulling 49b6c74 on mithrandi:property-tests into 18887df on stefankoegl:master.

@coveralls
Copy link

coveralls commented Jul 6, 2017

Coverage Status

Coverage remained the same at 96.439% when pulling 6cc61b1 on mithrandi:property-tests into 18887df on stefankoegl:master.

@coveralls
Copy link

coveralls commented Jul 6, 2017

Coverage Status

Coverage remained the same at 96.439% when pulling 6f10715 on mithrandi:property-tests into 18887df on stefankoegl:master.

@stefankoegl
Copy link
Owner

stefankoegl commented Jul 9, 2017

Thanks for your work! Let me address the issues that you raised:

The first problem is that jsonpointer expects pointers to be urlencoded, which I don't think is correct
(my reading of the RFC is that urlencoding should only be used when the pointer is part of a URL, so
nothing special to JSON pointer itself).

I agree that rfc6901 does not mention anything about urlencoding, so jsonpointer should not expect it. However I'm not sure if I understand you correctly... in jsonpoiner I can do the following (with a non-urlencoded pointer):

>>> import jsonpointer
>>> obj = {'a b': 1}
>>> jsonpointer.resolve_pointer(obj, '/a b')
1

Could you please open an issue in stefankeogl/pyhon-json-pointer to describe the issue with an example (or isolated, failing test)?

@stefankoegl
Copy link
Owner

stefankoegl commented Jul 9, 2017

OK, I think I understand your point... rather than the example above, the failing example should be

>>> import jsonpointer
>>> obj = {'a b': 1}
>>> jsonpointer.resolve_pointer(obj, '/a%20b')
1

Instead this should give an error because there is no member a%20b.

It seems that I added the unquoting (currently line 145) already in the very first commit. Initially the [RFC draft|https://tools.ietf.org/html/draft-pbryan-zyp-json-pointer-00] did contain

Property names SHOULD be URI encoded per [RFC2396]. In particular,
any "/" character in a property name MUST be encoded as "%2F" to
avoid being interpreted as a property reference token separator.

but this was removed before the final RFC, and is thus no longer correct.

Would you be interested in submitting a pull request for jsonpointer to fix this issue?

@mithrandi
Copy link
Contributor Author

Yup, your example is correct. I'll try to send a PR to jsonpointer this week sometime 😃

@stefankoegl
Copy link
Owner

I have now added an issue at stefankoegl/python-json-pointer#22.

@stefankoegl
Copy link
Owner

@mithrandi, do you still intend to work on this pull request? The tests are failing for most python versions, but the general idea seems interesting.

@stefankoegl
Copy link
Owner

I will continue the work in #79.

@mithrandi
Copy link
Contributor Author

Sorry, guess I never got back to this! Thanks for picking it up.

@mithrandi
Copy link
Contributor Author

Superseded by #79 I guess.

@mithrandi mithrandi closed this Jul 2, 2018
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.

None yet

3 participants