rspec / rspec-support

Common code needed by the other RSpec gems. Not intended for direct use.
https://rspec.info
MIT License
98 stars 103 forks source link

Internal affairs: `expect_warn_deprecation` matches any message when there's `fail` in the example #451

Closed pirj closed 3 years ago

pirj commented 3 years ago

Subject of the issue

expect_warn_deprecation matches any message when there's raise_error(RSpec::Expectations::ExpectationNotMetError) in the example. Might affect other rspec-support deprecation matcher helpers.

Your environment

Steps to reproduce

Original code:

  it 'prints a deprecation warning when given a value and negated' do
    expect_warn_deprecation(/complete nonsense/)
    expect { expect(3).not_to matcher }.to fail
  end

Simplification:

  it do
    expect_warn_deprecation(/foo/)
    expect {
      RSpec.configuration.reporter.deprecation(message: 'bar')
      expect(1).to change { 1 }
    }.to raise_error(RSpec::Expectations::ExpectationNotMetError)
  end

Expected behavior

       expected StandardError, got #<RSpec::Expectations::ExpectationNotMetError: expected "bar" to match /foo/
       Diff:
       @@ -1 +1 @@
       -/foo/
       +"bar"

or

       expected "bar" to match /foo/
       Diff:
       @@ -1 +1 @@
       -/foo/
       +"bar"

Actual behavior

  is expected to raise RSpec::Expectations::ExpectationNotMetError

1 example, 0 failures

:aggregate_failures fixes the issue with the first expectation, it is now printed, but now the inner failure is propagated.

pirj commented 3 years ago

The use of non-nested expectation resolves the issue:

    expect(RSpec.configuration.reporter).to
      receive(:deprecation).with(include(message: match(/Tahe/)))

just for the reference, original implementation:

  def expect_warn_deprecation(snippet=//)
    expect(RSpec.configuration.reporter).to receive(:deprecation) do |options|
      message = options[:message]
      expect(message).to match(snippet)
    end
  end
JonRowe commented 3 years ago

That approach looks reasonable, is there a PR?

pirj commented 3 years ago

There are no tests for the whole file, it's tricky to write, takes me a while. Since we're nearing that moment when deprecation messages are becoming extremely important to properly cover, I'll handle this ASAP.

pirj commented 3 years ago

I fail to write a test that would fail for the current implementation 😬 Would you accept such a PR?

Along the way I discovered that

around { |example| in_sub_process { example.run } }

doesn't work, marks all examples as pending.