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

Edge cases in Minitest/EmptyLineBeforeAssertionMethods #189

Closed
flavorjones opened this issue Oct 31, 2022 · 5 comments · Fixed by #195
Closed

Edge cases in Minitest/EmptyLineBeforeAssertionMethods #189

flavorjones opened this issue Oct 31, 2022 · 5 comments · Fixed by #195
Labels
bug Something isn't working

Comments

@flavorjones
Copy link
Contributor

The Minitest/EmptyLineBeforeAssertionMethods cop has some behaviors related to blocks (like #each loops) that I think either result in unnecessary warnings or missed warnings.

Example

require "minitest/autorun"
require "set"

class FooTest < MiniTest::Spec
  def test_foo
    set = Set.new([1,2,3])
    refute_nil(set)
    set.each do |thing|
      refute_nil(thing)
    end
    assert_equal(6, set.inject(:+))
  end

  def test_bar
    set = Set.new([1,2,3])
    set.each do |thing|
      refute_nil(thing)
    end
  end
end

Actual behavior

test/test_foo.rb:7:5: C: [Correctable] Minitest/EmptyLineBeforeAssertionMethods: Add empty line before assertion.
    refute_nil(set)
    ^^^^^^^^^^^^^^^
test/test_foo.rb:11:5: C: [Correctable] Minitest/EmptyLineBeforeAssertionMethods: Add empty line before assertion.
    assert_equal(6, set.inject(:+))
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Expected behavior

In test_foo, I do expect the warning on line 7, but I do not expect the warning on line 11.

In test_bar, I would expect a warning to appear before the set.each line.

In situations like this, I expect that a block containing only assertions should be treated as if it was an assertion.

RuboCop version

1.37.1 (using Parser 3.1.2.1, rubocop-ast 1.23.0, running on ruby 3.1.2) [x86_64-linux]
  - rubocop-minitest 0.23.0
@movermeyer
Copy link

Similarly, in this case:

test "i18n load path yaml files are formatted properly" do
  yaml_load_paths.each do |path|
    YAML.load_file(path)
  rescue Psych::Exception => e
    flunk("Error loading #{path}: #{e.inspect}")
  end
end

I would not expect Minitest/EmptyLineBeforeAssertionMethods to find an offense.

@flavorjones
Copy link
Contributor Author

To clarify @movermeyer's comment, that test generates this offense:

test/test_foo.rb:25:7: C: [Correctable] Minitest/EmptyLineBeforeAssertionMethods: Add empty line before assertion.
      flunk("Error loading #{path}: #{e.inspect}")
      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

koic added a commit to koic/rubocop-minitest that referenced this issue Nov 1, 2022
Fixes part of rubocop#189.

This PR fixes a false negative for `Minitest/EmptyLineBeforeAssertionMethods`
when using non assertion method statement before assertion method used in a block.
koic added a commit to koic/rubocop-minitest that referenced this issue Nov 1, 2022
Fixes part of rubocop#189.

This PR fixes a false negative for `Minitest/EmptyLineBeforeAssertionMethods`
when using non assertion method statement before assertion method used in a block.
@koic koic added the bug Something isn't working label Nov 1, 2022
koic added a commit to koic/rubocop-minitest that referenced this issue Nov 4, 2022
Follow up rubocop#189 (comment).

This PR fixes a false positive for `Minitest/EmptyLineBeforeAssertionMethods`
when using `rescue` before assertion method.
koic added a commit to koic/rubocop-minitest that referenced this issue Nov 4, 2022
…ssertionMethods`

Fixes rubocop#189 and rubocop#194.

This PR fixes a false negative for `Minitest/EmptyLineBeforeAssertionMethods`
when using assertion method used in block before assertion method.
@koic koic closed this as completed in #195 Nov 5, 2022
koic added a commit that referenced this issue Nov 5, 2022
…ty_line_before_assertion_methods

[Fix #189] Fix a false positive for `Minitest/EmptyLineBeforeAssertionMethods`
@MatzFan
Copy link

MatzFan commented Sep 18, 2023

With rubocop-minitest 0.31.1 I'm still getting a false positive for Minitest/EmptyLineBeforeAssertionMethods on this code for the refute_nil(thing) within the block.

require "minitest/autorun"
require "set"

class FooTest < MiniTest::Spec
  def test_bar
    set = Set.new([1,2,3])
    set.each do |thing|
      refute_nil(thing)
    end
  end
end

@koic
Copy link
Member

koic commented Sep 19, 2023

@MatzFan It looks like it's an expected behavior and doesn't have a false positive:
https://github.com/rubocop/rubocop-minitest/blob/v0.31.1/test/rubocop/cop/minitest/empty_line_before_assertion_methods_test.rb#L130-L150

If you have different intentions regarding the above test, please open a new issue based on the template. Thank you.

@MatzFan
Copy link

MatzFan commented Sep 19, 2023

Apologies, my mistake in not realizing I need to put a space before the block containing an assertion. Sorry for the false negative!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants