-
-
Notifications
You must be signed in to change notification settings - Fork 395
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Factorize PredicateMatcher. Improve failure message for Has
.
#1195
Conversation
fc5c69a
to
f16bd15
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you split this change up into two PRs please, I'd like to assess any refactoring of the matchers without change to their output.
Improve example in predicates.feature
@JonRowe rebased, ready for review & merging |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks for the side change on feature test. It is much clearer like that.
I have concerns about inspecting the target in the message here, if the goal is to be consistent I'd almost rather go the other way and make |
The goal is to remove 30 lines of mostly redundant code with a misleading failure message to boot.
I don't understand your concern. Please let me what I can do, as I have other yaks waiting... |
@JonRowe What changes (if any) would you like for this to be merged? |
I played around with this some and couldn't produce a better output without drastically changing our current behaviour in specs, so merged as is. |
Super thanks 🎉 |
@@ -65,15 +65,15 @@ def obj_with_block_method.has_some_stuff?; yield; end | |||
def o.has_some_stuff?; false; end | |||
expect { | |||
expect(o).to have_some_stuff | |||
}.to fail_with("expected #has_some_stuff? to return true, got false") | |||
}.to fail_with("expected `#{o.inspect}.has_some_stuff?` to return true, got false") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit out of context. Where does the inspect
part come from? I would expect the message to be "expected o.has_some_stuff?
to return true, got false". Is that something you were referring to in "produce a better output"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its because actual
e.g o
is being printed by the matcher
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. I didn't parse it at first.
* Remove outdated option rspec/rspec-core@0407831 * Use the non-globally exposed RSpec syntax rspec/rspec-core#2803 * Use the non-globally exposed RSpec syntax, cont rspec/rspec-core#2803 * Comply to strict predicate matchers See: - rspec/rspec-expectations#1195 - rspec/rspec-expectations#1196 - rspec/rspec-expectations#1277
This PR improves
Has
's output and makes it consistent withBePredicate
:Fixes the failure message where the result was not true or false. It used to say ", got true/false" no matter what the results are. So it could say "expected ... to return false, got true" when in actuality it's expecting the result to be falsey (upcoming PR) and it got a result of
42
!Failure message for now outputs the receiver.
The description:
becomes:
Misc: I also changed a feature example because it was showing a custom matcher
has_all_string_keys
which, while instructive, could be misinterpreted as an actually useful matcher when the builtin matchers are much better here (e.g.all(be_a String)
)This is a prelude to an upcoming PR.