[VOTE] Java plugin version 3.5

[Michael Gumowski]

Hey all,

For this release we focused on implementing new rules, and hardening or improving existing ones. A few features have been also included and numerous bugs have been fixed.

It is consequently time to ask for the vote to release version 3.5 of the java plugin.

This version brings notably the following features:

• 6 new rules:
• Exceptions should not be thrown from servlet methods
• "Thread.sleep" should not be used in tests
• JUnit rules should be used
• Classes should not be loaded dynamically
• Multiple loops over the same set should be combined
• Methods and field names should not be the same or differ only by capitalization
• Custom rules on test files (Thanks to Michel Pawlak's contribution!)
  
• @SuppressWarning rule's behavior changed from blacklist to whitelist (Thanks to Adam Gabryś' contribution!)
• Support of the annotation @SuppressWarnings at field and variable level, and not only at class and method level (suppressing SQ issues using the annotation)
  

Thanks to Massimo Paladin for helping out on new rules and bug fixes, and Nicolas Peru for it's precious work.

Issues solved: 

https://jira.sonarsource.com/jira/secure/ReleaseNote.jspa?projectId=10973&version=12422

You can test this version using the following snapshot : 

https://github.com/SonarSource/sonar-java/releases/download/3.5-RC1/sonar-java-plugin-3.5-SNAPSHOT.jar

Vote is open for 72 hours.

[ ] +1

[ ] +0

[ ] -1

Any feedback, as ever, more than welcome !

Michael GUMOWSKI | SonarSource

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

[Michel Pawlak]

Hi Michael,

It's unfortunate that you launch votes on Friday evening. Indeed during the week end some people cannot test the release in a professional context, and leaving one day is rather short depending on the workload (for instance I won't be able to test it before the end of the day, probably after 5pm.) I'll try to give you a feedback later anyway,

Would it be possible to extend the vote duration until tomorrow (if you have no feedback) ?

Kind regards,

Michel

[Nicolas Peru]

Hi Michel, 

We usually ends sprints on friday and launch the votes at that point. As we want to release before comitting back to master for the new sprint, that also mean we want to reduce the amount of time where we would be "blocked". 

However you are indeed right in the fact that it let's very few time for people to test during workday. 

That is also why we usually let people give feedback a little bit longer than the strict 72h. Let's say we will keep the vote open until tuesday at least. 

However, even if vote is closed, feedback is always precious and welcome (it will just get more time to ship a fix if required). 

So feel free to test this evening. 

Cheers,

Nicolas PERU | SonarSource

Senior Developer
http://sonarsource.com

https://twitter.com/benzonico

--
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/20367ec0-5066-4a29-979d-e70bde0c91d3%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

[dfl...@objectif-informatique.fr]

Hi,

+1

Tested with SonarQube 4.5.5
SONARJAVA-1213 : OK
Non-regression with custom rules (after modification of the call at the registrarContext API ): OK
Non-regression with sample project : OK

Regards,

Denis

[Michel Pawlak]

Hi again,

+1 could not run it against all our projects, but I saw no problem so far.

Will try with custom test rules tomorrow.

Thanks

Michel

[Michael Gumowski]

Hey all,

Thank you Denis & Michel for your tests so far!

@Michel : I'll wait for your feedback regarding your trial on custom test rules before closing the vote!

Regards,

Michael

Michael GUMOWSKI | SonarSource

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

--
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/1ce4530b-024a-4959-83bb-b0d7a50e0f33%40googlegroups.com.

[Michał Kordas]

Hi,

+0 from me.

I haven't seen any regression and generally new rules are really useful, but two things I noticed:

• Rule Methods should not be empty squid:S1186 triggers on empty public constructors without arguments even if other constructors are provided:
  public class A {
      int value;
  
      public A() {
      }
  
      public A(int value) {
          this.value = value;
      }
  }
  Do we really need additional comment there that empty constructor is needed to add possiblity to create class A without necessity to provide value?
  
  
• JUnit rules should be used squid:S2924 - why this rule does not include unused Expected Exception rule when it's set to none?
  
  

  @Rule public ExpectedException expectedException = none();
  
  or even
  
  @Rule public final ExpectedException expectedException = ExpectedException.none();
  
  

Thanks,

Michal Kordas

[Michael Gumowski]

Hello Michał,

Thanks for your tests and the feedback regarding the new rules. However, your remarks and any potential modification will not be handled for the current release, as they doesn't describe false positives or unexpected blocking behavior.

Can I ask you to create new thread in the SQ google group to discuss about these two rules? I now also think that they can be improved.

My remarks:

• Regarding the empty public constructors with no arguments, yes, for the time being you definitely have to add a comment in its body.
  Another approach could be to also consider javadoc for these cases. Currently only comments within the body will invalidate the issue. I tend to agree that it's maybe too much.
• Regarding JUnit rules, it could indeed be a nice addition to the rule but it need to be discussed first.

Regards,

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/37dcd150-c27d-47cb-9901-dfcc9b2922db%40googlegroups.com.

[Michael Gumowski]

Hey all,

I just received an email from Michel and unfortunately he won't have time to proceed further with his tests.

I consequently close the vote and I will continue with the release.

Results:

[2] +1 : Thx Michel & Denis

[1] +0 : Thx Michał for your feedback on new rules

[0] -1

Regards,

Michael GUMOWSKI | SonarSource

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

[Michel Pawlak]

Hi Michael,

I finally could upgrade our test instance, adapt our checks and run a full analysis.

1) All our custom test checks behave as expected so thanks again for having accepted my PR. I already have requests for more checks on test source.

2) One comment about S2924 : If I understand the check orrectly if the rule instance is not used,a violation is raised. If it's true, we won't be able to use this Check as we have Junit Rules such as this one :

@Category(SystemIntegrationTest.class)
public class MyJagacyTest {

    @Rule
    public ClientConfigurationRule rule = new ClientConfigurationRule(this);

    @ConfigureClient(credentials=CredentialsType.FULL_ACCESS, target="target1", sessionName="session1")
    public Client client1;

    @ConfigureClient(credentials=CredentialsType.FULL_ACCESS, target="target2", sessionName="session2")
    public Client  client2;

    @Test
    public void method_should_doSomething_with_client1() {
        // here test code for client1.
    }

    @Test
    public void method_should_doSomethingElse_with_client1() {
        // here test code for client1.
    }

    @Test
    public void method_should_doSomething_with_client2() {
        // here test code for client2.
    }

}

The rule checks that the class is correctly categorized, detects all Client fields, checks their annotation and instanciates under the hood the clients with information provided as parameters but also environment information provided through environment variables (these are system integration tests). All in all no call is done on the rule instance itself, except for its instanciation which takes the test class as parameter)

Further, if I'm not wrong the same problem will occur with the powermock Rule as you don't have to interact with it..

Cheers,

Michel

[Michael Gumowski]

Hello Michel,

Thank you for your feedback and for all the request you may have regarding test oriented rules! Any help on that subject is really welcome!

Regarding S2924, your understanding is wrong (maybe the description of the rule is not good enough). The check with only raise issues for fields of type "TemporaryFolder" or "TestName", when annotated wtith "@Rule".

In your case, as the field has type "ClientConfigurationRule", it won't be considered at all and not issue will be raised. Same for powermock.

Regards,

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/f886767f-2f98-4618-bf02-89736bad008e%40googlegroups.com.

[arjen.v...@gmail.com]

I just now found the time to test this new release with our custom rules. 

And like Denis run into the problem with 'registrarContext.registerClassesForRepository' which now only accepts 3 parameters instead of 2.

How am I to create a custom-jar that is able to work with Sonar-Java 3.4 and 3.5 ?

Use reflection to see which of either methods is there seems a bit ugly, am I missing something ?

Regards,

Arjen

[Michael Gumowski]

