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

test_* method out of ActiveSupport::TestCase subclasses #279

Closed
yahonda opened this issue Dec 12, 2023 · 7 comments · Fixed by #280
Closed

test_* method out of ActiveSupport::TestCase subclasses #279

yahonda opened this issue Dec 12, 2023 · 7 comments · Fixed by #280
Labels
feature request Request for new functionality

Comments

@yahonda
Copy link
Contributor

yahonda commented Dec 12, 2023

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

Reviewing rails/rails#50334 (comment) and found that there are some tests that are not executed because it does not belongs to ActiveSupport::TestCase subclass.

I'd like some cop to find test_* method out of ActiveSupport::TestCase subclasses.

Steps to reproduce

git clone https://github.com/rails/rails
cd rails/activerecord
bundle install
bin/test test/cases/assertions/query_assertions_test.rb -v

Actual behavior

Only ActiveRecord::Assertions::QueryAssertionsTest#test_assert_queries is executed.

$ bin/test test/cases/assertions/query_assertions_test.rb -v
Using sqlite3
Run options: -v --seed 60664

# Running:

ActiveRecord::Assertions::QueryAssertionsTest#test_assert_queries = 0.02 s = .

Finished in 0.024659s, 40.5533 runs/s, 486.6391 assertions/s.
1 runs, 12 assertions, 0 failures, 0 errors, 0 skips
$

There are two test_assert_queries and test_assert_no_queries methods and only test_assert_queries test is executed because it belongs to QueryAssertionsTest class. test_assert_no_queries is out of QueryAssertionsTest class then not executed.

Describe the solution you'd like

I wanted some cop to find any test_* methods that are out of ActiveSupport::TestCase subclasses.

Describe alternatives you've considered

Compare the Execute the test with -v option and git grep "def test_" to see which tests are executed, that is actually hard because there are many tests.

$ bin/test test/cases/assertions/query_assertions_test.rb -v
Using sqlite3
Run options: -v --seed 60664

# Running:

ActiveRecord::Assertions::QueryAssertionsTest#test_assert_queries = 0.02 s = .

Finished in 0.024659s, 40.5533 runs/s, 486.6391 assertions/s.
1 runs, 12 assertions, 0 failures, 0 errors, 0 skips
$
$ git grep 'def test_' test/cases/assertions/query_assertions_test.rb
test/cases/assertions/query_assertions_test.rb:      def test_assert_queries
test/cases/assertions/query_assertions_test.rb:    def test_assert_no_queries
$

Additional context

Add any other context or screenshots about the feature request here.

@koic
Copy link
Member

koic commented Dec 12, 2023

I see. Just to be sure, can you write examples of both a bad case and a good case that you expect?

@yahonda
Copy link
Contributor Author

yahonda commented Dec 12, 2023

Sure. Will update the good and bad cases.

@yahonda
Copy link
Contributor Author

yahonda commented Dec 12, 2023

Here is my expected bad and good example.

# bad
module ActiveRecord
  module Assertions
    class FooTest < ActiveSupport::TestCase
    end
    def test_something
      assert true
    end
  end
end
# good
module ActiveRecord
  module Assertions
    class FooTest < ActiveSupport::TestCase
      def test_something
        assert true
      end
    end
  end
end

@Earlopain
Copy link
Contributor

Tests may be extracted into modules to include them later with different behaviour, that should be considered.

I know the httpx gem makes heave use of that, see https://gitlab.com/os85/httpx/-/blob/master/test/support/requests/plugins/auth.rb?ref_type=heads and https://gitlab.com/os85/httpx/-/blob/master/test/http_test.rb?ref_type=heads with https://gitlab.com/os85/httpx/-/blob/master/test/https_test.rb?ref_type=heads for testing against http and https with the same code.

@koic koic added the feature request Request for new functionality label Dec 12, 2023
@andyw8
Copy link
Contributor

andyw8 commented Dec 12, 2023

I worry this will cause too many false positives – I'd like to see how it runs against https://github.com/jeromedalbert/real-world-ruby-apps.

koic added a commit to koic/rubocop-minitest that referenced this issue Dec 14, 2023
Resolves rubocop#279.

This PR adds new `Minitest/NonExecutableTestMethod` cop, which checks for
the use of test methods outside of a test class.
Test methods should be defined within a test class to ensure their execution.

NOTE: This cop assumes that classes whose superclass name includes the word
"`Test`" are test classes, in order to prevent false positives.

```ruby
# bad
class FooTest < Minitest::Test
end
def test_method_should_be_inside_test_class
end

# good
class FooTest < Minitest::Test
  def test_method_should_be_inside_test_class
  end
end
```
koic added a commit to koic/rubocop-minitest that referenced this issue Dec 14, 2023
Resolves rubocop#279.

This PR adds new `Minitest/NonExecutableTestMethod` cop, which checks for
the use of test methods outside of a test class.
Test methods should be defined within a test class to ensure their execution.

NOTE: This cop assumes that classes whose superclass name includes the word
"`Test`" are test classes, in order to prevent false positives.

```ruby
# bad
class FooTest < Minitest::Test
end
def test_method_should_be_inside_test_class
end

# good
class FooTest < Minitest::Test
  def test_method_should_be_inside_test_class
  end
end
```
@koic
Copy link
Member

koic commented Dec 14, 2023

I've opened #280. This new cop assumes that classes whose superclass name includes the word "Test" are test classes, in order to prevent false positives. Stricter criteria for detection are implemented to prevent false positives; however, the occurrence of some false negatives cannot be completely avoided.

@koic
Copy link
Member

koic commented Dec 14, 2023

Unfortunately, https://github.com/jeromedalbert/real-world-ruby-apps is too large, resulting in high costs for inspection, so it has not been carried out. However, it has been tested with rails/rails repo.

@koic koic closed this as completed in #280 Dec 15, 2023
koic added a commit that referenced this issue Dec 15, 2023
…t_method_cop

[Fix #279] Add new `Minitest/NonExecutableTestMethod` cop
yahonda added a commit to yahonda/rails that referenced this issue Dec 17, 2023
This pull request enables `Minitest/NonExecutableTestMethod` cop
to find non-executed test that is out of `ActiveSupport::TestCase` and its subclasses.

This cop is based on the request since there was a test that is not executed found
at rails#50334 (comment)
and implemented to RuboCop Minitest 0.34.0 via rubocop/rubocop-minitest#279

This cop works as follows.
As of right now, there is no offenses by reverting the merge commit via rails#50334

```
$ git revert -m 1 9517841
$ bundle exec rubocop
Inspecting 3254 files
... snip ...

Offenses:

activerecord/test/cases/assertions/query_assertions_test.rb:27:5: W: Minitest/NonExecutableTestMethod: Test method should be defined inside a test class to ensure execution.
    def test_assert_no_queries ...
    ^^^^^^^^^^^^^^^^^^^^^^^^^^

3254 files inspected, 1 offense detected
$
```
yahonda added a commit to yahonda/rails that referenced this issue Dec 17, 2023
This pull request enables `Minitest/NonExecutableTestMethod` cop
to find non-executed test that is out of `ActiveSupport::TestCase` and its subclasses.

This cop is based on the request since there was a test that is not executed found
at rails#50334 (comment)
and implemented to RuboCop Minitest 0.34.0 via rubocop/rubocop-minitest#279

This cop works as follows.
As of right now, there is no offenses by reverting the merge commit via rails#50334

```
$ git revert -m 1 9517841
$ bundle exec rubocop
Inspecting 3254 files
... snip ...

Offenses:

activerecord/test/cases/assertions/query_assertions_test.rb:27:5: W: Minitest/NonExecutableTestMethod: Test method should be defined inside a test class to ensure execution.
    def test_assert_no_queries ...
    ^^^^^^^^^^^^^^^^^^^^^^^^^^

3254 files inspected, 1 offense detected
$
```

* `Gemfile.lock` has been updated as follows
```
bundle update rubocop rubocop-minitest --conservative
```
rafaelfranca pushed a commit to yahonda/rails that referenced this issue Jan 3, 2024
This pull request enables `Minitest/NonExecutableTestMethod` cop
to find non-executed test that is out of `ActiveSupport::TestCase` and its subclasses.

This cop is based on the request since there was a test that is not executed found
at rails#50334 (comment)
and implemented to RuboCop Minitest 0.34.0 via rubocop/rubocop-minitest#279

This cop works as follows.
As of right now, there is no offenses by reverting the merge commit via rails#50334

```
$ git revert -m 1 9517841
$ bundle exec rubocop
Inspecting 3254 files
... snip ...

Offenses:

activerecord/test/cases/assertions/query_assertions_test.rb:27:5: W: Minitest/NonExecutableTestMethod: Test method should be defined inside a test class to ensure execution.
    def test_assert_no_queries ...
    ^^^^^^^^^^^^^^^^^^^^^^^^^^

3254 files inspected, 1 offense detected
$
```

* `Gemfile.lock` has been updated as follows
```
bundle update rubocop rubocop-minitest --conservative
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Request for new functionality
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants