java:S5841 invalid compliant solution

I’m trying your new AspectJ rules, and in particular java:S5841.

In your compliant solution section, you suggest

assertThat(logs).isEmpty().doesNotContain("error");

Which is not possible, since isEmpty() returns void.

Also, on a separate note, I believe that doesNotContain should not be included in this rule, or at least allMatch and doesNotContain should be separate rules. The semantics are very different.

For example it’s very common to say “this list should not contain something” and not care about the list’s size, but very rare to say “this list should not contain something and also not be empty”.

On the other hand your allMatch example in the rule definition makes absolute sense:

assertThat(logs).allMatch(e -> e.contains(“error”)); // Noncompliant, this test pass if logs are empty!

Also from a programmatic point of view, a “naked” allMatch can be matched (as suggested) with isNotEmpty(), where a “naked” doesNotContain cannot be matched with an isEmpty() (as wrongly suggested).

Hello,
thanks for taking the time to write this feedback.

Which is not possible, since isEmpty() returns void.

You are right, the correct code would look like

assertThat(logs).doesNotContain("error").isEmpty();

Concerning the second part of your post, I clearly understand your point. And at the same time, doesNotContain still faces the problem that whatever the argument, if the list is empty, the test will always pass. You should therefore either test the size or test the list content further.

This last part is maybe not well reflected in the description, in fact we are not raising an issue if you test the emptiness “indirectly”, for example:

assertThat(logs).contains("something").doesNotContain("error");

The situation we want to report is a case when you only test that a list does not contain something. In this case, I think you are missing something, and testing the emptiness is simple enough and the minimum you should do to improve the clarity of the test.

That being said, I agree that we are in a situation where this rule will report an issue on allMatch making perfect sense, and another one on doesNotContain which we can argue about. If it turns out to be a common feeling, we will be happy to consider reworking the rule.

With this additional context (and slightly modified description), does it still sounds strange to you?

Thanks for the reply. Yes I can confirm that we’re on the same page.

I still think that a simple assertThat(list).doesNotContain("something") is a valid use-case on it’s own.

It just means that the test invoked the production code in a way that this specific “something” should not be there.

This is from a personal example:

@Test
public void visibleForTesting_annotationOnPackagePrivateMethod_allowed() {
    // given
    Path[] classes = {Paths.get(SAMPLES_PATH + "AnnotationOnPackagePrivateMethod.class")};

    // when
    BugCollection bugCollection = spotbugs.performAnalysis(classes);

    // then
    doesNotViolateVisibleForTestingRules(bugCollection);
}

private void doesNotViolateVisibleForTestingRules(BugCollection bugCollection) {
    assertThat(bugCollection.getCollection())
        .extracting("type")
        .doesNotContain(
            "VISIBLE_FOR_TESTING_ONLY_ON_PACKAGE_PRIVATE",
            "VISIBLE_FOR_TESTING_ONLY_ON_METHOD",
            "VISIBLE_FOR_TESTING_ACCESS_VIOLATION");
}

The private method doesNotViolateVisibleForTestingRules is extracted because it’s reused by another 15 tests.
But not all 15 tests are set up in the exact same way, some violate other non-related spotbugs rules, and others don’t violate anything.
Hence, making the necessity of isEmpty/isNotEmpty pointless - for my specific scenario.

Of course it would be possible to copy/paste the assertion itself on all tests, so I can adjust each test properly, according to the list size.
Or I could return the assertion object itself, and append each test with isEmpty/isNotEmpty, like this:
doesNotViolateVisibleForTestingRules(bugCollection).isEmpty().

But these options are just work needed to satisfy Sonarcloud rules, and do not provide any benefit to my tests, nor fix any issue.

I hope this makes sense and is understandable.

PS: I have other such examples, but I hope one is sufficient to make my point…
PS2: funny thing - I just saw this: [Java] Class members annotated with @VisibleForTesting should not be accessed from production code, which is exactly what my spotbugs custom plugin does…

Thanks for the follow-up, I understand the situation.

I agree that depending on how you organize your code, you might end up in a situation where you don’t know/want to test if the list is empty or not, and in this case, it seems specific enough and resolving the issue as “won’t fix” would make sense to me.

these options are just work needed to satisfy Sonarcloud rules, and do not provide any benefit to my tests, nor fix any issue.

I perfectly agree. We are developers ourselves, we know the pain it can be, and we try hard so that nobody has this feeling. Still, I will happen, it probably means that you are in a situation that we didn’t expect, resolving the issue as won’t fix/false positive and coming here to share your situation and opinion is a good move in order to improve the situation!

I agree with Stefanos. The assertThat(list).doesNotContain("something") is a complete validation of the test and forcing me to add isEmpty() or isNotEmpty() not only violates the a-single-test-should-test-one-thing-only rule but also forces me to make some unwanted assumptions and may cause regression problems: imagine an intern seeing this warning on assertThat(output).doesNotContain("error"), then he runs the test, sees the list is empty and adds isEmpty() to the assertion. Some time later some other developer changes the code under test so there is an output but there are no “error” strings and test are broken while everything works as expected. “Why did you add ‘isEmpty’ ? Should this function always produce no output?” “I don’t know, sonarLint told me to”

1 Like

Hi @kRyszard,

I like your practice about “a test should test one thing” when there is a reliable comparison (like: isEqualTo, contains, …).
But if you test the absence of something, the risk of writing an erroneous comparison with a successful test status exists. In fact, you could provide in your test any random argument to isNotEqualTo or doesNotContain, and the test is more likely to pass. So your test could be a lie.
Unfortunately, our analyzer has no rule about testing the absence of an impossible value. But the rule S5841 tries at least to improve the reliability of the test by ensuring that you are testing the expected list, for example:

  • I’m probably testing the right variable ‘x’ because it contains “warning”:
assertThat(x).contains("warning").doesNotContain("error");
  • If I can not improve my confidence by testing an existing value, it means the list is empty, let’s make it explicit:
assertThat(x).doesNotContain("error").isEmpty();

So because the rule S5841 focuses on risky tests that on a given variable check and only check the absence of something, I still believe it’s a good safety net with a minor drawback.

I wouldn’t say the drawback is minor because it adds extra confusion when reading someone else’s code or even my own code that is at least one month old. I’d have to add a comment every time, saying that this other “contains” or “isNotEmpty” is only to be sure that I’m testing the right variable. Besides, the IDE should control if I’m testing the right variable with its “Variable ‘x’ is never used” warning.

Hi @kRyszard,
We will wait to have more feedback from other users to have a better understanding and to see if there’s a consensus before changing the logic of S5841. Meanwhile, you should probably disable the rule S5841.
Thanks for your feedback.