Cop which detects ensure is called even if it is skipped #169

yahonda · 2022-05-19T23:53:19Z

Is your feature request related to a problem? Please describe.

I want a cop that detects ensure is called even if this test has skip.
Here is an example.

foo.rb

# frozen_string_literal: true

require 'bundler/inline'

gemfile(true) do
  source 'https://rubygems.org'

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  # Activate the gem you are reporting the issue against.
  gem 'activesupport', '~> 7.0.0'
end

require 'active_support'
require 'active_support/core_ext/object/blank'
require 'minitest/autorun'

# Top level documentation comment
class BugTest < Minitest::Test
  def test_stuff
    skip 'this test is skipped.'
    assert 'zomg'.present?
    refute ''.present?
  ensure
    p 'ensure is called even if it is skipped.'
  end
end

Run foo.rb to show ensure is called even if this test is skipped

% ruby foo.rb
Fetching gem metadata from https://rubygems.org/........
Resolving dependencies...
Using concurrent-ruby 1.1.10
Using minitest 5.15.0
Using bundler 2.3.14
Using tzinfo 2.0.4
Using i18n 1.10.0
Using activesupport 7.0.3
Run options: --seed 43234

# Running:

"ensure is called even if it is skipped"
S

Finished in 0.000547s, 1828.1536 runs/s, 0.0000 assertions/s.

1 runs, 0 assertions, 0 failures, 0 errors, 1 skips

You have skipped tests. Run with --verbose for details.
%

RuboCop Minitest does not show offenses

% rubocop --require rubocop-minitest foo.rb

Inspecting 1 file
.

1 file inspected, no offenses detected
%

RuboCop and RuboCop Minitest versions

% rubocop -V
1.29.0 (using Parser 3.1.2.0, rubocop-ast 1.17.0, running on ruby 3.1.2 arm64-darwin21)
% gem info rubocop-minitest

*** LOCAL GEMS ***

rubocop-minitest (0.19.1, 0.17.0)
    Authors: Bozhidar Batsov, Jonas Arvidsson, Koichi ITO
    License: MIT
    Installed at (0.19.1): /Users/yahonda/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0
                 (0.17.0): /Users/yahonda/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0

    Automatic Minitest code style checking tool.
%

Describe the solution you'd like

I'd like to have some way to find offenses if test methods have ensure clause when they are skipped.

Describe alternatives you've considered

Change the test code to add some condition in ensure clause.

Additional context

I'm requesting this feature when I reviewed rails/rails#42691 that have a test that is skipped against MariaDB but ensure is called that has side effects.

The text was updated successfully, but these errors were encountered:

koic · 2022-05-20T02:20:28Z

Can you provide good example code and bad example code for this new cop?

yahonda · 2022-05-20T02:54:35Z

Can you provide good example code and bad example code for this new cop?

Sure.

rubocop_minitest_169.rb

# frozen_string_literal: true

require 'bundler/inline'

gemfile(true) do
  source 'https://rubygems.org'

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  # Activate the gem you are reporting the issue against.
  gem 'activesupport', '~> 7.0.0'
end

require 'active_support'
require 'active_support/core_ext/object/blank'
require 'minitest/autorun'

# Top level documentation comment
class BugTest < Minitest::Test
  # good
  def test_good_example
    skip 'this test is skipped.'
    assert 'zomg'.present?
    refute ''.present?
  ensure
    p 'ensure is called even if it is skipped.'
  end

  # bad
  def test_bad_example
    skip 'this test is skipped.'
    begin
      assert 'zomg'.present?
      refute ''.present?
    ensure
      p 'ensure is not called when it is skipped'
    end
  end
end

% ruby rubocop_minitest_169.rb
Fetching gem metadata from https://rubygems.org/........
Resolving dependencies...
Using minitest 5.15.0
Using bundler 2.3.14
Using concurrent-ruby 1.1.10
Using i18n 1.10.0
Using tzinfo 2.0.4
Using activesupport 7.0.3
Run options: --seed 53656

# Running:

S"ensure is called even if it is skipped."
S

Finished in 0.000671s, 2980.6259 runs/s, 0.0000 assertions/s.

2 runs, 0 assertions, 0 failures, 0 errors, 2 skips

You have skipped tests. Run with --verbose for details.
%

yahonda · 2022-05-20T04:24:50Z

My bad the good and bad examples are switched. Here is the correct one.

# frozen_string_literal: true

require 'bundler/inline'

gemfile(true) do
  source 'https://rubygems.org'

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  # Activate the gem you are reporting the issue against.
  gem 'activesupport', '~> 7.0.0'
end

require 'active_support'
require 'active_support/core_ext/object/blank'
require 'minitest/autorun'

# Top level documentation comment
class BugTest < Minitest::Test
  # good
  def test_good_example
    skip 'this test is skipped.'
    begin
      assert 'zomg'.present?
      refute ''.present?
    ensure
      p 'ensure is not called when it is skipped'
    end
  end

  # bad
  def test_bad_example
    skip 'this test is skipped.'
    assert 'zomg'.present?
    refute ''.present?
  ensure
    p 'ensure is called even if it is skipped.'
  end
end

Closes rubocop#169. This cop checks that `ensure` call even if `skip`. It is unexpected that `ensure` will be called when skipping test. If conditional `skip` is used, it checks that `ensure` is also called conditionally. On the other hand, it accepts `skip` used in` rescue` because `ensure` may be teardown process to `begin` setup process. ```ruby # bad def test_skip skip 'This test is skipped.' assert 'foo'.present? ensure do_something end # bad def test_conditional_skip skip 'This test is skipped.' if condition assert do_something ensure do_teardown end # good def test_skip skip 'This test is skipped.' begin assert 'foo'.present? ensure do_something end end # good def test_conditional_skip skip 'This test is skipped.' if condition assert do_something ensure if condition do_teardown end end # good def test_skip_is_used_in_rescue do_setup assert do_something rescue skip 'This test is skipped.' ensure do_teardown end ```

Closes rubocop#169. This cop checks that `ensure` call even if `skip`. It is unexpected that `ensure` will be called when skipping test. If conditional `skip` is used, it checks that `ensure` is also called conditionally. On the other hand, it accepts `skip` used in `rescue` because `ensure` may be teardown process to `begin` setup process. ```ruby # bad def test_skip skip 'This test is skipped.' assert 'foo'.present? ensure do_something end # bad def test_conditional_skip skip 'This test is skipped.' if condition assert do_something ensure do_teardown end # good def test_skip skip 'This test is skipped.' begin assert 'foo'.present? ensure do_something end end # good def test_conditional_skip skip 'This test is skipped.' if condition assert do_something ensure if condition do_teardown end end # good def test_skip_is_used_in_rescue do_setup assert do_something rescue skip 'This test is skipped.' ensure do_teardown end ```

koic · 2022-05-20T09:14:03Z

@yahonda I've opened #171 and confirmed with rails/rails repo.

diff --git a/Gemfile b/Gemfile
index eb755b90d0..1b04faca3c 100644
--- a/Gemfile
+++ b/Gemfile
@@ -37,7 +37,7 @@ gem "json", ">= 2.0.0"

 group :rubocop do
   gem "rubocop", ">= 1.25.1", require: false
-  gem "rubocop-minitest", require: false
+  gem "rubocop-minitest", require: false, github: 'koic/rubocop-minitest', branch: 'add_new_ensure_call_even_if_skip_cop'
   gem "rubocop-packaging", require: false
   gem "rubocop-performance", require: false
   gem "rubocop-rails", require: false

% bundle exec rubocop --only Minitest/EnsureCallEvenIfSkip
(snip)

Offenses:

activerecord/test/cases/connection_adapters/connection_handler_test.rb:434:9: C: Minitest/EnsureCallEvenIfSkip: ensure is called even though the test is skipped.
        ensure
        ^^^^^^
activerecord/test/cases/query_cache_test.rb:599:3: C: Minitest/EnsureCallEvenIfSkip: ensure is called even though the test is skipped.
  ensure
  ^^^^^^
activerecord/test/cases/query_cache_test.rb:628:3: C: Minitest/EnsureCallEvenIfSkip: ensure is called even though the test is skipped.
  ensure
  ^^^^^^
railties/test/engine/commands_test.rb:40:3: C: Minitest/EnsureCallEvenIfSkip: ensure is called even though the test is skipped.
  ensure
  ^^^^^^
railties/test/engine/commands_test.rb:50:3: C: Minitest/EnsureCallEvenIfSkip: ensure is called even though the test is skipped.
  ensure
  ^^^^^^
railties/test/engine/commands_test.rb:60:3: C: Minitest/EnsureCallEvenIfSkip: ensure is called even though the test is skipped.
  ensure
  ^^^^^^

3008 files inspected, 6 offenses detected

Maybe these are not false positives and look like the expected detection. Can you confirm it if you like?

Closes rubocop#169. This cop checks that `ensure` call even if `skip`. It is unexpected that `ensure` will be called when skipping test. If conditional `skip` is used, it checks that `ensure` is also called conditionally. On the other hand, it accepts `skip` used in `rescue` because `ensure` may be teardown process to `begin` setup process. ```ruby # bad def test_skip skip 'This test is skipped.' assert 'foo'.present? ensure do_something end # bad def test_conditional_skip skip 'This test is skipped.' if condition assert do_something ensure do_teardown end # good def test_skip skip 'This test is skipped.' begin assert 'foo'.present? ensure do_something end end # good def test_conditional_skip skip 'This test is skipped.' if condition assert do_something ensure if condition do_teardown end end # good def test_skip_is_used_in_rescue do_setup assert do_something rescue skip 'This test is skipped.' ensure do_teardown end ```

yahonda · 2022-05-20T23:13:07Z

I have confirmed the Minitest/EnsureCallEvenIfSkip finds offenses as I expected.

Closes rubocop#169. This cop checks that `ensure` call even if `skip`. It is unexpected that `ensure` will be called when skipping test. If conditional `skip` is used, it checks that `ensure` is also called conditionally. On the other hand, it accepts `skip` used in `rescue` because `ensure` may be teardown process to `begin` setup process. ```ruby # bad def test_skip skip 'This test is skipped.' assert 'foo'.present? ensure do_something end # bad def test_conditional_skip skip 'This test is skipped.' if condition assert do_something ensure do_teardown end # good def test_skip skip 'This test is skipped.' begin assert 'foo'.present? ensure do_something end end # good def test_conditional_skip skip 'This test is skipped.' if condition assert do_something ensure if condition do_teardown end end # good def test_skip_is_used_in_rescue do_setup assert do_something rescue skip 'This test is skipped.' ensure do_teardown end ```

[Fix #169] Add new `Minitest/SkipEnsure` cop

yahonda changed the title ~~Cop which~~ Cop which detects ensure is called even if it is skipped May 20, 2022

koic added the enhancement New feature or request label May 20, 2022

koic mentioned this issue May 20, 2022

[Fix #169] Add new Minitest/SkipEnsure cop #171

Merged

8 tasks

koic closed this as completed in #171 May 27, 2022

koic added a commit that referenced this issue May 27, 2022

Merge pull request #171 from koic/add_new_ensure_call_even_if_skip_cop

b6e4b5d

[Fix #169] Add new `Minitest/SkipEnsure` cop

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cop which detects ensure is called even if it is skipped #169

Cop which detects ensure is called even if it is skipped #169

yahonda commented May 19, 2022

koic commented May 20, 2022

yahonda commented May 20, 2022

yahonda commented May 20, 2022

koic commented May 20, 2022

yahonda commented May 20, 2022

Cop which detects ensure is called even if it is skipped #169

Cop which detects ensure is called even if it is skipped #169

Comments

yahonda commented May 19, 2022

Is your feature request related to a problem? Please describe.

Describe the solution you'd like

Describe alternatives you've considered

Additional context

koic commented May 20, 2022

yahonda commented May 20, 2022

yahonda commented May 20, 2022

koic commented May 20, 2022

yahonda commented May 20, 2022