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

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

Closed
wants to merge 11 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
8c81146
fixes diff when fuzzy finder anything is used
KarlHeitmann Apr 24, 2024
e4ece3d
fix works in hashes with nested hashes
KarlHeitmann Apr 24, 2024
4de7d8c
fix: gets rid of Metrics::CyclomaticComplexity in Differ#diff
KarlHeitmann May 7, 2024
d50d742
fix: gets rid of Metrics::PerceivedComplexity in Diff#diff
KarlHeitmann May 7, 2024
41e3c21
fix: gets rid of Metrics/MethodLength in Diff#diff
KarlHeitmann May 7, 2024
15627b7
fix: error build ruby 2.4,3,2: complain in prepend
KarlHeitmann May 7, 2024
f9a9e9d
fix: rearrange test to pass legacy ruby build 1.8.7 & REE
KarlHeitmann May 8, 2024
d44a555
fix: short circuit recursive_get_key when user opts out Mocks
KarlHeitmann May 10, 2024
2bb81a4
refactor: changes instance var by a returned local var
KarlHeitmann May 19, 2024
8be1fd8
refactor: marks tests for fuzzy matcher broken on 1.8.7
KarlHeitmann May 19, 2024
47cf1b2
adds private method Differ#run to get rid of PerceivedComplexity offense
KarlHeitmann May 19, 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
68 changes: 56 additions & 12 deletions lib/rspec/support/differ.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,19 +11,9 @@ module Support
# rubocop:disable Metrics/ClassLength
class Differ
def diff(actual, expected)
diff = ""
return "" if actual.nil? && expected.nil?

unless actual.nil? || expected.nil?
if all_strings?(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)
end
end

diff.to_s
run(actual, expected)
end

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

def diff_as_object_with_anything(total_keys, actual, expected)
total_keys.each do |keys|
pointer_expected = expected
pointer_actual = actual
final_key = keys.pop
keys.each do |k|
pointer_expected = pointer_expected[k]
pointer_actual = pointer_actual[k]
end
pointer_expected[final_key] = pointer_actual[final_key]
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 @@ -72,6 +76,46 @@ def initialize(opts={})
end

private
def run(actual, expected)
diff = ""

no_procs_no_numbers = lambda {|var1, var2| no_procs?(var1, var2) && no_numbers?(var1, var2)}
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason not to make this a method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know, I don't have a strong opinion either here. Originally I did it this way:

            if no_procs?(actual, expected) && no_numbers?(actual, expected)
               diff = diff_as_object_with_anything(actual, expected)
             end
          elsif no_procs?(actual, expected) && no_numbers?(actual, expected)
             diff = diff_as_object(actual, expected)

But Rubocop complained about offending Metrics::PerceivedComplexity rule, in order to lower the perceived complexity score I made the lambda function to reduce the number of && operators by 1, and that made the offense disappear.

I didn't made a method because I didn't know if it was overkill or not to create a method that only me will use it in only two places. But I don't have a strong opinion, I can write the method if you think it is better :)

Copy link
Member

Choose a reason for hiding this comment

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

I mean is it possible to extract ‘no_procs_no_numbers?’ to a method? That would even further reduce the complexity, right? And (I may be mistaken) reduce memory allocations on subsequent calls.


if all_strings?(actual, expected)
diff = diff_as_string(coerce_to_string(actual), coerce_to_string(expected)) if any_multiline_strings?(actual, expected)
elsif (keys = all_keys_from_hash(expected)) && keys.any?
Copy link
Member

Choose a reason for hiding this comment

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

What if we check no_procs_no_numbers here, and the other check goes inside?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice, that makes it easier to read! Thanks! I've submitted a new commit with your suggestion.

Copy link
Member

Choose a reason for hiding this comment

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

Something broke specs, I hope it’s not because of this cosmetic suggestion. Please feel free to revert if this is the case

diff = diff_as_object_with_anything(keys, actual, expected) if no_procs_no_numbers.call(actual, expected)
elsif no_procs_no_numbers.call(actual, expected)
diff = diff_as_object(actual, expected)
end

diff.to_s
end

def all_keys_from_hash(arg)
return false unless Hash === arg

recursive_get_keys(arg)
end

def recursive_get_keys(hash)
return [] unless defined?(RSpec::Mocks::ArgumentMatchers::AnyArgMatcher)

hash.reduce([]) do |acc, pair|
if RSpec::Mocks::ArgumentMatchers::AnyArgMatcher === pair[1]
acc << [pair[0]]
else
if Hash === pair[1]
keys = recursive_get_keys(pair[1])
keys.each do |key|
key.unshift pair[0]
acc << key
end
end
end
acc
end
end

def no_procs?(*args)
safely_flatten(args).none? { |a| Proc === a }
Expand Down
70 changes: 70 additions & 0 deletions spec/rspec/support/differ_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
# encoding: utf-8

require 'spec_helper'
require 'ostruct'
require 'timeout'
Expand Down Expand Up @@ -555,6 +556,75 @@ def inspect; "<BrokenObject>"; end
expect(differ.diff(false, true)).to_not be_empty
end
end

unless RUBY_VERSION == '1.8.7' # We can't count on the ordering of the hash on 1.8.7, fuzzy matcher anything will continue broken on that version
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
expect(diff).to be_diffed_as(expected_diff)
end

context "with nested hash" do
it "outputs only key value pair that triggered diff, anything_key should absorb actual value" do
actual = { :an_key => "dummy", :fixed => "fixed", :nested => { :anything_key => "bcdd0399-1cfe-4", :name => "foo", :trigger => "trigger" } }
expected = { :an_key => anything, :fixed => "fixed", :nested => { :anything_key => anything, :name => "foo", :trigger => "wrong" } }
diff = differ.diff(actual, expected)
expected_diff = dedent(<<-'EOD')
|
|@@ -1,4 +1,4 @@
| :an_key => "dummy",
| :fixed => "fixed",
|-:nested => {:anything_key=>"bcdd0399-1cfe-4", :name=>"foo", :trigger=>"wrong"},
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 in the fence about this change. For nested arrays, doesn’t it make more sense to indicate that a certain key was anything?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't have a strong opinion for nested arrays. Displaying that a certain key was anything for nested arrays was the original behavior.

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 still not too familiar with this addition. Do you yhink it is possible to keep the effect of collapsed “an_key” which I undoubtedly like, but make no changes to the “anything_key”’s value in expected of a nested array?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea, sure. If you checkout the first commit of this PR, you'll be on a state where there is no recursion, thus no further modifications on nested objects.

I have no problem at all to revert to that state. I don't have a strong opinion about morphing anything fuzzy matcher in nested objects. After all, when a nested object differs you get the entire object highlighted in red anyways, so I understand morphing deep nested values (like the one you are unfamiliar with: :anything_key) may not be worth to do, and it may be better to only keep the effect of collapsed first level keys like "an_key".

But I don't know the answer to this, just want to give you options so you can choose a good answer. You have a lot more experience than I in this project :)

|+:nested => {:anything_key=>"bcdd0399-1cfe-4", :name=>"foo", :trigger=>"trigger"},
|
EOD
expect(diff).to be_diffed_as(expected_diff)
end
end

context "with nested hash and subnested hash" do
it "outputs only key value pair that triggered diff, anything_key should absorb actual value" do
actual = {
:an_key => "dummy", :fixed => "fixed",
:nested => {
:subnested => { :name => "foo", :trigger => "trigger", :subnested_anything_key => "bcdd0399-1cfe-4de1-a481-ca6b17d41ed8" },
:nested_anything_key => "9930ddcb-1cfe-4de1-a481-ca6b17d41ed8"
}
}
expected = {
:an_key => anything, :fixed => "fixed",
:nested => {
:subnested => { :name => "foo", :trigger => "wrong", :subnested_anything_key => anything },
:nested_anything_key => anything
}
}
# binding.break
diff = differ.diff(actual, expected)
expected_diff = dedent(<<-'EOD')
|
|@@ -1,4 +1,4 @@
| :an_key => "dummy",
| :fixed => "fixed",
|-:nested => {:nested_anything_key=>"9930ddcb-1cfe-4de1-a481-ca6b17d41ed8", :subnested=>{:name=>"foo", :subnested_anything_key=>"bcdd0399-1cfe-4de1-a481-ca6b17d41ed8", :trigger=>"wrong"}},
|+:nested => {:nested_anything_key=>"9930ddcb-1cfe-4de1-a481-ca6b17d41ed8", :subnested=>{:name=>"foo", :subnested_anything_key=>"bcdd0399-1cfe-4de1-a481-ca6b17d41ed8", :trigger=>"trigger"}},
|
EOD
expect(diff).to be_diffed_as(expected_diff)
end
end
end
end
end
end
end
Expand Down
Loading