Fix predicates to return actual true/false values

[marcandre]

This PR fixes pending?, skipped? and pending_fixed? so they return true / 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.

rspec ./spec/rspec/core/metadata_spec.rb:104 # RSpec::Core::Metadata for an example creates an empty execution result
rspec ./spec/rspec/core/example_group_spec.rb:1126 # RSpec::Core::ExampleGroup skip with message in metadata generates a skipped example
rspec ./spec/rspec/core/example_group_spec.rb[1:36:1] # RSpec::Core::ExampleGroup.xit generates a skipped example
rspec ./spec/rspec/core/example_group_spec.rb[1:38:1] # RSpec::Core::ExampleGroup.xexample generates a skipped example
rspec ./spec/rspec/core/example_group_spec.rb[1:37:1] # RSpec::Core::ExampleGroup.xspecify generates a skipped example

Precursor to rspec/rspec-expectations#1196

[pirj]

👍

[pirj]

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.

[marcandre]

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.

[JonRowe]

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?

[marcandre]

I don't know how you tried, or what you tried, or why you tried adding specs to the predicates (when strict_predicates does it for you). Clearly pending_fixed

Just try cherrypicking the last commit and run it with my (as of yet unmerged) strict_predicates for rspec-expectation.

You will get the following errors:

Failures:

  1) Aggregating failures for a non-pending example via `:aggregate_failures` metadata applies `aggregate_failures` to examples or groups tagged with `:aggregate_failures`
     Failure/Error: expect(ex.execution_result).not_to be_pending_fixed
       expected `#<RSpec::Core::Example::ExecutionResult:0x00007fc9c4633d60 @started_at=2020-09-02 14:49:57.709784 -0400, @status=:failed, @finished_at=2020-09-02 14:49:57.71033 -0400, @run_time=0.000546, @exception=#<RSpec::Expectations::MultipleExpectationsNotMetError: RSpec::Expectations::MultipleExpectationsNotMetError>>.pending_fixed?` to return false, got nil
     Shared Example Group: "failure aggregation" called from ./spec/rspec/core/aggregate_failures_spec.rb:125
     # ./spec/rspec/core/aggregate_failures_spec.rb:70:in `block (4 levels) in <top (required)>'
     # ./spec/support/sandboxing.rb:16:in `block (3 levels) in <top (required)>'
     # ./spec/support/sandboxing.rb:7:in `block (2 levels) in <top (required)>'

  2) RSpec::Core::ExampleGroup.xexample generates a skipped example
     Failure/Error: expect(self.group.examples.first).to be_skipped
       expected `#<RSpec::Core::Example "is pending">.skipped?` to return true, got "Temporarily skipped with xexample"
     # ./spec/rspec/core/example_group_spec.rb:1145:in `block (4 levels) in <module:Core>'
     # ./spec/support/sandboxing.rb:16:in `block (3 levels) in <top (required)>'
     # ./spec/support/sandboxing.rb:7:in `block (2 levels) in <top (required)>'

  3) RSpec::Core::ExampleGroup.xspecify generates a skipped example
     Failure/Error: expect(self.group.examples.first).to be_skipped
       expected `#<RSpec::Core::Example "is pending">.skipped?` to return true, got "Temporarily skipped with xspecify"
     # ./spec/rspec/core/example_group_spec.rb:1145:in `block (4 levels) in <module:Core>'
     # ./spec/support/sandboxing.rb:16:in `block (3 levels) in <top (required)>'
     # ./spec/support/sandboxing.rb:7:in `block (2 levels) in <top (required)>'

  4) RSpec::Core::ExampleGroup.xit generates a skipped example
     Failure/Error: expect(self.group.examples.first).to be_skipped
       expected `#<RSpec::Core::Example "is pending">.skipped?` to return true, got "Temporarily skipped with xit"
     # ./spec/rspec/core/example_group_spec.rb:1145:in `block (4 levels) in <module:Core>'
     # ./spec/support/sandboxing.rb:16:in `block (3 levels) in <top (required)>'
     # ./spec/support/sandboxing.rb:7:in `block (2 levels) in <top (required)>'

  5) RSpec::Core::ExampleGroup skip with message in metadata generates a skipped example
     Failure/Error: expect(self.group.examples.first).to be_skipped
       expected `#<RSpec::Core::Example "skip this">.skipped?` to return true, got "not done"
     # ./spec/rspec/core/example_group_spec.rb:1128:in `block (3 levels) in <module:Core>'
     # ./spec/support/sandboxing.rb:16:in `block (3 levels) in <top (required)>'
     # ./spec/support/sandboxing.rb:7:in `block (2 levels) in <top (required)>'

Finished in 23.7 seconds (files took 1.56 seconds to load)
2180 examples, 5 failures, 3 pending

Failed examples:

rspec ./spec/rspec/core/aggregate_failures_spec.rb[1:1:2:1] # Aggregating failures for a non-pending example via `:aggregate_failures` metadata applies `aggregate_failures` to examples or groups tagged with `:aggregate_failures`
rspec ./spec/rspec/core/example_group_spec.rb[1:38:1] # RSpec::Core::ExampleGroup.xexample generates a skipped example
rspec ./spec/rspec/core/example_group_spec.rb[1:37:1] # RSpec::Core::ExampleGroup.xspecify generates a skipped example
rspec ./spec/rspec/core/example_group_spec.rb[1:36:1] # RSpec::Core::ExampleGroup.xit generates a skipped example
rspec ./spec/rspec/core/example_group_spec.rb:1126 # RSpec::Core::ExampleGroup skip with message in metadata generates a skipped example

Of course, this is what led me to change the aliases for def with !!. Probably pending_fixed could be initialized at false, I'm not sure about the rest.

[marcandre]

Changed to initialize @pending_fixed as false.

[JonRowe]

or why you tried adding specs

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 !! is necessary.

[JonRowe]

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.

[marcandre]

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?

[JonRowe]

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 :)