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

Feature Request: support for partial JSON matching #20

Open
pauloavelar opened this issue Dec 27, 2019 · 6 comments · May be fixed by #25
Open

Feature Request: support for partial JSON matching #20

pauloavelar opened this issue Dec 27, 2019 · 6 comments · May be fixed by #25
Labels
enhancement New feature or request

Comments

@pauloavelar
Copy link

Hello. First of all, great package!
It really reduces boilerplate code in tests and makes everything more readable.

I have a feature request/consideration I hope would get included to this project and I may help implement it if that's something that makes sense for this library: Partial JSON matching.

It can be behind a flag or in another method, but it basically means a JSON with 4 properties, when checked against a JSON with 2 properties, would not return the following error:

expected 2 keys at '$' but got 4 keys
unexpected object key(s) ["status","cause"] found at '$'

More often than not, we need to check for the overall structure and main fields and not the whole thing, and having to check for presence of every single field can take a lot of time.

Is this something that makes sense for you as the author of this library?
As much as I like creating my own fork, I think it's best for the community if we keep things centralized.

Thanks.

@kinbiko
Copy link
Owner

kinbiko commented Dec 31, 2019

Hi @pauloavelar,

Thanks for your kind words, and your interest in the package.

I think your proposal makes sense, and it's something I've been thinking of implementing myself. I'm probably going to be really busy for the next couple of months, but if you have the opportunity to, please feel free to implement it, and I'll make time to review it.

I'm thinking perhaps the following method can be implemented:

func (a *Asserter) AssertPartialf(path, actualJSON, expectedJSON string, fmtArgs ...interface{})

where path is the JSON-path. I think as long as it can handle a path like $.data.issues[3].name that will do the trick.
We would also have to implement validation against the path parameter:

  • must start with $
  • must not contain double quotes or other illegal runes.

etc.

If find you don't have the time either, I might get back to it later.
If you do find the time, please have a look at the developer guide I whipped up a long time ago. I think it's still accurate. I'll be sure to review it ASAP if you do raise a PR that passes the build.

How does that sound?

@pauloavelar
Copy link
Author

Deal. I read the developer guide and it makes sense.
While I am not sure if I will have the time to get it done myself, I might as well try and see how it goes.

As for the function signature, it looks good as a check for a single property, which I think is also very useful. I had a different approach in mind (below) and depending on how it goes I will try to get both working.

func (a *Asserter) AssertContainsf(actualJSON, expectedJSON string, fmtArgs ...interface{})

The variable names perhaps could change to be clearer as to what they represent, but the idea is asserting that the expectedJSON is contained in the actualJSON, something along the lines of Spring's ContentResultMatchers.

@kinbiko
Copy link
Owner

kinbiko commented Jan 4, 2020

If I understand correctly, that sounds like it would be a more powerful solution than my initial suggestion, and therefore better.

My understanding is something like this:

ja.AssertContainsf(`{"a": 1, "b": 2}`, `{"b": 2}`) // passes as everything in the expected JSON matches
ja.AssertContainsf(`{"a": 1, "b": 2}`, `{"c": 3}`) // fails as the actual JSON has no "c" property

Could you clarify how it would work with arrays?
I'm not sure where to draw the line If the expected JSON contains an array with a single element, and the actual JSON contains multiple elements. Is it a case of:

  1. "does the element in the expected JSON appear anywhere in the array?"

or is it more strict?

  1. "does the element in the expected JSON appear at index 0 of the actual JSON?"

In the case of 1, partial assertions cannot be used if the order matters -- and this is invisible to a reader of the code that has not read the documentation of AssertContainsf (which presumably would mention that the order doesn't matter in arrays), leading to tests passing when they should be failing.

In the case of 2, we'd have to know exactly where in an array the expected data is. We could do something similar to "<<PRESENCE>>" (e.g. "<<UNORDERED>>") as a 0th-element-visual-cue for readers of the test code to indicate that we're ignoring the ordering of this particular array.

Out of these two, I much prefer the second option, but I'm guessing there's probably a better solution that I don't see just yet.

Perhaps you have an idea @pauloavelar? Do you know how Spring does it?

@pauloavelar
Copy link
Author

I did not know how Spring handles arrays because it is not an assertion I often do.

When handling collections I usually deserialize the JSON into a model, iterate and assert its properties. Things can get verbose, but they are usually clearer that way.

That said, I have prepared a small example of how Spring's ResultMatchers work if you want to check it out.
Apparently they don't do any partial matching with arrays and completely disregard ordering.

Interesting notes:

  • There is a flag for strict checking which makes the actual JSON not extensible.
  • There is also support for a jsonPath matcher that supports something similar to your idea.
  • It seems Spring uses a library called JSONassert for these matchers.

@kinbiko
Copy link
Owner

kinbiko commented Jan 10, 2020

Thank you @pauloavelar for doing this research and getting back to me. I'm not convinced I agree with the way Spring does things in this regard.

I'm starting work on this feature, following the approach I proposed in my previous comment (case 2.).
I'll raise a PR, at the earliest next week (but probably later), with a proposed approach. Just to be clear, I'm implementing it to try it out in other projects I'm working on to verify the design. I might not like what I write, and end up dropping it.

If you have the time and once/if I'm personally happy with the solution I would love your opinion on it as well, seeing as you've contributed a great deal to designing this feature.
If I end up scrapping it, I'll let you know.

@kinbiko kinbiko added the enhancement New feature or request label May 5, 2020
@kinbiko
Copy link
Owner

kinbiko commented May 5, 2020

Just a quick update: I ended up doing a bit of refactoring (currently in master, not released) that would make this change easier and I've started working on the implement-assertf branch, but life has gotten in the way of me progressing this beyond that.
If anyone wants to help that'd be most welcome, otherwise I'll get around it eventually.

Tl;dr: Not scrapped, but also not expecting progress in the next few months.

@kinbiko kinbiko linked a pull request Mar 7, 2021 that will close this issue
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: In Progress
Development

Successfully merging a pull request may close this issue.

2 participants