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?