Skip to content
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

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

Closed
pirj opened this issue Dec 2, 2020 · 4 comments · Fixed by #452
Closed
Labels
Milestone

Comments

@pirj
Copy link
Member

pirj commented Dec 2, 2020

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

  • Ruby version: 2.7.1
  • rspec-expectations version: d1239222 (main)

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
Copy link
Member Author

pirj commented Dec 2, 2020

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
Copy link
Member

JonRowe commented Dec 2, 2020

That approach looks reasonable, is there a PR?

@pirj
Copy link
Member Author

pirj commented Dec 6, 2020

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 pirj transferred this issue from rspec/rspec-expectations Dec 15, 2020
@pirj
Copy link
Member Author

pirj commented Dec 15, 2020

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants