Fix diff output when a fuzzy finder anything is inside an expected hash

[KarlHeitmann]

This is a simplified version of PR #596 , it solves the issue #551 "Diff reports confusing output when used with "fuzzy" matchers like anything "

This PR will fix only anything values associated to top level keys of a hash. It will not work with nested hashes.

Example output BEFORE changes in this PR:

@@ -1,4 +1,4 @@
-:anything_key => anything,
+:anything_key => "bcdd0399-1cfe-4de1-a481-ca6b17d41ed8",
 :fixed => "fixed",
-:trigger => "wrong",
+:trigger => "trigger",

Example output AFTER changes in this PR:

@@ -1,4 +1,4 @@
 :anything_key => anything,
 :fixed => "fixed",
-:trigger => "wrong",
+:trigger => "trigger",

[pirj]

undefined method `flatten' for #<Hash

in 1.8.7

Looks great otherwise, thank you!

[pirj]

For the elevator pitch, may I kindly as you to add before and after example outputs to the pr description?

[KarlHeitmann]

Ready!

• I've just added the before/after example outputs on the PR description.

• The error undefined method flatten for #<Hash> is really weird. I am not calling that method anywhere on the _spec.rb file (the file that is complaining), and the application file differ.rb calls flatten on line 135. But I have not touched that line on my branch. I made a workaround to bypass the new code if you are using ruby version 1.8.7. That should be enough to solve the issue...

[pirj]

It seems this is due to this. We call safely_flatten with a hash arg, and it calls flatten on it.
This will break 1.8.7. We can’t afford this. Even though I’m a proponent of soft-deprecating older Rubies in our code, this doesn’t include breaking hypothetical suites that might still exist.

Can the same be achieved somehow differently? Like taking .values from the hash?

[pirj]

Please disregard my note regarding breaking 1.8.7, I’ve just noticed that you have a check for this.

Still, does it make sense to flatten? We could just check the too level of the hash, right?

[pirj]

I’m also struggling to understand how this works.
We do pass expected and actual there, and they end up wrapped in an array when flatten is called on it?

[KarlHeitmann]

Indeed... that's weird... What if I remove the star operator from #no_procs and no_numbers method?

The purpose of this no_procs_and_no_numbers? method is to reduce the number of && operators used in the #diff method, thus appeasing rubocop by removing the Metrics::PerceivedComplexity offense.

[KarlHeitmann]

I've just simplified no_procs_and_no_numbers? in a new commit by removing the star operator.

I don't know if that's the best solution, the downside is it modifies the original no_procs? and no_numbers? input argument. Usually I don't like to alter the original code that existed before doing my changes.

Other solutions I could have taken that didn't need to alter the no_procs? and no_numbers? methods are:

• Alternative 2: copy paste the logic of no_procs? and no_numbers? and joins the expression with && operator

      def no_procs_and_no_numbers?(*args)
        safely_flatten(args).none? { |a| Proc === a } && safely_flatten(args).none? { |a| Numeric === a }
      end

• Alternative 3: an optimization of Alternative 2. This one will have better performance

      def no_procs_and_no_numbers?(*args)
        safely_flatten(args).none? { |a| Proc === a || Numeric === a }
      end

Both alternatives makes all test pass on my local machine.

[pirj]

args here will be an array of two elements?
How does it work to flatten an array of two hashes?

Can we check no_procs?(expected) && no_procs?(actual) && no_numbers? ….

Each if those methods runs the (potentially expensive?) safe_flatten. I’d prefer this to be done just once (or, better - never!)

Can’t we just take top-level keys of each of those arrays without flattening recursively?

This would work on 1.8.7
No tricks with has/array structures
Early exit if there is a proc/number
No extra checks in nested values that we don’t care about
Less code
No splatting/desplatting

[KarlHeitmann]

UPDATE: yeah... what you are mentioning here is a good idea: get rid of the safely_flatten by just taking the top-level keys of each of those elements: actual & expected. I am doing some benchmarks because I have not found the complexity of Hash methods like .none? or .any? on the ruby hash documentation or ruby enumerable documentation. I hope I'll have a simple proposal ready in the next days to submit it here :)

[KarlHeitmann]

I realized the change is simpler than I thought. I just introduced a new commit that reverts the no_procs_and_no_numbers? method and restores no_procs? and no_numbers? to its original state. This new commit introduces Differ#all_hashes?(actual, expected) private method that will check if actual and expected are Hashes. In such cases, it will call the method that checks the presence of RSpec::Mocks::ArgumentMatchers::AnyArgMatcher fuzzy matcher as value on the expected hash, then will mutate its anything value and will fold that diff on the output string.

I am doing this instead of using safely_flatten because the behavior I'm proposing on this PR is a specific case scenario: the user has given an actual and expected vars that both are Hash instances, and the expected var can contain an anything fuzzy matcher, so the output diff will contain less noise.

safely_flatten was introduced in fc01e3e5b4e711a471dde0dd65d5e528439e9fdf, and before this commit, methods such as def no_procs?(*args) were written like this:

def no_procs?(*args)
  args.flatten.none? { |a| Proc === a }
end

But that method was only called once in that old file. You can check it out with this command:

git show fc01e3e5~:lib/rspec/support/differ.rb

This leads me to think that before using safely_flatten, calling args.flatten inside no_procs?(*args) was just a fancy way to check the actual and expected vars were not Proc objects; instead of using something like this:

def no_procs?(actual, expected)
  (Proc != actual) && (Proc != expected)
end

The fancy way is more readable, and it has the benefit if actual or expected are Array objects, they can be recursively flattened and you could diff_as_object objects nested deep down in actual & expected arrays, as long as these nested objects are not Procs. Pretty cool, huh? The problem with this approach is that you could enter into an infinite recursion, that's why later on safely_flatten method was introduced.

But again, the code changes I'm proposing are narrowed down to a specific case scenario, where the user wants to diff two hashes in which one of them an anything fuzzy matcher is being used as a value of a top-level key of the expected var. No need to search recursively. Recursion should be addressed in my previous PR #596

[KarlHeitmann]

I've just submitted another commit, because I noticed another issue: on this line, the expected var mutates. So, if you provide Differ with a expected hash that contains an anything fuzzy matcher as a value in one of its top-level keys, after the differ.diff call the expected var will no longer contain the fuzzy matcher: it will contain the corresponding actual var value.

Hopefully the spec I've added will explain what the issue better than how I am explaining it here. Here is the code snippet:

it "checks the 'expected' var continues having the 'anything' fuzzy matcher, it has not mutated" do
  actual = { :fixed => "fixed", :trigger => "trigger", :anything_key => "bcdd0399-1cfe-4de1-a481-ca6b17d41ed8" }
  expected = { :fixed => "fixed", :trigger => "wrong", :anything_key => anything }
  differ.diff(actual, expected)
  expect(expected).to eq({ :fixed => "fixed", :trigger => "wrong", :anything_key => anything })
end

Here is the output before the application code changes (failing test)

     Failure/Error: DEFAULT_FAILURE_NOTIFIER = lambda { |failure, _opts| raise failure }
     
       expected: {:anything_key=>anything, :fixed=>"fixed", :trigger=>"wrong"}
            got: {:anything_key=>"bcdd0399-1cfe-4de1-a481-ca6b17d41ed8", :fixed=>"fixed", :trigger=>"wrong"}
     
       (compared using ==)

Here is the output AFTER the application code changes (test succeeds)

[pirj]

Nice find. Can this be the root cause of this getting its way to the diff in the first place?

1,4 @@
-:anything_key => anything,
+:anything_key => "bcdd0399-1cfe-4de1-a481-ca6b17d41ed8",

[pirj]

We only call this from all_hashes?, and it already has this check

Potentially, this method can be mistakenly called from somewhere else causing errors for those not using rspec-mocks, but i’m less worried about this at this point.

[KarlHeitmann]

Yes, it is currently called only under if all_hashes?(actual, expected) condition, and all_hashes? is also checking if the user is opting out rspec-mocks. This check in line 62 is redundant if we assume this method will only be called inside all_hashes? condition . But it is there in case someone mistakenly call the method from somewhere else, so the program won't break if rspec-mocks is opted-out.

If someone opts-out rspec-mock, diff_hashes_as_object should have the exact same behavior as diff_as_object. That's the purpose of this redundant check.

Personally, I like redundancy. But I understand sometimes it may be overkill or unnecessary... maybe defining diff_hashes_as_object as private is good enough? I don't have a strong opinion here.

[pirj]

What I really liked about your previous changes was the potential to get rid of the recursive check and the safely_flatten method.
No pressure, as this is not the goal of the pr, but it would be a nice bonus to fixing the issue. And a performance improvement in theory.

[KarlHeitmann]

I understand, it would be nice to get rid of that recursivity. You can count on me to brainstorm on another PR or submit a proposal to fix that issue. I am enjoying reading and getting my hands on this project, and once this PR is finished (by finished I mean either merged or closed (if you and the other maintainers believe the changes I'm proposing here are too risky, or not worth doing them)) I'd like to propose some changes to make the BuiltIn::Change matcher class diffable when it tries to match a Hash in rspec-expectations repo... but that's another story.

About your concern with safely_flatten, that method was introduced ~10 years ago. Does anyone still knows why that method is there? I think its purpose is not only to check in a one method call whether all the arguments are not Proc, but along with no_numbers? to make Differ work with deep down nested arrays inside actual and expected vars. If we stick with my previous change, we can make the method faster, but we may loose the Differ feature to diff deep down nested arrays in a seamlessly way.

Just some brainstorming and ramble here: Is it worth to keep that recursivity? when an expected and actual vars with deep nested arrays are submitted as input to Differ, is worth to display a big diff output? are big diff strings useful when you have to scroll down pages of information? or a diff string is useful only when it shows the exact difference and a little bit of context? Maybe the answers to these questions will pop up when we will see such diff strings... I don't know the answer.

On the project I am currently working on, when I'm testing the actual and expected values of a hash with nested hashes inside, I've found is useful to strip my actual hash of its nested objects in my test set-up, and inside an aggregate_failures block use one expect(...)... sentence for the main hash, and for each nested object use an extra expect(...)... sentence. That's a nice workaround instead of testing a big hash in a single expect(...)... sentence and expect a single diff to show me -everything- I want to see (the idea behind #596 )

[pirj]

I can’t tell of the top of my head with certainty, but I can’t recall if nested hashes are (usefully) diffed.

I’d rather simplify the code if no spec fails.

[pirj]

Now when I look at this, this is indeed and improvement to -anything_key +anything_key we had.
But do we need a potentially huge value here? An uuid is fine, but what about a 5kb json-like thing?

Most certainly if we’ve anythinged it in the test, we don’t care what’s inside, including the diff?

@JonRowe does this make sense?

[KarlHeitmann]

Are you concerned by the case a 5kb json-like thing would be copied into the expected -> anything value? that will definitively be a performance issue. Would it be worth doing the change? Is there a way we can make sure the 5kb json-like thing will be a shallow copy? Maybe a shallow copy will mitigate the performance issue.

[KarlHeitmann]

Maybe a shallow copy will not impact memory ram consumption. But what about CPU time? does diff_as_object iterates the actual and expected vars regardless of their .object_id? or it checks the object_id and if they match it will cease any iteration?

Another question: rspec-support module should be allowed to mutate expected vars? regardless they will be later on return them to their original state? My proposed changes on this PR are a hacky way to fold noise when the developers use anything fuzzy matcher on their hash, I understand this may not be allowed to do here.

[KarlHeitmann]

Apparently, a 5kb json-like thing will impact CPU time ONLY if the matcher fails.

  def diff_as_object(actual, expected)
    actual_as_string = object_to_string(actual) # STEP 1
    expected_as_string = object_to_string(expected) # STEP 2
    diff_as_string(actual_as_string, expected_as_string) # STEP 3
  end

STEP 1 and STEP 2 will get the string representation of 5kb json-like thing in two different strings. STEP 3 will perform the comparison of both strings. This will be an expensive operation because both strings will be huge. In contrast, without the changes on this PR, comparing the string representation of this 5kb json-like thing with anything fuzzy matcher would have been easy-peasy

AFAIK, RSpec::Support::Differ is called when an expectation matcher fails AND the expectation matcher responds to diffable?. If the matcher does not responds to diffable, expected and actual vars are assigned nil and RSpec::Support::Differ will not perform any action at all.

My assessment is this will impact performance only when the test fails and the developer is using a matcher that produces an output-diff. But this is my assessment about performance, I still don't know the answer to: should rspec-support module be allowed to mutate expected vars in the developer test bench (ie, his _spec.rb files)? or rspec-support should remain a "pure" read-only module?

[JonRowe]

I think @pirj's concern was about larger diffs, but as things should be identical the differ will often not print anything at all

[pirj]

Identical - yes. But if it’s neighbouring with the difference, like the ‘anything_key’ here on line 567, and it is big, i would prefer to see the literal “anything” to be printed rather than 5kb of json.

[KarlHeitmann]

Yes, the cause of:

1,4 @@
-:anything_key => anything,
+:anything_key => "bcdd0399-1cfe-4de1-a481-ca6b17d41ed8",

is the mutation there: First I take the expected var, I ask it if it is a Hash instance, and if it is a Hash, I'll select its values on the top-level keys that are anything and mutate them into their corresponding actual value. That's the core idea to fold all that noise. On the first PR #596 I applied that to all nested hashes recursively... but that's not very useful because the current differ RSpec::Support::Differ shows a single red line change for any nested hash... it is not very useful and I don't think it is worth doing that (side note: although it is proven possible to do on #596 ).

Before submitting the latest commit c3e9ea926463829a048a131f7b85fef16c186ef5 , I thought the mutation would occur inside the Differ class, but will not "bubble up" to the test scope... but I was wrong, that commit fixes that issue.

[JonRowe]

Sorry for the delayed review, I think this localised change makes sense as it should reduce diff confusion. I have some suggestions though.

[JonRowe]

I wouldn't check if we have an any arg here, the other method already does this.

Suggested change

	def all_hashes?(actual, expected)
	defined?(RSpec::Mocks::ArgumentMatchers::AnyArgMatcher) && (Hash === actual) && (Hash === expected)
	end
	def all_hashes?(actual, expected)
	(Hash === actual) && (Hash === expected)
	end

[JonRowe]

Typically we define such methods conditionally rather than doing checks in the method e.g.

Suggested change

	def diff_hashes_as_object(actual, expected)
	if defined?(RSpec::Mocks::ArgumentMatchers::AnyArgMatcher)
	anything_hash = expected.select { |_, v| RSpec::Mocks::ArgumentMatchers::AnyArgMatcher === v }
	anything_hash.each_key do |k|
	expected[k] = actual[k]
	end
	diff_string = diff_as_object(actual, expected)
	anything_hash.each do |k, v|
	expected[k] = v
	end
	diff_string
	else
	diff_as_object(actual, expected)
	end
	end
	if defined?(RSpec::Mocks::ArgumentMatchers::AnyArgMatcher)
	def diff_hashes_as_object(actual, expected)
	anything_hash = expected.select { |_, v| RSpec::Mocks::ArgumentMatchers::AnyArgMatcher === v }
	anything_hash.each_key do |k|
	expected[k] = actual[k]
	end
	diff_string = diff_as_object(actual, expected)
	anything_hash.each do |k, v|
	expected[k] = v
	end
	diff_string
	end
	else
	def diff_hashes_as_object(actual, expected)
	diff_as_object(actual, expected)
	end
	end

I'd also like to see a hash built from expected rather than mutating the original hash e.g.

expected_to_diff =
  expected.reduce({}) do |hash, (key, value)|
    if RSpec::Mocks::ArgumentMatchers::AnyArgMatcher === v
      hash[key] = actual[key]  
    else
      hash[key] = expected[key]
    end
    hash
  end

This has the benefit of less enumerations as well as a safety aspect

[JonRowe]

I think @pirj's concern was about larger diffs, but as things should be identical the differ will often not print anything at all

[KarlHeitmann]

Almost ready @JonRowe . ⚠️ ⚠️ ⚠️ But there is a caveat ⚠️ ⚠️ ⚠️

• The first commit 31f086e addresses your comments in which I could make safe changes.

• The second commit dd7a4c7 addresses your comment related to conditionally define my method diff_hashes_as_object or just "alias" it to diff_as_object. But I discovered these changes can break your test suite by throwing a random false positive test failure.

If I run only that test file with the command:

bundle exec rspec ./spec/rspec/support/differ_spec.rb

A failure rises. Because the RSpec.describe Differ sentence ⚠️ ⚠️ ⚠️ will load RSpec::Support::Differ before RSpec::Mocks::ArgumentMatchers::AnyArgMatcher is required on RSpec ⚠️ ⚠️ ⚠️ . So, the test I introduced with the anything matcher will always fail with the command above: because it will define the alias version of my method instead of defining my method that handles anything fuzzy matcher.

But if you run the full test suite with:

bundle exec rspec spec

Chances are all tests will pass, unless differ_spec.rb file IS the 1st file to be run. I've ran the full test suite many many times (20+ and I got bored) and I was always lucky. I've introduced a puts "FLAKEY TEST FUZZY MATCHER RUN" sentence on the flaky test, Below is an example screenshot with 4 trials... but if your CI CD has very bad luck, you may find some false positives.

I don't know how to fix this issue, is there a way to force the test suite to not run differ_spec.rb first? But if we do so, anyone that wants to add something to RSpec::Support::Differ will stumble upon this issue when running rspec ./spec/rspec/support/differ_spec.rb

I've tried to dig in and find where AnyArgMatcher is loaded to try to force differ_spec.rb to load it before the RSpec.describe Differ do sentence. But I didn't had success. the furthest I went is here: rspec-core/lib/rspec/core/dsl.rb#L43 , on that line, I've put a binding.break before the class gets defined. I hope this screenshot can provide some more information:

What should we do about this?

[pirj]

What if we quote it?

RSpec.describe "Differ" do

it’s the way we recommend writing specs anyway. Would it solve the flakiness issue?

Alternatively. We autoload classes, and numerous places may trigger AnyArgMatcher to load.
If we have no intention to run specs when rspec-mocks are not loaded on purpose, then I don’t see any harm if you require the file from rspec-mocks that defines it explicitly at the bottom of this spec file.

[KarlHeitmann]

That's awesome! RSpec.describe "Differ" do fixed the issue! I've run the single test file and the whole test suite and I had no problems at all.

If there are no more changes left to do, I can squash all the commits into a single one.

BTW, What's the difference between RSpec.describe the class and the name of the class as string?

[pirj]

Nice! Let me have another quick look.

RSpec.describe the class and the name of the class as string?

This autoloading (this is the first in my practice). And also probably described_class won’t work.

[pirj]

uninitialized constant RSpec::Support::HunkGenerator
# ./spec/rspec/support/differ_spec.rb:169

on 2.7 🤔

[pirj]

There are a few things with potential improvements in this code. But I don’t think they are related to this PR.

The change looks good. Thank you!

[KarlHeitmann]

You're welcome! Is there anything else I can do for you here? @pirj @JonRowe ? I can squash all the commits into a single one if you think this PR is OK.

BTW, in Ruby 2.7 build it is complaining about spec/rspec/support/differ_spec.rb:169

  1) Differ #diff handles any encoding error that occurs with a helpful error message
     Failure/Error:
       expect(RSpec::Support::HunkGenerator).to receive(:new).
         and_raise(Encoding::CompatibilityError)

     NameError:
       uninitialized constant RSpec::Support::HunkGenerator

I don't know how can I reproduce this error on my local machine. I have not touched this line 169 before, is it a flaky test? or is it related to changing:

RSpec.describe Differ do

to

RSpec.describe "Differ" do

?

[pirj]

2.7 passed on re-run. Must be order-dependent load order flakiness. Let’s see if it bugs us again. But I don’t think it should stop from merging.

Thanks again.
@JonRowe wdyt?

[JonRowe]

Ugh I hate to say this at this late juncture, but I think @pirj's earlier point about large nested hashes has a point, if our point here is to make the diff smaller by making things equal, and the matcher is matching on anything then why aren't we doing the inverse of this? e.g. Creating a actual_to_diff and setting the ignored keys to "anything"

[KarlHeitmann]

That's thinking outside of the box! I tried your suggestion and it works! I didn't thought about it... but using the actual_to_diff approach is better than using the expected_to_diff approach 👍

[pirj]

Just perfect now 🙌 Thank you!
And thanks @JonRowe for the smart suggestion.

[pirj]

🤩

[pirj]

HunkGenerator again, flaky

[JonRowe]

Thanks!