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

google_checks SuppressionXpathSingleFilter for 'MethodName' check is too lenient in several ways #15609

Closed
mches opened this issue Sep 2, 2024 · 4 comments

Comments

@mches
Copy link
Contributor

mches commented Sep 2, 2024

I have read check documentation: https://checkstyle.org/filters/suppressionxpathsinglefilter.html
I have downloaded the latest checkstyle from: https://checkstyle.org/cmdline.html#Download_and_Run
I have executed the cli and showed it below, as cli describes the problem better than 1,000 words

/var/tmp $ javac PrintStreamTest.java

/var/tmp $ cat /google_checks.xml
cat: /google_checks.xml: No such file or directory

/var/tmp $ cat PrintStreamTest.java
import java.lang.annotation.Documented;
import java.lang.annotation.ElementType;
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
import java.lang.annotation.Target;

class PrintStreamTest {
  void println__String_(@Test String x) {
    class Local {
      void println__String_(String x) {
        System.out.println(x);
      }
    }

    new Local().println__String_(x);
  }

  @Target(ElementType.PARAMETER)
  @Retention(RetentionPolicy.RUNTIME)
  @Documented
  @interface Test {}
}

/var/tmp $ java $RUN_LOCALE -jar checkstyle-10.18.1-all.jar -c /google_checks.xml PrintStreamTest.java
Starting audit...
Audit done.

Describe what you expect in detail.
The two illegally-named non-test methods should be reported e.g.

Starting audit...
[WARN] /var/tmp/PrintStreamTest.java:8:8: Method name 'println__String_' must match pattern '^[a-z][a-z0-9][a-zA-Z0-9]*$'. [MethodName]
[WARN] /var/tmp/PrintStreamTest.java:10:12: Method name 'println__String_' must match pattern '^[a-z][a-z0-9][a-zA-Z0-9]*$'. [MethodName]
Audit done.
  1. Multiple consecutive underscores are not allowed for test methods.
  2. Trailing underscores are not allowed for test methods.
  3. Each underscore-separated component should start with a lowercase letter.
  4. Without a method annotation, the method is not a test method.
  5. A method on a local or anonymous class declared in a test method is not necessarily a test method.

If approved, here is a proposal I would like to submit as a PR.

Before:

    <module name="SuppressionXpathSingleFilter">
      <property name="checks" value="MethodName"/>
      <property name="query" value="//METHOD_DEF[
                                     .//ANNOTATION/IDENT[contains(@text, 'Test')]]//*"/>
      <property name="message" value="'[a-z][a-z0-9][_a-zA-Z0-9]*'.*"/>
    </module>

↓↓↓

After:

    <module name="SuppressionXpathSingleFilter">
      <property name="checks" value="MethodName"/>
      <property name="query" value="//METHOD_DEF[
                                     ./MODIFIERS/ANNOTATION//IDENT[contains(@text, 'Test')]]/IDENT"/>
      <property name="message" value="'[a-z][a-z0-9][a-zA-Z0-9]*(?:_[a-z][a-z0-9][a-zA-Z0-9]*)*'"/>
    </module>

Note this proposal also accommodates fully-qualified test annotations. i.e.

/var/tmp $ javac -cp apiguardian-api-1.1.2.jar:junit-jupiter-api-5.11.0.jar Test.java

/var/tmp $ cat /google_checks.xml
cat: /google_checks.xml: No such file or directory

/var/tmp $ cat Test.java
class Test {
  @org.junit.jupiter.api.Test
  void underscoreSeparated_lowerCamelCase_components() {}
}

/var/tmp $ java $RUN_LOCALE -jar checkstyle-10.18.1-all.jar -c /google_checks.xml Test.java
Starting audit...
[WARN] /var/tmp/Test.java:3:8: Method name 'underscoreSeparated_lowerCamelCase_components' must match pattern '^[a-z][a-z0-9][a-zA-Z0-9]*$'. [MethodName]
Audit done.
mches added a commit to markitect-dev/checkstyle that referenced this issue Sep 2, 2024
mches added a commit to markitect-dev/checkstyle that referenced this issue Sep 3, 2024
mches added a commit to markitect-dev/checkstyle that referenced this issue Sep 3, 2024
mches added a commit to markitect-dev/checkstyle that referenced this issue Sep 3, 2024
mches added a commit to markitect-dev/checkstyle that referenced this issue Sep 3, 2024
mches added a commit to markitect-dev/checkstyle that referenced this issue Sep 3, 2024
mches added a commit to markitect-dev/checkstyle that referenced this issue Sep 3, 2024
@romani
Copy link
Member

romani commented Sep 3, 2024

Still not clear ...

Should be removed from issue description

mches added a commit to markitect-dev/checkstyle that referenced this issue Sep 4, 2024
mches added a commit to markitect-dev/checkstyle that referenced this issue Sep 4, 2024
@Zopsss
Copy link
Contributor

Zopsss commented Sep 4, 2024

  1. Each underscore-separated component should start with a lowercase letter.

We should allow digits immediately after underscore, reason explained at: #15617 (comment)

@mches
Copy link
Contributor Author

mches commented Sep 5, 2024

  1. Each underscore-separated component should start with a lowercase letter.

We should allow digits immediately after underscore, reason explained at: #15617 (comment)

I'll have a look. Thanks for the thorough review

mches added a commit to markitect-dev/checkstyle that referenced this issue Sep 7, 2024
mches added a commit to markitect-dev/checkstyle that referenced this issue Sep 10, 2024
mches added a commit to markitect-dev/checkstyle that referenced this issue Sep 10, 2024
mches added a commit to markitect-dev/checkstyle that referenced this issue Sep 10, 2024
mches added a commit to markitect-dev/checkstyle that referenced this issue Sep 10, 2024
mches added a commit to markitect-dev/checkstyle that referenced this issue Sep 10, 2024
mches added a commit to markitect-dev/checkstyle that referenced this issue Sep 10, 2024
mches added a commit to markitect-dev/checkstyle that referenced this issue Sep 10, 2024
mches added a commit to markitect-dev/checkstyle that referenced this issue Sep 11, 2024
mches added a commit to markitect-dev/checkstyle that referenced this issue Sep 11, 2024
romani pushed a commit that referenced this issue Sep 16, 2024
@github-actions github-actions bot added this to the 10.18.2 milestone Sep 16, 2024
@romani romani added the bug label Sep 16, 2024
@romani
Copy link
Member

romani commented Sep 16, 2024

Fix is merged

@romani romani closed this as completed Sep 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants