Skip to content

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 owen-mc and tamasvajk 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.

Co-authored-by: mc <42146119+mchammer01@users.noreply.github.com>
@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

owen-mc
owen-mc previously approved these changes Apr 1, 2025
owen-mc
owen-mc previously approved these changes Apr 2, 2025
Copy link
Contributor

@owen-mc owen-mc left a comment

Choose a reason for hiding this comment

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

Approved, modulo a few things which need to be cleared up before merging.

@jcogs33 jcogs33 added the no-change-note-required This PR does not need a change note label Apr 4, 2025
@jcogs33 jcogs33 requested review from owen-mc and tamasvajk April 21, 2025 18:21
@jcogs33 jcogs33 merged commit ed99088 into github:main Apr 22, 2025
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Java no-change-note-required This PR does not need a change note 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