[JAVA] squid:UndocumentedApi and @VisibleForTesting

[David Racodon]

Hi

Based on https://nemo.sonarqube.org/issues/search#issues=AVRa7OyKBhFUGJcI4ye3, I'm wondering if the squid:UndocumentedApi rule shouldn't be excluded on methods annotated with VisibleForTesting.

Thank you

Regards,

David RACODON

Freelance QA Consultant

LinkedIn

[Tim Webster]

I raised something similar:

https://groups.google.com/forum/?utm_medium=email&utm_source=footer#!msg/sonarqube/Ug2ASiiU-0c/eIfWNkFyAwAJ

@VisibleForTesting is an extremely useful annotation.  I would like to encourage people here to use it, but not if its use is going to lead to more issues being raised.  Obviously it can be used to hide 'code smells', where a refactor of the code would eliminate the need for special testing methods, but there are times where it is the best option (usually interfacing with legacy/3rd party code we have no control over).

But I'm not sure what approach would be the best.  Do we ask that all rules are excluded on code where this annotation is used?

Tim

[David Racodon]

Hi Tim,

@VisibleForTesting is an extremely useful annotation

I fully agree with you. And it clearly documents what the developer intended to do.

Obviously it can be used to hide 'code smells', where a refactor of the code would eliminate the need for special testing methods, but there are times where it is the best option (usually interfacing with legacy/3rd party code we have no control over).

Indeed, but sometimes it's the best option to keep both the code and the tests clear and simple. There are other options to access private methods in unit tests: PowerMock or reflection for example. But there are not better (if not worse) than @VisibleForTesting annotation.

And by the way, this annotation is used in the SonarQube Java plugin in several places. Here for example: https://github.com/SonarSource/sonar-java/blob/master/java-frontend/src/main/java/org/sonar/java/SonarComponents.java#L211

I would like to encourage people here to use it, but not if its use is going to lead to more issues being raised

I would as well. But it seems to generate a bit too much noise for now.

https://groups.google.com/forum/?utm_medium=email&utm_source=footer#!msg/sonarqube/Ug2ASiiU-0c/eIfWNkFyAwAJ

I'm +1 to exclude methods with @VisibleForTesting annotation for this squid:UnusedProtectedMethod check as well.

But I'm not sure what approach would be the best.  Do we ask that all rules are excluded on code where this annotation is used?

Not from my point of view. I would prefer to only tune the rules that would be impacted: squid:UndocumentedApi, squid:UnusedProtectedMethod, etc. And it looks like this tuning has been performed for some rules already: https://github.com/SonarSource/sonar-java/blob/master/java-checks/src/main/java/org/sonar/java/checks/StaticFinalArrayNotPrivateCheck.java#L85

To mix with the discussion you had with Nicolas:

on one side I think that making something more visible for testing is really not a good practice anyway 

In some cases I would agree, in others I would not. But for me, it's another matter. Another rule, not activated by default, could be added to raise an issue while hitting a @VisibleForTesting annotation. But from my point of view, existing rules should not generate "noise"/"false positive" when @VisibleForTesting is used.

What do you think?

Thank you

Regards,

David RACODON

Freelance QA Consultant

LinkedIn

--
You received this message because you are subscribed to the Google Groups "SonarQube" group.
To unsubscribe from this group and stop receiving emails from it, send an email to sonarqube+...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/sonarqube/202e1a20-7733-4472-96c7-e493e6659c4c%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Tim Webster]

Hi David,

I've though about this a bit more...

I think we're on the same page here, but if we want to be able to 'tune' the rules, where does it stop?  We can't really expect them to update the Java plugin every time someone wants to exclude a different rule when @VisibleForTesting is used.  OK, maybe if it's only a couple of rules (say, the ones we mentioned), but any more and it might get silly.

Maybe an option to shut off all violations in a block of code where @VisibleForTesting is used?  But then, this is still production code and we wouldn't want to miss any critical/blocker issues because they have been hidden by this.

What about just including a // NOSONAR or @SuppressWarnings?  With the latter you can target the specific rule at least.  It's a pain to do this every time you want to use the annotation though.

It would be nice to hear if SonarSource has any more ideas, now that we've established more of an interest...

Tim

[Michael Gumowski]

Hey David, Tim,

Thanks a lot for discussing the subject! In the current sprint of the Java Analyzer (for version 3.14), we implemented a new approach to simplify handling of situations where we want to discard issues raised by a multiple rules, without having to modify their implementations.

We applied this principle to handle the I18N framework from Eclipse (https://jira.sonarsource.com/browse/SONARJAVA-1640), as well as Lombok annotations (https://jira.sonarsource.com/browse/SONARJAVA-1642).

As a starter, I think that we could use the same approach to easily discard issues from squid:UndocumentedApi, as well as squid:UnusedProtectedMethod when a method is annotated with @VisibleForTesting. We will then add extra rules when needed, if it is justified.

WDYT?

Cheers,

Michael GUMOWSKI | SonarSource

Software Developer @ Language Team
http://sonarsource.com

To view this discussion on the web visit https://groups.google.com/d/msgid/sonarqube/f18865b2-c3fe-4a9b-96b4-fb4224bdec77%40googlegroups.com.

[David Racodon]

Hi Michael,

This rule by rule approach suits me well.

Thanks!

Regards,

To view this discussion on the web visit https://groups.google.com/d/msgid/sonarqube/CABGmqn%3DTc0M-EHC7U617OsJUDj%3D%3Du5zPSAfd9c5ZZvJKW7dpHg%40mail.gmail.com.

[Tim Webster]

OK - but I'm not clear on how the filter works.  Do you (SonarSource) decide what rules to exclude and then hard-code them into the Java plugin (or a similar process)?

[Michael Gumowski]

Yes - we hardcoded in the current filters the knowledge of which rule to discard and in what situation. We decided which rules to exclude, based on previous feedback from the community.

Now, to clarify how they work:

These "custom" issue filters are implemented by the java plugin, and used during analysis. They are implemented almost like usual java rules, based on visitors and relying on semantic, but with extended behavior. This allows them to identify parts of the code where given rules should not raise issues, prior to their execution. Then, every time a rule will try to raise an issue during analysis, the filters decide if the issue is valid or should be discarded.

To give you a concrete example, let's consider the lombok filter. When it encounters a class annotated, for instance, with '@lombok.Data', it knows before executing any rule that some issues won't be relevant and should be filtered. For this specific class with this specific annotation, the filter will then discard explicitly the issues raised by the following rule:

• squid:S1068 (Unused "private" fields should be removed), 
• squid:S2160 (Subclasses that add fields should override "equals") 
• squid:S1210 ("equals(Object obj)" should be overridden along with the "compareTo(T obj)" method)

As you can guess, using this mechanism allowed us to remove all the lombok-specific code from the 3 rules listed above!

Note that issue filtering using this approach is still a very young feature for the java plugin. Consequently, it is not possible to configure the filters, nor to disable them prior to the analysis. It may change in the future, and the API they are based on could maybe also be pushed to public API, allowing usage of custom filters by custom java plugins... but we have nothing planned yet.

Michael GUMOWSKI | SonarSource

Software Developer @ Language Team
http://sonarsource.com

[Tim Webster]

Hi Michael,

thanks for the detailed explanation.  Yes - configurable filters would be very useful indeed!

Thanks,

Tim