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
Fix predicates to return actual true/false values #2756
Conversation
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.
👍
@@ -87,6 +87,9 @@ def handle_current_dir_change | |||
c.expect_with :rspec do |expectations| | |||
expectations.include_chain_clauses_in_custom_matcher_descriptions = true | |||
expectations.max_formatted_output_length = 1000 | |||
if expectations.respond_to? :strict_predicate_matchers= | |||
expectations.strict_predicate_matchers = true | |||
end |
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 guess we can skip that, and add separately when rspec/rspec-expectations#1196 is merged. Otherwise, we'll have to remove the check later on anyway.
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.
It's a chicken and egg problem. Without this, rspec/rspec-expectations#1196 will break the build as it introduces changes in the failure messages in this spec suite.
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 went to try to add specs to the predicates, but they are always booleans as far as I can see, you can set pending_fixed
manually, but it seems the only change needed is the predicate config due to the changed message. I'm happy to have just that in this PR and a later PR to remove the check?
I don't know how you tried, or what you tried, or why you tried adding specs to the predicates (when Just try cherrypicking the last commit and run it with my (as of yet unmerged) You will get the following errors:
Of course, this is what led me to change the aliases for |
1b7c5f1
to
3f0cc77
Compare
Changed to initialize |
To get round the chicken and the egg problem, if the predicates here are strict then other build will pass, typically when we change the behaviour like this, we add specs that are explicit and fail without the change, so in this case having a spec that will fail on main and pass on this branch without the changes of rspec-expectations. It seems that although pending does seperate message out to elsewhere and overwrite it, skip does keep the message in metadata which I overlooked, so that predicate |
3f0cc77
to
3e8b6b1
Compare
Thanks for your patience here, I actually reverted to your original commit once I added some proper specs as I was able to find a non boolean value (e.g. the nil state) that actually is valid, and simply setting false implies a setting of metadata keys I was uncomfortable with. Closing this as merged by #2758, but we'll need a follow up to enable strict predicates later on. |
Ok. How do you propose to go about rspec/rspec-expectations#1196 ? Merge (breaking temporarily the build) and then changing the expected failure messages in this gem, until fix predicates is turned on one day? |
Apologies I overlooked the error message problem (I intended to fix it) a follow up in #2759 should resolve that as my original intent was :) |
This commit was imported from rspec/rspec-core@456388f.
This PR fixes
pending?
,skipped?
andpending_fixed?
so they returntrue
/false
values.It also turns on
expectations.strict_predicate_matchers
config; with this new config, some specs were failing because these predicates were not returning the right values.Precursor to rspec/rspec-expectations#1196