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

New differ #365

Open
benoittgt opened this issue Feb 6, 2019 · 12 comments
Open

New differ #365

benoittgt opened this issue Feb 6, 2019 · 12 comments
Labels

Comments

@benoittgt
Copy link
Member

benoittgt commented Feb 6, 2019

Hello

Few people express the lack of clear informations with the RSpec diff output. As expressed [1][2]. @myronmarston explained the issue with diff-lcs and why we should use a new matcher for RSpec dedicated to RSpec semantics.

A differ written for RSpec, that understands the semantics of RSpec matching, would produce far better output. - source

List of issues with current differ

Gems that try to improve the differ

Attempt tried


I planned long time ago to port the differ that is included with Elixir's ExUnit but didn't do it because of lack of time.

I am wondering what we are trying to do, what are the expectations. I have listed few issues with the current differ below. I think super-diff by @mcmire is a good start but the dependency should be dedicated to RSpec and deal with more types of objects.

I would love to work and get help on this subject for RSpec 4 but I need to know the expectations.
For example get rid of diff-lcs and create a new RSpec gem that will include diff mechanism plus a new ObjectFormater but only to output diff of expected VS actual?

At the moment it is difficult to change ObjectFormater.

The ObjectFormatter is currently used in several ways, to format objects for descriptions etc, and to format them for expected / actual [source]

My last attempt to improve only string diff was a failure because ObjectFormater is used in various place and I had many failing tests in different RSpec gems.

Maybe we should first create a new class that will include diff mechanism (start with diff-lcs) plus a new ObjectFormater for actual/expected diff? For the end user it will mean no changes but for us it will be easier to start to work on a better diff output for RSpec 4?

"for each desired change, make the change easy (warning: this may be hard), then make the easy change"

@benkoshy
Copy link

benkoshy commented Oct 30, 2019

@benoittgt Thank you for looking into this. Has the diff issue - that with confusing outputs when strings are actually different - been solved in Rspec?

@benoittgt
Copy link
Member Author

@BKSpurgeon At the moment no. This imply lot's of work and I am quite too busy at the moment to tackle that. I am sorry.

@benoittgt
Copy link
Member Author

I would like to work on:

Maybe we should first create a new class that will include diff mechanism (start with diff-lcs) plus a new ObjectFormater for actual/expected diff? For the end user it will mean no changes but for us it will be easier to start to work on a better diff output for RSpec 4?

Do you think @JonRowe this is the correct move?

@JonRowe
Copy link
Member

JonRowe commented Jul 3, 2020

Yes I think the first step is to make the differ configurable, with a reference implementation that uses diff-lcs, then we can add others including our own, we did make a start at this, there is a differ injected into expectations I believe...

@mcmire
Copy link

mcmire commented Jul 4, 2020

Hello! I know my gem super_diff was mentioned in the description above, but I wanted to resurface it in case it was buried. I've got it to a point to where I've used it every day at work for several months now and it works pretty well. It's fairly sophisticated in how it represents diffs internally: it will produce an AST of sorts first, then use that AST to generate an intelligent, data-structure-aware diff. It does use diff-lcs, but only for arrays and multiline strings (only because it works well in that particular use case). But it also supports custom diffing logic, and it knows how to make comparisons with things like "fuzzy" objects (an_object_having_attributes, a_collection_containing_exactly, kind_of, etc.). I am also working on a way to automatically elide portions of a large diff so that it's easier to read.

There was a question of what new differ code in RSpec would offer and what the expectations would be around that. I will say that I followed my own wants and wishes when it came to developing super_diff based on my experience dealing with large API responses in tests and such, but I would be curious if it satisfies the issues that people have raised in the past around other cases too. So first I would recommend trying it out if you haven't :) But of course, I'd still very much like it if RSpec had the same abilities, so I'm happy to share the knowledge I have and help with integrating those ideas into a new differ, if that is of interest. Just throwing that out there for your consideration!

@pirj pirj modified the milestone: 3.1 Dec 2, 2020
@pirj
Copy link
Member

pirj commented Dec 23, 2020

It caught my eye when a colleague used a desktop git tool with such an amazing output:
image

@pirj
Copy link
Member

pirj commented May 18, 2022

There are some new kids on the block, https://github.com/bpohoriletz/hash_deep_diff, and its (spiritual?) ancestors https://github.com/CodingZeal/hash_diff and https://github.com/liufengyun/hashdiff.
Nothing that compares to super_diff at a glance, though.

@JonRowe
Copy link
Member

JonRowe commented May 18, 2022

Realistically I think we'd vendor any new tool at this point, our needs aren't super complicated but its better to not rely on 3rd parties for us

@xiaohui-zhangxh
Copy link

I got a Hash diff problem that keys have different order, the result is difficult to read. this code works:

`spec/supports/hashdiff.rb

module HashDiffPatcher
  def diff_as_object(actual, expected)
    if actual.instance_of?(Hash) && expected.instance_of?(Hash)
      super(actual.sort.to_h, expected.sort.to_h)
    else
      super
    end
  end
end

RSpec::Support::Differ.prepend HashDiffPatcher

@pirj
Copy link
Member

pirj commented Oct 23, 2022

Would you like to send a PR with some benchmarks, @xiaohui-zhangxh ?
Semi-related PR if you're interested in problems. Might not affect the differ, but still worth looking at.

Questions to consider:

  • is it possible to sort hashes with keys of uncomparable types ({nil => 'nil', 0 => '0'})?
  • do all supported Ruby versions support .to_h on an Array that is result of .sort?
  • what if <=> returns 0 for two keys that are not really identical and, as the .sort doc states, the result is unstable?
  • what if <=> returns 0 for two keys that are not really identical and we later compare with a more strict comparison?

@xiaohui-zhangxh
Copy link

Would you like to send a PR with some benchmarks, @xiaohui-zhangxh ? Semi-related PR if you're interested in problems. Might not affect the differ, but still worth looking at.

Questions to consider:

  • is it possible to sort hashes with keys of uncomparable types ({nil => 'nil', 0 => '0'})?
  • do all supported Ruby versions support .to_h on an Array that is result of .sort?
  • what if <=> returns 0 for two keys that are not really identical and, as the .sort doc states, the result is unstable?
  • what if <=> returns 0 for two keys that are not really identical and we later compare with a more strict comparison?

You are right, there is problem with uncomparable keys. because my test case is only for simple hash, so make a self-maintained patcher(support) is enough. To make a PR maybe Hashdiff solution is better.

@slavingia
Copy link

Any movement on this? Would be great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants