Closed b-loyola closed 2 years ago
What will diff output after this change?
Diff:
:object1 => 0 minutes and 90 seconds,
-:object2 => 1 minutes and 90 seconds,
+:object2 => 0 minutes and 90 seconds,
I love the idea.
Maybe we could specify in the failing output the custom inspector used?
@pirj , I've updated the expected behaviour section to clarify what the diff would look like after registering the new inspector. Hopefully that answers your question but let me know if it doesn't. Note that the change here would not affect any diffs unless a new inspector is registered. It only makes it easier to add custom inspectors to allow for customization of diff output for certain objects.
@benoittgt can you clarify in terms of implementation what you have in mind? Are you thinking we should differentiate between custom inspectors and default ones (the ones currently in INSPECTOR_CLASSES
) and concatenate something in the inspect
itself to denote that the object was inspected with a custom inspector? E.g.:
Diff:
@@ -1,3 +1,3 @@
:object1 => 90 (inspected with TimePartsInspector),
-:object2 => 150 (inspected with TimePartsInspector),
+:object2 => 90 (inspected with TimePartsInspector),
From what I understand of the code (and I could definitely be misunderstanding this), with the exception of arrays and hashes, currently it will always use an inspector to inspect any object, falling back to UninspectableObjectInspector
and ultimately to InspectableObjectInspector
if all else fails. So I believe we would need to differentiate between the default ones custom ones to avoid adding too much noise in the failing output?
@b-loyola yes that was exactly what I was thinking when I saw your example.
So I believe we would need to differentiate between the default ones custom ones to avoid adding too much noise in the failing output?
Yes completely. Only custom one could be "noisy" by default. I am worried about having future issues on RSpec with people that use invalid custom inspector but doesn't know it is used in diff.
Also I think this PR should be added in RSpec 4 only. :)
But I think that first we should wait for @JonRowe feedbacks on your existing PR. ;)
There are two problems here.
TimeParts#to_s
and TimeParts#inspect
? Like e.g. the beforementioned ActiveSupport::Duration
does?
1.minute.to_s => "60"
60.seconds.to_s # => "60"
1.minute.inspect # => "1 minute"
60.seconds.inspect # => "60 seconds"
This wouldn't be a problem if:
super_diff
is an option)2.1 Hashes
Failure/Error: it { expect({foo: 1.minute, bar: 1.minute}).to include(foo: 60.seconds, bar: 59.seconds) }
expected {:bar => 1 minute, :foo => 1 minute} to include {:bar => 59 seconds}
Diff:
@@ -1,3 +1,3 @@
-:bar => 59 seconds,
-:foo => 60 seconds,
+:bar => 1 minute,
+:foo => 1 minute,
as you already mentioned, it's the diff that has a deficiency. Should have been:
{
:foo => 60 seconds,
- :bar => 59 seconds,
+ :bar => 1 minute,
}
For comparison, super_diff
's output:
{
foo: #<ActiveSupport::Duration:0x00007fd7a3e3d2f8 {
@parts=nil,
@value=60
}>,
- bar: #<ActiveSupport::Duration:0x00007fd7a3e3cfd8 {
- @parts=nil,
- @value=59
- }>
+ bar: #<ActiveSupport::Duration:0x00007fd7a3e3d208 {
+ @parts=nil,
+ @value=60
+ }>
}
A bit too verbose for my taste, but it doesn't suffer from either problems.
With or without require "super_diff/active_support"
doesn't make a difference.
2.2 Arrays
Failure/Error: it { expect([1.minute, 1.minute]).to contain_exactly(60.seconds, 59.seconds) }
expected collection contained: [59 seconds, 60 seconds]
actual collection contained: [1 minute, 1 minute]
the missing elements were: [59 seconds]
the extra elements were: [1 minute]
Surely, it would better look as
[
- 59 seconds,
+ 1 minute,
60 seconds
]
For comparison, super_diff
's output:
[
#<ActiveSupport::Duration:0x00007fd7a31c6c68 {
@parts=nil,
@value=60
}>,
+ #<ActiveSupport::Duration:0x00007fd7a31c6ba0 {
+ @parts=nil,
+ @value=60
+ }>,
- #<ActiveSupport::Duration:0x00007fd7a31c6808 {
- @parts=nil,
- @value=59
- }>,
]
Again, quite verbose.
Speaking of custom inspectors, I'd love to highlight @benoittgt's concern:
use invalid custom inspector but doesn't know it is used in diff
The failure message output for eq
/eql
/equal
/be
matchers is identical, but their comparison is different. It might be surprising to see:
Failure/Error: it { expect(1.minute).to equal(60.seconds) }
Diff:
- 60
+ 60
super_diff
for example:
Failure/Error: it { expect(1.minute).to equal 60.seconds }
Expected #<ActiveSupport::Duration:0x00007fa69a55a030 @parts=nil, @value=60>
to equal #<ActiveSupport::Duration:0x00007fa69a559f40 @parts=nil, @value=60>
# /Users/pirj/.rvm/gems/ruby-2.7.2@cowboy-web/gems/super_diff-0.8.0/lib/super_diff/rspec/monkey_patches.rb:43:in `handle_failure'
And stock RSpec wins the race here:
expected #<ActiveSupport::Duration:60840> => 60 seconds
got #<ActiveSupport::Duration:60940> => 1 minute
Compared using equal?, which compares object identity, but expected and actual are not the same object. Use
`expect(actual).to eq(expected)` if you don't care about object identity in this example.
Diff:
@@ -1 +1 @@
-60 seconds
+1 minute
For those issues, custom differs could be an option, but it's quite a long way to catch up with super_diff
[[1], [2]].
@benoittgt that makes sense. But I think a potential pitfall with appending a (inspected with CustomInspector)
to custom inspector diffs directly in ObjectFormatter
is that it a makes it more difficult to customize diff output when the objective is to make two objects from different classes which are considered equivalent to be displayed in the same way in diffs.
For example, let's say I want ActiveSupport::Duration
to be inspected just like Integer
since for most intents and purposes it behaves like an integer . That is, 1.hour == 3600
returns true
so it would be nice to be able to customize durations to be inspected the same way as integers for failing specs output only. For example, currently if I write the following assertion:
hash1 = {
travel_duration: 1.hour,
meal_duration: 2.hours
}
hash2 = {
travel_duration: 3600,
meal_duration: 7000
}
expect(hash1).to eq(hash2)
it fails with:
Failure/Error: expect(hash1).to eq(hash2)
expected: {:meal_duration=>7000, :travel_duration=>3600}
got: {:meal_duration=>2 hours, :travel_duration=>1 hour}
(compared using ==)
Diff:
@@ -1,3 +1,3 @@
-:meal_duration => 7000,
-:travel_duration => 3600,
+:meal_duration => 2 hours,
+:travel_duration => 1 hour,
In this case, only meal_duration
is failing the assertion, but travel_duration
is appearing in the diff as well. If we appended a (inspected with DurationInspector)
which is an inspector which can_inspect?
only returns true to instances of ActiveSupport::Duration
, then we still end up with a red herring for travel_duration
in the diff:
Diff:
@@ -1,3 +1,3 @@
-:meal_duration => 7000,
-:travel_duration => 3600,
+:meal_duration => 7200 (inspected with DurationInspector),
+:travel_duration => 3600 (inspected with DurationInspector),
I think this means that to cover this case without bigger changes we'd need to use the same inspector for both ActiveSupport::Duration
and Integer
in order for these to now show up in the diff?
With that in mind I'd say that my opinion is that a note about which inspector was used probably doesn't belong in ObjectFormatter
. It could probably be added somewhere else (maybe in the Differ
?) so that it lists all custom inspectors used, but I'm not sure how much work that would be and how much scope it would add here.
@pirj thanks for the detailed feedback and all of the examples. I definitely agree and I wasn't really thinking of the object identity case here so I do see how that could be a potential pitfall.
That being said though, the objective here was just to provide a more friendly interface to customize object inspection for spec purposes only.
For instance, right now if I have an assertion which compares hashes where all the values are instances of ActiveSupport::Duration
, it can become difficult to tell what is failing the assertion. This is because inspect
for these objects really doesn't convey the true nature of the information which they represent (number of seconds as an integer):
(2.hours - 60.minutes) == 1.hour
#=> true
(2.hours - 60.minutes) == 3600
#=> true
(2.hours - 60.minutes).inspect
#=> "2 hours and -60 minutes"
1.hour.inspect
#=> "1 hour"
3600.inspect
#=> "3600"
So adding the ability to register a custom inspector is really just a way to enable a custom representation of an object for spec comparisons only (without the need to monkey-patch a class' #inspect
in a spec helper), if there is a wish to do so.
As mentioned in the description this is actually currently possible to do as a "hack", by prepending a custom inspector to INSPECTOR_CLASSES
, like this:
RSpec::Support::ObjectFormatter::INSPECTOR_CLASSES.unshift(MyCustomInspector)
So the main goal of this PR is just to make this a better interface and potentially open up a path forward to do something like what is mentioned here, e.g.:
RSpec.configure do |config|
config.inspectors.register(MyCustomInspectorForDuration)
config.inspectors.register(MyCustomInspectorForSomeOtherObject)
# etc
end
From my (rather subjective and opinionated) point of view, objects should adhere to some protocol that consumers can rely on. In this case, it's "return the internal representation for inspect
that is helpful in development, return business representation for to_s
, for runtime". And ActiveSupport::Duration
does its job very well on this front.
Unfortunately, Ruby is not so formalized as other languages, and this statement has no reinforcement I know of.
I'm personally reluctant to decorate objects for diffing purposes, especially if it seems possible to keep the presentation under control by adhering to the above rule. Given that the differ is matcher-aware and doesn't solely rely on the a.inspect == b.inspect
.
In any case, the specs are too specific and don't indicate clearly what the difference custom inspectors make. I would ask to add more specs, but don't really want to waste your time. If you are dedicated to improve differ's output, I'd suggest improving the output for Hash/Array, for cases described above. It's a sensitive topic, though, since various CI/IDEs relying on the current output format.
Thanks @pirj that's great feedback. While I agree with your opinion that objects should adhere to some protocol that consumers can rely on, my opinion is that it's not easy (if at all possible) to keep the presentation under control just by adhering to the protocol mentioned above. In fact, the ActiveSupport::Duration
object is a great example where that protocol is followed but the Differ
and ObjectFormatter
currently don't do a great job in presenting the differences because it's not easy to do so. Even if it was possible though, there are probably countless codebases with with objects that don't follow this pattern out there, and I'd be reluctant to implement something that assumes all objects follow that protocol and end up with a worse presentation than what is currently done.
From my (also very subjective and opinionated) point of view, it's better to allow users to customize the output to be the way they want it for specific objects than to introduce breaking changes in presentation that assume that an object should be sometimes presented with inspect
and sometimes presented with some other method (e.g. to_s
), which I think could be more confusing.
In a separate point, I checked what the differ does with regards to the object identity and it seems to handle that case pretty well since it shows that they are different objects:
expected #<ActiveSupport::Duration:16960> => 90 seconds
got #<ActiveSupport::Duration:16980> => 90 seconds
Compared using equal?, which compares object identity,
but expected and actual are not the same object. Use
`expect(actual).to eq(expected)` if you don't care about
object identity in this example.
This would still be pretty readable with the custom inspector:
expected #<ActiveSupport::Duration:16960> => 90
got #<ActiveSupport::Duration:16980> => 90
Compared using equal?, which compares object identity,
but expected and actual are not the same object. Use
`expect(actual).to eq(expected)` if you don't care about
object identity in this example.
Anyway, I'm happy to add more test cases here if that will help push this forward, but also don't want to waste more time if this is not going to be considered. In my opinion, while this is definitely not a complete solution it's a step in the right direction. If there's disagreement there then there's probably no point in continuing to work on this, but if we agree that this can provide value then I can definitely add more tests here. If that's the case just let me know what kind of specs you would like to see here.
@b-loyola One more example I can think of.
expect(state_change_margins).to eq([0.celsius, 100.celsius]) # celsius
expected collection contained: [0'C, 100'C] (inspected with TemperatureInspector)
actual collection contained: [-10, 100]
Should there only be just one base representation for inspection in diff? What if the object is comparable with an integer (`100`) and a string (`100 F`), should we inspect is as a string, or as an integer?
Personally, I'm not convinced by the idea. However, my opinion counts last, as I have the least experience with differs code.
Let's see what @benoittgt and @JonRowe think.
should we inspect is as a string, or as an integer?
@pirj that's kind of my point here. I don't think it's rspec's job to know how to represent every object out there, and allowing different codebases to customize how to represent objects for only for diffing purposes is what this PR is all about (or at least paving way for doing so). If in one codebase there is an object which is usually compared with integers, registering a custom inspector for that object to be displayed as an integer might make sense. If in another codebase there's an object that is usually compared to strings, there might be a desire to customize how that object is inspected for diffing. And in most cases they will probably just want what the default behaviour is, and let the representation just follow what inspect
for the object returns - that's fine too, nothing changes in this case - if an inspector is not registered there would be no changes to this behaviour.
And to voice my (again subjective) opinion here, while I know this is possible I kind of disagree with the idea of an object that can be both equal to 100
(integer) and "100'C"
(string) because - and maybe javascript developers will disagree with this - to me it doesn't make sense for an object to be equal to both an integer and a string, specially when the string an integer will not equal each other. However, I don't think any of this is in the scope of the change proposed here - these cases are very hard to deal with and I'd say the rspec differ probably doesn't need to support that. If you really wanted to support this though, someone could propose it in a separate PR - the change proposed here is not mutually exclusive with that.
Is it possible to achieve something similar and file-local via refinements?
Not sure where to put "using" though.
Below did not work
module Inspection
refine Whatever do
def inspect
"custom inspection"
end
end
end
# ObjectFormatter is the class that send "inspect" to Whatever's – I'm assuming it should pull the refinement?
RSpec::Support::ObjectFormatter.class_exec do
using Inspection
end
describe Whatever do
using Inspection
example do
puts Whatever.new.inspect # => "custom inspection"
expect([]).to include(Whatever.new) # => generic ... expected [#<Whatever x: nil>] got [] ...
end
end
https://github.com/halostatue/diff-lcs/issues/70 semi-related
Closing as this is probably stale now, can reopen if there's any interest.
Subject of the issue
When comparing objects nested in diffable objects (e.g.
Hash
), it can be difficult to differentiate between objects which are failing the comparison and objects which are considered equal but have different outputs forinspect
. One notable example isActiveSupport::Duration
where60.minutes
is considered equivalent to1.hour
, but produces different output forinspect
which can cause confusion and seem like it is failing the assertion when it is in fact just failing the inspect comparison.The proposal is to allow inspectors to be registered in
ObjectFormatter
so that custom inspectors can be added for these classes, to improve the signal to noise ratio in diffs when looking at failed tests.At the moment it is possible to 'hack' this by manually prepending a custom inspector class to
RSpec::Support::ObjectFormatter::INSPECTOR_CLASSES
, but here we create a more friendly interface to allow this through something like:If this gains enough traction this could be expanded to allow for configuring this in RSpec's configuration, perhaps in a similar way to what is mentioned here.
Related to https://github.com/rspec/rspec-expectations/issues/97
Your environment
Steps to reproduce
To see an example of failing specs which provide misleading diffs, run this example and note how in the
'does not show in the diff when objects are equivalent'
example it seems that bothobject1
andobject2
are failing the assertion, when in fact onlyobject2
is failing the assertion.Expected behavior
We should be able to register a custom inspector, to be able to modify the output of inspect for specific objects, so that it becomes easier to tell what is actually failing the assertion:
This would mean that after registering the custom inspector above, the diff would look like this:
Actual behavior
Diffs are based on the raw outoput of
inspect
, so it is hard to tell that the value inobject1
is not actually failing the assertion, and that only the value inobject2
is the issue: