rspec / rspec-expectations

Provides a readable API to express expected outcomes of a code example
https://rspec.info
MIT License
1.26k stars 397 forks source link

Please return true/false from aggregate_failures in a custom matcher #1223

Closed henrahmagix closed 3 years ago

henrahmagix commented 3 years ago

Subject of the issue

Make aggregate_failures easier to use in custom matchers defined as groups of expectations. Currently it returns nil, so you must end the match block with true else the matcher will always fail.

The steps below are simplified, and it could be argued that it should be separate matchers, or move the aggregate_failures block into the example.

However, I'm currently using this with the all matcher on an array of differing length (shared examples), so I need to aggregate_failures around each item of the array to make it easier to spot which items in the array are failing. In this way, it's not possible to group expectations into a custom matcher and run all the assertions before failing.

Your environment

Steps to reproduce

context 'with custom matcher' do
  matcher :be_custom do
    match(notify_expectation_failures: true) do |actual|
      aggregate_failures do
        expect(actual).to start_with 'c'
        expect(actual).to end_with 'm'
        expect(actual).to include 'usto'
      end
    end
  end

  it { expect('custom').to be_custom }
end

Expected behavior

Test passes ☺️

Actual behavior

Test fails 😢

with custom matcher
  is expected to be custom (FAILED - 1)

Failures:

  1) with custom matcher is expected to be custom
     Failure/Error: fit { expect('custom').to be_custom }
       expected "custom" to be custom
     # ./spec/custom_spec.rb:12:in `block (2 levels) in <top (required)>'

Finished in 0.02801 seconds (files took 0.16621 seconds to load)
1 example, 1 failure

Failed examples:

rspec ./spec/custom_spec.rb:12 # with custom matcher is expected to be custom

Custom fix in the meantime

Return true after the aggregate_failures block. However, this means the matcher cannot be used as negated 😞

context 'with custom matcher' do
  matcher :be_custom do
    match(notify_expectation_failures: true) do |actual|
      aggregate_failures do
        expect(actual).to start_with 'c'
        expect(actual).to end_with 'm'
        expect(actual).to include 'usto'
      end
      true # how can we detect success or failure in the block above?
    end
  end

  it { expect('custom').to    be_custom } # passes
  it { expect('hello').not_to be_custom } # fails
end

Thanks very much for an awesome testing library ☺️ 🎉

pirj commented 3 years ago

I have never seen expect being used inside of a match block, and couldn't find such usage in the documentation.

You may try to use the following instead:

  matcher :be_custom do
    def custom
      @matcher ||=
        start_with('c')
          .and end_with('m')
          .and include('usto')
    end

    match do |actual|
      custom.matches?(actual)
    end

    failure_message do |actual|
      custom.failure_message
    end
  end
     Failure/Error: it { expect('111').to be_custom }
          expected "111" to start with "c"
       ...and:
             expected "111" to end with "m"
          ...and:
             expected "111" to include "usto"

No notify_expectation_failures/aggregate_failures needed.

Don't forget to define failure_message_when_negated and match_when_negated if you plan to use it with not_to.

Please feel free to reopen if I'm missing something.

Thanks very much for an awesome testing library

🙇 🤗

henrahmagix commented 3 years ago

Hi @pirj,

Thanks for the possible solution :)

I didn’t realise you can get failure messages from the matchers! That may work for me.

However, my understanding was that notify_expectation_failures: true exists so that expect can be used in custom matchers?

pirj commented 3 years ago

You're right, it seems to have to work.

I hope your real use case is not as complicated and the proposed solution works in some form. If it does not - please reopen and provide more details, we'll figure it out.

JonRowe commented 3 years ago

main contains a fix for this if you're interested

henrahmagix commented 3 years ago

Aaahhh this is so nice, thanks everyone for reopening it and fixing it! Much love 💖

benoittgt commented 3 years ago

Fixed by https://github.com/rspec/rspec-expectations/pull/1225