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

Java: Add new quality query to detect missing @Nested annotation in JUnit5 tests #19094

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

jcogs33
Copy link
Contributor

@jcogs33 jcogs33 commented Mar 21, 2025

Description

Adds a new quality query to detect missing @Nested annotations on JUnit 5 inner test classes. This query is migrated from the advance security team's quality queries.

Consideration

Changes from original query. Let me know if you disagree with any of these changes:

  • Removed not testClass.isStatic() since the non-static requirement already seemed to be handled by testClass instanceof InnerClass. Let me know if the additional non-static check is needed for some reason I'm not aware of.
  • Since inner classes in Java are by definition non-static (c.f. Nested Classes Java docs), I have updated the metadata and other documentation to remove the redundant "non-static" descriptor. This includes updating the query ID and therefore results in the need for a previous ID. Let me know if you think I should keep the original wording instead.
  • Added an exclusion for abstract classes since placing an @Nested annotation on abstract classes is invalid and may cause an error. This exclusion reduces the number of alerts on the MRVA top-100 from 5 to 1 and on the MRVA top-1000 from 41 to 29.
  • Added exclusions for anonymous, local, and private classes since JUnit seems to define an inner class as non-private, non-anonymous, and non-local. These exclusions further reduce the number of alerts on the MRVA top-100 from 1 to 0 and on the MRVA top-1000 from 29 to 25.
  • Included @RepeatedTest, @ParameterizedTest, @TestFactory, and @TestTemplate when identifying JUnit 5 test methods. This inclusion adds 4 more results on the MRVA top-1000.
  • Added testability and frameworks/junit metadata tags to align with the tags on the other queries in java/ql/src/Likely Bugs/Frameworks/JUnit.

Questions:

  • Should this query have problem.severity of error instead of warning since it results in tests not running correctly?
  • I've added this query to the ccr suite. Let me know if I should not add it yet?

Other Notes:

  • Autofixes mostly look good, but tries to over-fix in a few cases and misses an import in another case.
  • DCA looks good.

@jcogs33 jcogs33 force-pushed the jcogs33/java/junit5-missing-nested-annotation branch from 8653c19 to daad77a Compare March 22, 2025 03:27
@jcogs33 jcogs33 force-pushed the jcogs33/java/junit5-missing-nested-annotation branch from b207ce1 to 0f00262 Compare March 24, 2025 01:52
@jcogs33 jcogs33 marked this pull request as ready for review March 24, 2025 01:53
@jcogs33 jcogs33 requested a review from a team as a code owner March 24, 2025 01:53
@jcogs33 jcogs33 requested review from tamasvajk and owen-mc March 24, 2025 01:53
tamasvajk
tamasvajk previously approved these changes Mar 24, 2025
Copy link
Contributor

@tamasvajk tamasvajk left a comment

Choose a reason for hiding this comment

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

LGTM, I have no preference on the problem.severity.

@jcogs33
Copy link
Contributor Author

jcogs33 commented Mar 24, 2025

I have no preference on the problem.severity.

I'll leave as warning then.

@jcogs33 jcogs33 added the ready-for-doc-review This PR requires and is ready for review from the GitHub docs team. label Mar 24, 2025
@vgrl vgrl added ready-for-doc-review This PR requires and is ready for review from the GitHub docs team. and removed ready-for-doc-review This PR requires and is ready for review from the GitHub docs team. labels Mar 24, 2025
mchammer01
mchammer01 previously approved these changes Mar 25, 2025
Copy link
Contributor

@mchammer01 mchammer01 left a comment

Choose a reason for hiding this comment

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

@jcogs33 👋🏻 - approving on behalf of Docs ⚡
Left a few minor suggestions, mainly to improve readability.


## Implementation Notes

This rule is focused on missing `@Nested` annotations on non-static nested (inner) test classes. Static nested test classes should not be annotated with `@Nested`. As a result, the absence of a `@Nested` annotation on such classes is compliant. Identifying incorrect application of a `@Nested` annotation to static nested classes is out of scope for this rule.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we say "query" instead of "rule"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wondered that as well, but found that there are many pre-existing qhelp files that use the term "rule" (see search results) so I left as-is.

I can change to "query" if anyone feels strongly about it. If we choose to say "query" instead, then we should align on that for all of the ported queries, since I think several say "rule". e.g. java/empty-method

@jcogs33
Copy link
Contributor Author

jcogs33 commented Mar 25, 2025

Thanks @mchammer01! I've applied your suggestions.

@jcogs33
Copy link
Contributor Author

jcogs33 commented Mar 25, 2025

cc @knewbury01

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Java ready-for-doc-review This PR requires and is ready for review from the GitHub docs team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants