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

Do not treat test_ methods with arguments as test methods #231

Merged
merged 1 commit into from Jan 18, 2023

Conversation

fatkodima
Copy link
Contributor

While testing #229 on https://github.com/openstreetmap/openstreetmap-website, it detected this - https://github.com/openstreetmap/openstreetmap-website/blob/b278bc06ef720af8c21c99659c4d70334384c386/test/integration/oauth2_test.rb#L141, because rubocop-minitest detects all class methods starting with test_ as test cases. But we should ignore such methods having arguments.

@koic
Copy link
Member

koic commented Jan 17, 2023

Makes sense! AFAIK, the conditions of the test method are as follows:

  1. name starts with test_
  2. is of public scope
  3. no arguments

Can you add a test code and changelog entry?

@fatkodima fatkodima force-pushed the ignore-test-methods-with-arguments branch from e7cea32 to b5a5723 Compare January 17, 2023 17:53
@fatkodima
Copy link
Contributor Author

Added a test code and changelog entry. I decided to extract test cases into a separate file instead of dumping into some random cop test file. Is it ok?

end
RUBY

def test_test_case_p_returns_true_for_test_case
Copy link
Contributor

Choose a reason for hiding this comment

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

What does the p mean here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"predicate". It's a replacement for ? at the end of the method name. I saw this convention in the ruby source code. For example, https://github.com/ruby/ruby/blob/a9bcc058bb4cd954e8406c93debb876983627ca9/array.c#L8851.

Copy link
Member

Choose a reason for hiding this comment

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

Even if p is used in the product code, the test_ descriptions might be easier to understand with predicate.
On a side note, p reminds me of LISP :-)

Comment on lines 40 to 47
def test_test_case_p_returns_false_for_test_methods_with_arguments
refute Helper.test_case?(method_node(SOURCE, :test_with_arguments))
end

private

def method_node(source, method_name)
class_node = parse_source!(source).ast
Copy link
Member

Choose a reason for hiding this comment

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

It could be reduced to the single argument. The entire place where SOURCE is used.

Suggested change
def test_test_case_p_returns_false_for_test_methods_with_arguments
refute Helper.test_case?(method_node(SOURCE, :test_with_arguments))
end
private
def method_node(source, method_name)
class_node = parse_source!(source).ast
def test_test_case_p_returns_false_for_test_methods_with_arguments
refute Helper.test_case?(method_node(:test_with_arguments))
end
private
def method_node(method_name)
class_node = parse_source!(SOURCE).ast

@@ -43,6 +44,10 @@ def test_cases(class_node)
test_cases + test_blocks
end

def test_case_signature?(def_node)
Copy link
Member

Choose a reason for hiding this comment

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

test_method? might be better, AFAIK signature doesn't include scope.

Suggested change
def test_case_signature?(def_node)
def test_method?(def_node)

@fatkodima fatkodima force-pushed the ignore-test-methods-with-arguments branch from b5a5723 to 2400514 Compare January 18, 2023 11:12
@fatkodima
Copy link
Contributor Author

Updated with the suggestions.

@koic koic merged commit d0ce98e into rubocop:master Jan 18, 2023
@koic
Copy link
Member

koic commented Jan 18, 2023

Thanks!

@fatkodima fatkodima deleted the ignore-test-methods-with-arguments branch January 18, 2023 19:21
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.

None yet

3 participants