[java] New rule: AssertEqualsArgumentOrder - #6713

Documentation Preview

Compared to main:
This changeset changes 0 violations,
introduces 0 new violations, 0 new errors and 0 new configuration errors,
removes 0 violations, 0 errors and 0 configuration errors.
There are 0 changed duplications, 0 new duplications and 0 removed duplications.
There are 0 changed CPD errors, 0 new CPD errors and 0 removed CPD errors.

Regression Tester Report

(comment created at 2026-06-19 17:09:29+00:00 for 8cc3eaa)

You might want to add support for TestNG, spring-test, and JSONAssert

@adangel I checked how other frameworks do this (links in initial comment) and I'm no longer sure this should be a single rule -- the indication is the same (constant being used as "actual"), but the implications are different: in some cases the error message is confusing, in others it's likely a copy-paste error making the whole test method pointless. The upside of having one rule for both is that we don't have to inspect the same nodes twice.

Btw. the trivial assertions with assertTrue are covered by UnnecessaryBooleanAssertion, it might be useful to also cover assertNull(null) / assertNotNull(null) / assertNull("literallly never null") by a similar rule.

I'm no longer sure this should be a single rule

I'd say the scope of the rule is fine as it is. What you have right now is AFAICT equivalent to MisorderedAssertEqualsArguments/AssertEqualsArgumentOrderChecker, isn't it? The other linked checks could serve as ideas for other rules.

What you have right now is AFAICT equivalent to MisorderedAssertEqualsArguments/AssertEqualsArgumentOrderChecker, isn't it?

Well right now it also detects the comparison of two literals, which falls under something like EqualsWithItself.

The question is whether we ever want to have a rule for detecting assertEquals(variableX, variableX) -- if yes, and there is a rule EqualsWithItself for it, is it an issue that assertEquals("a","a") would be flagged by two rules?

Ah, I see. Looking at the rule description...

        * Passing a constant as both actual and expected value makes
       the assertion always fail or always pass. In these cases the assertion should be
       replaced by `fail()` or removed.
       * Passing a constant as the actual value and a computed value as the expected value
       indicates that the actual and expected values were swapped, leading to confusing
       error messages should the test fail.

That indeed "feels" like two different things.

I'm no longer sure this should be a single rule -- the indication is the same (constant being used as "actual"), but the implications are different: in some cases the error message is confusing, in others it's likely a copy-paste error making the whole test method pointless. The upside of having one rule for both is that we don't have to inspect the same nodes twice.

In general, I like the rule. It will be useful (and we should enable it for our own dogfood ruleset).

I think, we should keep it as one rule - a rule that concentrates on problems around using assertEquals. If we ever do quickfix/autofix (which would require to know which solution is correct), we can split the rule later at anytime.

Ah, I see. Looking at the rule description...

        * Passing a constant as both actual and expected value makes
       the assertion always fail or always pass. In these cases the assertion should be
       replaced by `fail()` or removed.
       * Passing a constant as the actual value and a computed value as the expected value
       indicates that the actual and expected values were swapped, leading to confusing
       error messages should the test fail.

That indeed "feels" like two different things.

Maybe we can describe the rule a bit more general like "There is an assert equals statement, that doesn't look right".

Thanks to your comments about the parameter order, I remembered now the problem when switching from Junit4 to Junit Jupiter:

// JUnit 3 and 4, Spring: [message], expected, actual
// JUnit Jupiter: expected, actual, [message]

If - for some reason - you provided a message, but use the same value for the message as expected, then upgrading to JUnit Jupiter by just statically importing the other assertEquals gives you the problem "expected and actual are the same"... I remember, I had to touch every assertEquals, when we migrated to Junit 5 in PMD...

So, maybe, we just shouldn't suggest to replace the assertEquals with "fail" or remove it. Maybe we should just point out, that "expected" and "actual" parameters are the same - and this not something you usually want. We could of course check, whether we have 3 parameters and suggest to move the first parameter to the end (assuming this was a JUnit4 message parameter). But I wouldn't do that and keep it simple.

But I see, you already updated the rule description...

Alternative rule name suggestions: AssertEqualsArgumentMismatch, PotentialAssertEqualsArgumentMismatch, UnusualAssertEquals

rule violation message suggestion: assertEquals() called with swapped arguments or constants

also the rule example should probably be updated to not only include constants, but also include the swapped expected/actual case.