Rules generating questionable reports for lambda expressions in junits

Examining reports on SonarCloud generated from Jenkins job kicked off by gerrit.

Example at: https://sonarcloud.io/project/issues?id=onap_policy-common&open=AXLjCROzGx4GvWbyq19t

java:S5778

Sonar is now reporting “Refactor the code of the lambda to have only one invocation throwing an exception” for cases that were previously OK. Examining it further, the underlying condition is, “Only one method invocation is expected when testing runtime exceptions”.

First, this should only be an issue when testing unchecked exceptions; it should not matter when testing checked exceptions. Second, even when testing unchecked exceptions, if the other invocations cannot generate the particular Runtime exception being tested, then it should not matter if they are invoked.

It’s not like writing junit tests isn’t already tedious. Now, every method call in every lambda is going to have to be split out into its own statement. This needs to be tightened up so that it only reports if the assertion can pass due to more than one of the invocations throwing an exception.

Hello @jrh3, thanks for sharing your opinion on this subject.

First, let me try to summarize the current situation to be sure to be on the same page.

We have two rules targeting the problem: (having multiple method calls inside code tested for exception is a bad pattern).

  • RSPEC-5783: Only one method invocation is expected when testing checked exceptions.

It will raise an issue if more than one method call made in the lambda explicitly throws the tested checked exception. This is bad (“critical bug”) since it is not possible to know what is really tested.
If you have multiple calls, but only one throwing the tested one, it will not raise an issue. This can still be considered as a bad pattern, but at the same time, you are sure you are testing exactly what was intended.

  • RSPEC-5778 Only one method invocation is expected when testing runtime exceptions.

This case still faces the same problem as described before, but we can not be sure about it. Since it is clearly not as bad as the first case, this one was reduced to “Major Code Smell” severity. In addition, even when the test is valid, it will still increase the clarity, you will be able to see at one glance what method is expected to throw the exception, without having to dig into the code, or rely on convention (“last” method call is the one tested). I believe it fits well with the definition of code smell.

It’s not like writing junit tests isn’t already tedious.

We are developers ourselves (and also using our own analyzer), we perfectly know that it can be tedious to write tests, this is why we strive to provide rules increasing as little as possible the tediousness and only if we strongly believe it will benefits to the developer at the end.

Does it clarify the situation?

Hi Quentin,

Sounds like RSPEC-5783 already works as I’d want it to work.

Regarding RSPEC-5778, on the other hand, I’d prefer it to work in a similar fashion. However, I guess since it’s dealing with unchecked exceptions, there is no easy way for sonar to know that “new JsonObject()” never throws a given runtime exception. I do see the point in what the rule is trying to expose, which is why I called this a “questionable report” instead of an “erroneous report”. Nevertheless, it doesn’t apply in most of our cases, thus all of the changes we have to make to eliminate this complaint will go into that “doing this work just to satisfy sonar… grumble, grumble” category.

there is no easy way for sonar to know that “new JsonObject()” never throws a given runtime exception

Exactly, and at the same time, it also means that it’s not trivial for the developer either.

Nevertheless, it doesn’t apply in most of our cases

Could you think of a way to reduce this noise?