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

level by level diff of unequal nested data structures. srsly. #54

Closed
dchelimsky opened this issue Dec 27, 2010 · 29 comments
Closed

level by level diff of unequal nested data structures. srsly. #54

dchelimsky opened this issue Dec 27, 2010 · 29 comments

Comments

@dchelimsky
Copy link
Contributor

Per http://twitter.com/mcmire/status/19288568174219266.

@dchelimsky
Copy link
Contributor Author

This sort of thing is easy to do when the nested data types match, but what about something like this:

expected = [
  { "person" => { "name" => "A", :friends => [] } },
  { "person" => { "name" => "B",
    :friends => [ "A" ]
  }
]

actual = [
  { "person" => { "name" => "A", :friends => [] } },
  { "person" => { "name" => "B",
    :friends => [ { "person" => { "name" => "A", :friends => [] } } ]
  }
]

How should the diff be represented here, such that it leads the reader directly to the problem?

There are many more questions I have about this, but let's start here.

@mcmire
Copy link

mcmire commented Dec 27, 2010

Yeah, I knew I was opening a can of worms ;)

In this case I'd want the output to be something like this:

expected: [{"person"=>{"name"=>"A", :friends=>[]}}, {"person"=>{"name"=>"B", :friends=>["A"]}]
got: [{"person"=>{"name"=>"A", :friends=>[]}}, {"person"=>{"name"=>"B", :friends=>[{"person"=>{"name"=>"A", :friends=>[]}}]}]

Details:
- *[1]["person"][:friends][0] isn't equal
  - expected: "A"
  - got: {"person"=>{"name"=>"A", :friends=>[]}}

@mcmire
Copy link

mcmire commented Jan 18, 2011

Coming back to this, I started prototyping a diff tool. Right now it only handles simple types (strings, numbers) and arrays, but you should be able to get the idea: https://github.com/mcmire/super_diff

@dchelimsky
Copy link
Contributor Author

That looks pretty cool. rspec-1 let you configure the differ, but nobody ever used that feature, so I left it out of rspec-2. Rather than forcing users to use special matchers, it'd be cool if that got reinstated and all you needed to do was:

RSpec.configure {|c| c.diff_with :super_diff }

or some such. WDYT?

@dchelimsky
Copy link
Contributor Author

Actually, the gist ref'd in #9 does an interesting job diffing strings. Maybe the diffing control could be even more granular, where users could define differs for different types of objects?

@mcmire
Copy link

mcmire commented Jan 18, 2011

Ah, that gist is interesting. Yeah, a more modular per-class configuration is probably the route to go -- I can only handle Ruby types like Array and Hash, but this string differ is a good example, or another one is if you need to diff an OrderedHash (which is stored slightly differently than a hash, but the diff output needs to be exactly like one).

@JonRowe
Copy link
Member

JonRowe commented Mar 3, 2013

Whatever happened to this? Did you finish your diff tool @mcmire? Or has there been such a lack of interest that this can be closed now @dchelimsky?

@dchelimsky
Copy link
Contributor Author

@myronmarston any interest in this?

@mcmire
Copy link

mcmire commented Mar 3, 2013

I never finished my diff tool, although you can check it out here: http://github.com/mcmire/super_diff. There's a couple of reasons I stopped working on it: 1) it ended up being more complicated than I thought (all sorts of interesting use cases you have to handle) and 2) when you are diffing two complex data structures, doing a breakdown on the element level (which is the approach I took) gets a little verbose -- you end up having to mentally parse the output. I would welcome thoughts on my tool, though.

@myronmarston
Copy link
Member

I think having an improved differ in rspec-expectations (or support for selecting/injecting your own differ, as in #97) would be desirable, but this is way down my list of priorities and I don't know how soon (if ever) it would become a priority, particularly since there haven't been other users requesting that.

I'm on the fence about if this should be closed or not -- it's still a valid, useful request, but I don't know if any rspec committers will ever get around to it.

@mcmire
Copy link

mcmire commented Mar 4, 2013

FWIW, whatever you want to do is fine with me. I need to take another look at this but I don't really have the time right now.

@myronmarston
Copy link
Member

Let's close it then. I'd still be happy to merge a PR from a contributor improving on things here, but we don't need an open issue for that to happen.

@jfelchner
Copy link

@myronmarston I just found this and not being able to easily see the difference between large API responses that we're testing is a nightmare for us currently.

I'm definitely interested in taking a look at this.

If you just want to give me an idea of what you'd like the API to look like and which files I need to look at to get me started and then I'll try to take it the rest of the way.

@myronmarston
Copy link
Member

I think there are two routes we could take with the API:

  • The custom differ would be responsible for providing the actual full diff.
  • The custom differ would be more of an "inspector" that would convert the expected and actual data structures into some kind of line-by-line string form, and then RSpec would pass that to Diff:::LCS to have it do a line-by-line textual diff. Essentially, the "inspector" would be like Object#inspect except it would be diff aware and would produce string output that diffs well.

Which would you prefer?

@jfelchner
Copy link

@myronmarston I feel like Diff::LCS is a great diffing tool, so if we can find a solution that lets it do what it's good at, that that would be preferable.

The one major difficulty that I can see is the sub matchers and how those would be displayed.

Example:

expect(
  {
    a: match(/[fb]oo/),
    b: 'yo',
  }
).to eql(
  {
    a: 'foo',
    b: 'YO!',
  }
)

So that doesn't match, but we wouldn't want to diff of a to be shown. Only the b line doesn't match.

Although I'd love to just be able to give Diff::LCS a couple strings and rely on it to do its job, that may not be possible.

Thoughts?

@mcmire
Copy link

mcmire commented Jan 3, 2015

@jfelchner I never finished it, but check out the project I created for ideas and challenges: http://github.com/mcmire/super_diff

@jfelchner
Copy link

@mcmire cool I'll take a look. Any thoughts on the specific regex example I gave above? Where there are sub matchers involved?

@mcmire
Copy link

mcmire commented Jan 3, 2015

Well the output (I imagine) could look something like this:

Error: Hashes of same size but with differing elements.

Expected: {:a => match(/[fb]oo/)), :b => 'yo'}
Got: {:a => 'foo', :b => 'YO!'}

Details:
- *[:b]: Differing strings.
  - Expected: "yo"
  - Got: "YO!"

As far as actually handling submatchers, it's possible, you'd just have to bake it in somehow -- look for instances of RSpec::Matchers::BuiltIn::BaseMatcher, or something.

Also the "Expected/Got" above assumes that match returns an object whose #inspect method returns something like "match(regex)". It doesn't, but that could either be added, or you could recursively walk the given structure and decorate any instances of RSpec::Matchers::BuiltIn::Match with something that does implement #inspect (and same for other matchers).

Please note I made the gem as a standalone tool, though the eventual plan was to somehow integrate it better with RSpec. I didn't get that far so I don't have any ideas on that unfortunately.

@jfelchner
Copy link

Hmmmmm... I'll look more into the gem.

@mcmire
Copy link

mcmire commented Jan 3, 2015

Cool. And by the way I did plan on hooking it into Diff::LCS (I don't think it quite works at the moment though): https://github.com/mcmire/super_diff/blob/master/lib/super_diff/recursive_differ.rb

@bunyk
Copy link

bunyk commented Jun 13, 2018

While I don't know how to use your differ in our tests (I'm not a ruby dev), nicely formatted failures could be achieved by writing specs like:

requre 'json'
...
    expect(JSON.pretty_generate(actual)).to eq(JSON.pretty_generate(expected))
...

Just in case someone is looking for something like this.

@jponc
Copy link

jponc commented Dec 20, 2018

https://github.com/mcmire/super_diff

@mcmire it'll be great if this gets merged to rspec-expectations, good stuff man 👍

@mcmire
Copy link

mcmire commented Dec 21, 2018

@jponc Thanks! I started rewriting this recently as you can see. It works okay — right now I'm sort of testing it out on an existing app. Once I have it working well I'll focus on trying to integrate it with RSpec in a non-hacky way :)

@jfelchner
Copy link

If the option is there, I’d like to see this library be able to output a structure of only what is different between two hashes. Is that in the plans?

@mcmire
Copy link

mcmire commented Dec 23, 2018

@jfelchner Are you talking about a data structure that represents a diff or a string that represents a diff?

@jfelchner
Copy link

A data structure. So:

{
  foo: ‘bar’,
  baz: 2,
}

# vs

{
  foo: ‘baz’,
  baz: 2,
  wiz: ‘ard’
}

# would output something like:

{
  foo: ‘baz’,
  wiz: ‘ard’
}

@JonRowe
Copy link
Member

JonRowe commented Dec 24, 2018

@benoittgt has been working on a new differ for RSpec 4, I'm open to making this configurable in future (to our own, diff-lcs, or other) but we don't be including a 3rd party gem as a dependency from RSpec 4 onwards.

It's important to note that we want to favour the diff of complex structures by piece but in the senario above, the eventual formatted diff RSpec wants to show is:

{
-  foo: ‘bar’,
+  foo: ‘baz’,
   baz: 2,
+  wiz: 'ard'
}

@jfelchner
Copy link

jfelchner commented Dec 25, 2018

@JonRowe I was just talking about allowing this gem to be able to do this. Outside of usage in RSpec. Not that this would be the format I’d want to see in specs.

The format I’d want in a spec run is what you’re proposing.

@mcmire
Copy link

mcmire commented Dec 26, 2018

@jfelchner That's not one of the goals of the gem, but if you want to open an issue so you can provide more context and we can discuss it there then I'm all ears!

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

No branches or pull requests

7 participants