java:S5838 is self contradictory

SonarQube Cloud
1

Rule java:S5838 provides contradictory and confusing results as demonstrated by expanding the code smells in this class.

The rule recommends I replace the first statement below with the second and the second statement with the first.

assertThat("foo".compareTo("bar")).isPositive();
assertThat("foo".compareTo("bar")).isGreaterThan(0);

The same rule recommends similarly contradictory advice for:

assertThat("bar".compareTo("foo")).isNegative();
assertThat("bar".compareTo("foo")).isLessThan(0);

It also seems to prefer the third statement below to the first, and the first to the second for:

assertThat("foo".compareTo("foo")).isZero();
assertThat("foo".compareTo("foo")).isEqualTo(0);
assertThat("foo".compareTo("foo")).isEqualByComparingTo(0);

(I would have thought the first statement would be preferable in all cases based on the statement that the rule “reports an issue when an assertion can be simplified to a dedicated one.”)

Complete SonarCloud Project and GitHub Project (I ran the analysis manually)

3

Hello @rhwood, welcome to the community, and thanks for sharing your feedback.

The expected simplification is:

assertThat("foo".compareTo("bar")).isGreaterThan(0);

“Use isPositive() instead.”

Okay, so far so good.

assertThat("foo".compareTo("bar")).isPositive();

“Use assertThat(actual).isGreaterThan(expected) instead.”
You are comparing "foo" and "bar", so actual is "foo" and expected is "bar".
This becomes:

assertThat("foo").isGreaterThan("bar");

Which is the simplified version.

That being said, I understand that the last step can be misunderstood. We tried at one point to provide a precise message, without actual and expected but the real values, but it turns out to be really complex and not even clearer.
In doubt, we expect the user to have a look at the rule description (which is currently broken in SonarCloud by the way, but it’s another problem) and go through examples to see the kind of simplifications are expected.

If you see a better way to report this issue, that would not have confused you, feel free to suggest it here.

Hope it clarifies the situation.

5

In these tests, I am asserting that Comparable.compareTo() as implemented by my class (String in this example) provides the expected output (which is an int). Are you suggesting that the rule is suggesting that instead of directly testing that method, I should be relying on assertj to implicitly call compareTo() in it’s isGreaterThan() and isLessThan() assertions?

Randall

6

Okay, I see. For the record, the same can happen with when testing equals method:

assertThat("foo".equals("bar")).isTrue();
assertThat("foo").isEqualTo("bar");

You can rely on Assertj to call compareTo() (or equals) in this situation.
In practice though, I would not argue if someone tells me that explicitly calling the method is better.
If you prefer to be explicit, you should resolve the issue as won’t fix. I think this situation is an acceptable corner case and I don’t see any easy improvement to improve it.

If you have ideas to improve the rule or believe the situation is unacceptable, feel free to come back to us with more examples and arguments to help to find a better solution.

7

Thank you. Looking at it again, it totally makes sense now.

Looking at the rule in SonarCloud, I did not see the reference to compareTo() that is clearly present when viewed in the Rules list on rules.sonarsource.com and which I think would have made sense to me then.

8

This topic was automatically closed 7 days after the last reply. New replies are no longer allowed.