Hello Arjen,

You raise a very good point. Unfortunately a bit too late to be able to include a potential fix in version 3.5, which have already been released...

It is indeed a breaking change in the API and it is consequently not possible to create a jar compatible with both version. Reflection can do the trick, but, as you said, it will be hideous.

Out of curiosity, why would you need to use the same custom-jar for two different version of the java plugin? Multiple SQ environment?

Regards,

Michael GUMOWSKI | SonarSource

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

--

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/91c6ef52-b019-49b3-8414-2ce6562c42f2%40googlegroups.com.

[arjen.v...@gmail.com]

Hi,

We create a plugin for a company that uses it to monitor its subscontractors.

So the plugin is indeed deployed on several sites, even at different companies who (might) run different sonarqube setups.

It might be a good idea to try to keep the API at least binary compatible between minor releases. But I suppose this one just slipped through the review process.

I'll go for a reflection hack for the time being :-)

Regards,

Arjen

[Michel Pawlak]

Being the one who made the pull request, I'll try to answer your question from my point of view.

I would completely agree with you if the CheckRegistrar class was marked as stable. However it is annotated with @com.google.common.annotations.Beta. 

Were you aware of this ?

When people use an api they should first check if it is stable, then if they use a @Beta api they have to accept that this api can change. Prior to using it, users have to evaluate the benefits / risks of using beta products or apis. But once they use them (because they can make or save money thanks to this move) they can't complain about having to change their code if the api changes.

In my company we also had to modify our custom plugins due to this change (even if I'm the one who made the PR). None complained : writing custom checks gives much more benefits than having to modify a plugin due to such an api change.

Back to your question about how you can have two different versions of your plugin using two versions of sonar java plugin, Here is a suggestion : think of your plugin as an integrator / archtect would think instead of how a developer does. For instance, create a Maven project as follows :

root -> put here all common dependencies, stuff, etc.

root/myplugin-java34 -> module that depends on java plugin 3.5 and on "core", put here your SonarPlugin + your JavaChecksRegistrar implementation for java plugin 3.4 -> this module activates the creation of a sonar plugin

root/myplugin-java35 -> module that depends on java plugin 3.5 and on "core", put here your SonarPlugin + your JavaChecksRegistrar implementation for java plugin 3.5 -> this module activates the creation of a sonar plugin

root/core -> core module of your plugin, put here all your checks, other sensors, decorators... You can even put an abstract version of your plugin here as well as an abstract class with all your checks (that does not implement CheckRegistrar) if you don't want to duplicate code

then "cd root", "mvn clean install", -> you have two sonar plugins, with two different names (configurable) without having used reflection

Kind regards,

Michel

[arjen.v...@gmail.com]

Hi,

I know it is marked as @Beta, however it is also the method used in the example sonar java plugin project, so it seems to be the default way to go.(?)

And given that it would be easy to just overload the method next time arguments are changed, I just consider it something that slipped through and would have been fixed otherwise.

Also, thank you for your suggestion for a modular approach with maven to create a 3.4 and a 3.5 jar, it's a nice clever alternative !

However, having one jar is preferable if you have to update multiple 'customers' and servers, to prevent them from accidentally upgrading and breaking the custom plugin.

Regards,

Arjen

[Michel Pawlak]

Hi,

While I understand your wish, I can just suggest to read the Beta annotation javadoc : http://docs.guava-libraries.googlecode.com/git/javadoc/com/google/common/annotations/Beta.html

Signifies that a public API (public class, method or field) is subject to incompatible changes, or even removal, in a future release. An API bearing this annotation is exempt from any compatibility guarantees made by its containing library. Note that the presence of this annotation implies nothing about the quality or performance of the API in question, only the fact that it is not "API-frozen."
It is generally safe for applications to depend on beta APIs, at the cost of some extra work during upgrades. However it is generally inadvisable for libraries (which get included on users' CLASSPATHs, outside the library developers' control) to do so.

Kind regards,

Michel