Skip to content
This repository has been archived by the owner on Nov 30, 2024. It is now read-only.

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

Merged
merged 11 commits into from
Jun 30, 2024
Merged

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
b993756
Fix diff output when a fuzzy finder anything is inside an expected hash
KarlHeitmann Apr 24, 2024
4c22e70
Add code review feedback, this feat works only ruby version > 1.8.7
KarlHeitmann May 25, 2024
048ac0a
simplifies no_procs? and no_numbers? by removing star operator
KarlHeitmann May 30, 2024
c3e9ea9
introduces Differ#all_hashes to check actual and expected are Hash
KarlHeitmann Jun 8, 2024
416cd49
if AnyArgMatcher is defined, diff_hashes_as_object folds anything
KarlHeitmann Jun 9, 2024
0dea769
fix: removed unnecesary rspec-mock check
KarlHeitmann Jun 12, 2024
31f086e
applied code review feedback: removed defined? check and use reduce
KarlHeitmann Jun 15, 2024
dd7a4c7
code review feedback: defines `diff_hashes_as_object` conditionally
KarlHeitmann Jun 15, 2024
938096c
Changes Differ by "Differ" to load AnyArgMatcher before loading Differ
KarlHeitmann Jun 16, 2024
7046659
code review feedback: invert expected_to_diff to actual_to_diff
KarlHeitmann Jun 27, 2024
6b7ff69
Update lib/rspec/support/differ.rb
pirj Jun 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 23 additions & 4 deletions lib/rspec/support/differ.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,12 @@ def diff(actual, expected)
if any_multiline_strings?(actual, expected)
diff = diff_as_string(coerce_to_string(actual), coerce_to_string(expected))
end
elsif no_procs?(actual, expected) && no_numbers?(actual, expected)
diff = diff_as_object(actual, expected)
elsif no_procs_and_no_numbers?(actual, expected)
if (RUBY_VERSION.to_f > 1.8) && hash_with_anything?(expected)
diff = diff_as_object_with_anything(actual, expected)
else
diff = diff_as_object(actual, expected)
end
end
end

Expand Down Expand Up @@ -56,6 +60,13 @@ def diff_as_string(actual, expected)
end
# rubocop:enable Metrics/MethodLength

def diff_as_object_with_anything(actual, expected)
expected.select { |_, v| RSpec::Mocks::ArgumentMatchers::AnyArgMatcher === v }.each_key do |k|
expected[k] = actual[k]
end
diff_as_object(actual, expected)
end

def diff_as_object(actual, expected)
actual_as_string = object_to_string(actual)
expected_as_string = object_to_string(expected)
Expand All @@ -73,7 +84,15 @@ def initialize(opts={})

private

def no_procs?(*args)
def hash_with_anything?(arg)
Hash === arg && safely_flatten(arg).any? { |a| RSpec::Mocks::ArgumentMatchers::AnyArgMatcher === a }
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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?

end

def no_procs_and_no_numbers?(*args)
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

no_procs?(args) && no_numbers?(args)
Copy link
Member

Choose a reason for hiding this comment

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

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

end

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

Expand All @@ -85,7 +104,7 @@ def any_multiline_strings?(*args)
all_strings?(*args) && safely_flatten(args).any? { |a| multiline?(a) }
end

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

Expand Down
21 changes: 21 additions & 0 deletions spec/rspec/support/differ_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -555,6 +555,27 @@ def inspect; "<BrokenObject>"; end
expect(differ.diff(false, true)).to_not be_empty
end
end

unless RUBY_VERSION == '1.8.7'
describe "fuzzy matcher anything" do
it "outputs only key value pair that triggered diff, anything_key should absorb actual value" do
actual = { :fixed => "fixed", :trigger => "trigger", :anything_key => "bcdd0399-1cfe-4de1-a481-ca6b17d41ed8" }
expected = { :fixed => "fixed", :trigger => "wrong", :anything_key => anything }
diff = differ.diff(actual, expected)
expected_diff = dedent(<<-'EOD')
|
|@@ -1,4 +1,4 @@
| :anything_key => "bcdd0399-1cfe-4de1-a481-ca6b17d41ed8",
| :fixed => "fixed",
|-:trigger => "wrong",
|+:trigger => "trigger",
|
EOD
# puts diff
expect(diff).to be_diffed_as(expected_diff)
end
end
end
end
end
end
Expand Down
Loading