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

Fix false-negative for UnspecifiedException when matcher is chained #1940

Merged
merged 1 commit into from
Jul 20, 2024

Conversation

r7kamura
Copy link
Contributor

I noticed that this cop could not detect the following cases, so fixed it.

expect {
  foo
}.to raise_exception.and change { bar }

Before submitting the PR make sure the following are checked:

  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Updated documentation.
  • Added an entry to the CHANGELOG.md if the new code introduces user-observable changes.
  • The build (bundle exec rake) passes (be sure to run this locally, since it may produce updated documentation that you will need to commit).

@r7kamura r7kamura requested a review from a team as a code owner July 17, 2024 20:32
@r7kamura r7kamura force-pushed the unspecified-exception-chain branch from 419da80 to 2858671 Compare July 18, 2024 17:08
@r7kamura r7kamura force-pushed the unspecified-exception-chain branch from 2858671 to 60199ba Compare July 18, 2024 17:37
return false unless node&.block_type?
expect_to = find_expect_to(node)
return false unless expect_to
return false if expect_to.block_node&.arguments?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this checking expect { |args| … }.to raise_error?
Why do we ignore if there are any, how do we care?
Do we have a an example that will fail if we remove this check?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want to test the raised error in detail, to may be called with a block with an argument as follows:

expect { foo }.to raise_error do |error|
  expect(error.bar).to eq("baz")
end

If this code is removed, the following test will fail:

it 'allows classes with blocks with do/end' do
expect_no_offenses(<<~RUBY)
expect {
raise StandardError.new('error')
}.to raise_error do |error|
error.data
end
RUBY
end

Copy link
Member

@pirj pirj Jul 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood.
Such syntax is confusing as hell. The block is not passed to to, but raise_error itself, isn’t it?

I recall there’s some poke-yoke warning in RSpec warning users when the block goes actually to the wrong one.
Ps and mistake happens if you change do/end to {} 🙈

However, yes, I now recall that we decided that such syntax should be tolerated as it is expected that aome check agains what the exception is will be made inside that block.

Copy link
Member

@pirj pirj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

Copy link
Member

@ydah ydah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@pirj pirj merged commit 9eb23a6 into rubocop:master Jul 20, 2024
22 of 24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants