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

SONARJAVA-5256 Fix false positives on S2699 for AssertJ assertions #5008

Merged
merged 2 commits into from
Feb 11, 2025

Conversation

romainbrenguier
Copy link
Contributor

@romainbrenguier romainbrenguier commented Feb 7, 2025

SONARJAVA-5256

There are many methods that can be used to write assertions with AssertJ. The previous pattern was only recognizing a couple of assertJ classes.
With this PR we generalize the pattern for recognizing AssertJ assertion methods so that it applies to many of these classes.

@romainbrenguier romainbrenguier force-pushed the romain/fp-bddAssertion branch 4 times, most recently from a9a9958 to de36286 Compare February 7, 2025 10:23
This adds a test with a call to AssertJ's BDDAssertions.thenNoException().isThrownBy(...) which is currently a false positive for rule S2699 (Tests should include assertions).
This also adds a second example with isTrue() call on some AssertJ *Assert class.
@romainbrenguier romainbrenguier force-pushed the romain/fp-bddAssertion branch 2 times, most recently from 90e6217 to 5a65981 Compare February 10, 2025 14:20
Comment on lines 47 to 49
@VisibleForTesting
static final Pattern ASSERTJ_ASSERTION_METHODS_PATTERN = Pattern.compile(
"(allMatch|assert|contains|doesNot|has|is|returns|satisfies)([A-Z].*)?");
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I didn't look enough carefully during the first review. I forgot to suggest:


  • Try to move computation in the static field as much as possible (executed only once) instead of during each method invocation (executed millions of times).

So I would call asMatchPredicate() in the static field initialization:

Suggested change
@VisibleForTesting
static final Pattern ASSERTJ_ASSERTION_METHODS_PATTERN = Pattern.compile(
"(allMatch|assert|contains|doesNot|has|is|returns|satisfies)([A-Z].*)?");
@VisibleForTesting
static final Predicate<String> ASSERTJ_ASSERTION_METHODS_PREDICATE = Pattern.compile(
"(allMatch|assert|contains|doesNot|has|is|returns|satisfies)([A-Z].*)?").asMatchPredicate();

  • Never use the String#matches(String) that will call several times Pattern.compile.

We could have the following predicate:

  private static final Pattern ASSERTJ_ASSERTION_CLASSNAME_PATTERN = Pattern.compile(
    "org\\.assertj\\.core\\.api\\.[a-zA-Z]+Assert");

  private static final Predicate<Type> ASSERTJ_ASSERTION_TYPE_PREDICATE =
    type -> ASSERTJ_ASSERTION_CLASSNAME_PATTERN.matcher(type.fullyQualifiedName()).matches();

So we can replace:

MethodMatchers.create().ofType(type -> type.fullyQualifiedName().matches("org\\.assertj\\.core\\.api\\.[a-zA-Z]+Assert"))

with

MethodMatchers.create().ofType(ASSERTJ_ASSERTION_TYPE_PREDICATE)

The previous conditions were too restrictive and there are many assertion methods in AssertJ. They generally follow the patterns that are described here.
Note that this introduces a false negative in autoscan tests.

Co-authored-by: Alban Auzeill <[email protected]>
Copy link
Member

@alban-auzeill alban-auzeill left a comment

Choose a reason for hiding this comment

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

Thank you for the changes!

@alban-auzeill alban-auzeill merged commit 27684c4 into master Feb 11, 2025
17 checks passed
@alban-auzeill alban-auzeill deleted the romain/fp-bddAssertion branch February 11, 2025 08:13
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.

2 participants