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

KarlHeitmann · 2024-05-25T03:11:11Z

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 · 2024-05-25T07:17:00Z

undefined method `flatten' for #<Hash

in 1.8.7

Looks great otherwise, thank you!

pirj · 2024-05-25T07:18:13Z

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

KarlHeitmann · 2024-05-25T22:33:02Z

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 · 2024-05-26T14:24:07Z

lib/rspec/support/differ.rb

@@ -73,6 +84,14 @@ def initialize(opts={})

    private

+      def hash_with_anything?(arg)
+        Hash === arg && safely_flatten(arg).any? { |a| RSpec::Mocks::ArgumentMatchers::AnyArgMatcher === a }


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?

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 · 2024-05-26T18:50:29Z

lib/rspec/support/differ.rb

+        Hash === arg && safely_flatten(arg).any? { |a| RSpec::Mocks::ArgumentMatchers::AnyArgMatcher === a }
+      end
+
+      def no_procs_and_no_numbers?(*args)


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?

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 · 2024-05-30T05:27:49Z

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 · 2024-05-30T09:41:04Z

lib/rspec/support/differ.rb

+      end
+
+      def no_procs_and_no_numbers?(*args)
+        no_procs?(args) && no_numbers?(args)


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 · 2024-06-03T01:52:27Z

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 · 2024-06-08T22:21:52Z

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 · 2024-06-09T02:19:16Z

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 · 2024-06-09T07:57:18Z

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",

lib/rspec/support/differ.rb

pirj · 2024-06-09T08:02:23Z

lib/rspec/support/differ.rb

@@ -56,6 +58,28 @@ def diff_as_string(actual, expected)
      end
      # rubocop:enable Metrics/MethodLength

+      def diff_hashes_as_object(actual, expected)
+        if defined?(RSpec::Mocks::ArgumentMatchers::AnyArgMatcher)


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.

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 · 2024-06-09T08:06:44Z

lib/rspec/support/differ.rb

@@ -77,6 +101,10 @@ def no_procs?(*args)
        safely_flatten(args).none? { |a| Proc === a }


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.

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 )

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 · 2024-06-09T08:12:10Z

spec/rspec/support/differ_spec.rb

+            expected_diff = dedent(<<-'EOD')
+              |
+              |@@ -1,4 +1,4 @@
+              | :anything_key => "bcdd0399-1cfe-4de1-a481-ca6b17d41ed8",


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?

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.

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.

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?

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

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 · 2024-06-12T02:43:24Z

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 · 2024-06-14T11:46:10Z

lib/rspec/support/differ.rb

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


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 · 2024-06-14T11:57:16Z

lib/rspec/support/differ.rb

+      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


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

spec/rspec/support/differ_spec.rb

JonRowe · 2024-06-14T12:00:29Z

spec/rspec/support/differ_spec.rb

+            expected_diff = dedent(<<-'EOD')
+              |
+              |@@ -1,4 +1,4 @@
+              | :anything_key => "bcdd0399-1cfe-4de1-a481-ca6b17d41ed8",


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 · 2024-06-15T05:04:41Z

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 · 2024-06-15T15:59:30Z

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 · 2024-06-16T01:47:05Z

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 · 2024-06-16T06:52:22Z

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 · 2024-06-16T06:54:37Z

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 · 2024-06-26T14:08:08Z

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 · 2024-06-26T16:56:40Z

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 · 2024-06-26T17:00:49Z

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"

lib/rspec/support/differ.rb

KarlHeitmann · 2024-06-27T04:04:17Z

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 · 2024-06-27T05:54:45Z

spec/rspec/support/differ_spec.rb

+            expected_diff = dedent(<<-'EOD')
+              |
+              |@@ -1,4 +1,4 @@
+              | :anything_key => anything,


lib/rspec/support/differ.rb

Co-authored-by: Jon Rowe <mail@jonrowe.co.uk>

pirj · 2024-06-27T10:14:11Z

HunkGenerator again, flaky

JonRowe · 2024-06-30T21:35:21Z

Thanks!

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

b993756

KarlHeitmann mentioned this pull request May 25, 2024

Fixes diff when fuzzy finder anything is used in a Hash object (proof of concept) #596

Closed

Add code review feedback, this feat works only ruby version > 1.8.7

4c22e70

KarlHeitmann force-pushed the fix_diff_fuzzy_matcher_anything_v2 branch from daa5bb5 to 4c22e70 Compare May 25, 2024 22:32

pirj reviewed May 26, 2024

View reviewed changes

simplifies no_procs? and no_numbers? by removing star operator

048ac0a

pirj reviewed May 30, 2024

View reviewed changes

KarlHeitmann force-pushed the fix_diff_fuzzy_matcher_anything_v2 branch from 77aa475 to e5de130 Compare June 8, 2024 22:24

introduces Differ#all_hashes to check actual and expected are Hash

c3e9ea9

KarlHeitmann force-pushed the fix_diff_fuzzy_matcher_anything_v2 branch from e5de130 to c3e9ea9 Compare June 9, 2024 01:34

if AnyArgMatcher is defined, diff_hashes_as_object folds anything

416cd49

KarlHeitmann force-pushed the fix_diff_fuzzy_matcher_anything_v2 branch from 237db12 to 416cd49 Compare June 9, 2024 02:13

pirj reviewed Jun 9, 2024

View reviewed changes

lib/rspec/support/differ.rb Outdated Show resolved Hide resolved

pirj reviewed Jun 9, 2024

View reviewed changes

fix: removed unnecesary rspec-mock check

0dea769

JonRowe requested changes Jun 14, 2024

View reviewed changes

KarlHeitmann added 2 commits June 14, 2024 23:16

applied code review feedback: removed defined? check and use reduce

31f086e

code review feedback: defines diff_hashes_as_object conditionally

dd7a4c7

KarlHeitmann force-pushed the fix_diff_fuzzy_matcher_anything_v2 branch from 15843e3 to dd7a4c7 Compare June 15, 2024 04:50

KarlHeitmann requested a review from JonRowe June 15, 2024 05:04

Changes Differ by "Differ" to load AnyArgMatcher before loading Differ

938096c

KarlHeitmann requested a review from pirj June 16, 2024 01:47

pirj approved these changes Jun 16, 2024

View reviewed changes

JonRowe reviewed Jun 26, 2024

View reviewed changes

lib/rspec/support/differ.rb Outdated Show resolved Hide resolved

code review feedback: invert expected_to_diff to actual_to_diff

7046659

KarlHeitmann requested review from JonRowe and pirj June 27, 2024 04:04

pirj approved these changes Jun 27, 2024

View reviewed changes

spec/rspec/support/differ_spec.rb

expected_diff = dedent(<<-'EOD')

|

|@@ -1,4 +1,4 @@

| :anything_key => anything,

Copy link

Member

pirj Jun 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤩

JonRowe reviewed Jun 27, 2024

View reviewed changes

lib/rspec/support/differ.rb Outdated Show resolved Hide resolved

Update lib/rspec/support/differ.rb

6b7ff69

Co-authored-by: Jon Rowe <mail@jonrowe.co.uk>

pirj requested a review from JonRowe June 27, 2024 10:20

JonRowe merged commit d6bdd9f into rspec:main Jun 30, 2024
29 of 30 checks passed

JonRowe added a commit that referenced this pull request Jun 30, 2024

Changelog for #599

23b3756

KarlHeitmann deleted the fix_diff_fuzzy_matcher_anything_v2 branch June 30, 2024 22:31

		@@ -77,6 +101,10 @@ def no_procs?(*args)
		safely_flatten(args).none? { \|a\| Proc === a }

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

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

Conversation

KarlHeitmann commented May 25, 2024 • edited by pirj Loading

pirj commented May 25, 2024

pirj commented May 25, 2024

KarlHeitmann commented May 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KarlHeitmann commented May 30, 2024

Choose a reason for hiding this comment

KarlHeitmann commented Jun 3, 2024

KarlHeitmann commented Jun 8, 2024

KarlHeitmann commented Jun 9, 2024

pirj commented Jun 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KarlHeitmann commented Jun 12, 2024

JonRowe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KarlHeitmann commented Jun 15, 2024

pirj commented Jun 15, 2024

KarlHeitmann commented Jun 16, 2024

pirj commented Jun 16, 2024

pirj commented Jun 16, 2024

pirj left a comment

Choose a reason for hiding this comment

KarlHeitmann commented Jun 26, 2024

pirj commented Jun 26, 2024

JonRowe commented Jun 26, 2024

KarlHeitmann commented Jun 27, 2024

pirj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pirj commented Jun 27, 2024

JonRowe commented Jun 30, 2024

KarlHeitmann commented May 25, 2024 •

edited by pirj

Loading

pirj commented Jun 9, 2024 •

edited

Loading