SONARJAVA-1793: Add support for Truth framework assertions.

[Johnnei]

Please ensure your pull request adheres to the following guidelines:

• Use the following formatting style: SonarSource/sonar-developer-toolset
• Unit tests are passing and you provided a unit test for your fix
• ITs should pass : To run ITs locally, checkout the README of the project.
• If there is a Jira ticket available, please make your commits and pull request start with the ticket number (SONARJAVA-XXXX)

A method invocation on the 'Subject' base-type of the Truth framework will
be considered as an assertion call.

[benzonico]

Hi @Johnnei,

Thanks for the contribution. However I am a bit bothered by the choice of implementation here as it is done in such a way that both this rule and https://jira.sonarsource.com/browse/RSPEC-2970 cases are covered. I don't think this should be the case here given the current implementation (ie, either we merge all the cases in a single rule or we keep things separated but not half/half).

[Johnnei]

Hi @benzonico,

I've chosen this path because I saw similar test-cases for the "fest assertions" (Example: https://github.com/Johnnei/sonar-java/blob/0530e79bb0960bc769a9f6bf624cffd687757c02/java-checks/src/test/files/checks/AssertionsInTestsCheckJunit4.java#L121) so I (sadly) did the assumption that those cases should be included within the scope of this rule.

Deciding whether the rules should be merged or the implementation be split is probably affecting more than just the Java Analyzer, so I'll pass that question back to: Which direction do you prefer to go:

• Split the implementation into their respective checks (If so: should incomplete assertions be considered as assertions?)
• Merge the two RSPECs

[benzonico]

@Johnnei could you please rebase your PR on master : I deactivated PR analysis so, at least CI can run on travis. Thanks.

We'll get back to you on functional choices.

[Johnnei]

@benzonico I'll rebase all my PR's once I'm home. I don't have the private key of my PGP key available at my office laptop.

[benzonico]

@Johnnei No pblm. FYI : your PRs are most likely to be on stale state for a week or two as there will be a small pause on java analyzer plugin development. No worry though, they won't be forgotten.

[Johnnei]

@benzonico Thanks for the heads-up!

[Johnnei]

@benzonico Done 😄

[m-g-sonar]

Hey,

Just coming back to you @Johnnei. Nice catch about spotting conflict between rules.
Let's keep the rules separated and be coherent in their implementations.

To do so, let's consider incomplete assertions as being assertions anyway (from a RSPEC-2699 point of view), and raise issues in implementation of RSPEC-2970 for all the frameworks with incomplete assertions.

Now, regarding your implementation, couldn't you use a method matcher to match the method from Truth? A nameCriteria.startingWith("assert") should be enough, with no parameter constraints.

[Johnnei]

Hey @m-g-sonar,

If I recall correctly my initial implementation was 2 new matchers (didn't spot the startWiths one). But with those I saw several test-cases (the incomplete assertions) which felt wrong to me so I started comparing with the other supported frameworks which supported incomplete calls, and thus made the wrong assumptions.

As for your functional decision: I do agree. I've been thinking about this as well and found it to be justifiable to keep both RSPECS in order to be able to flag issues in the scenario: x valid assertions and y incomplete assertions.

With this conclusion I would argue that the non-compliant example in RSPEC-2699 should be adjusted as well as with this case the incomplete fest assertion would be fine for RSPEC-2699 and raise an issue on RSPEC-2970. I would also expect some form of duplication of the implementations of the two RSPECS if that case should raise an issue in both RSPECS.

I'll rework this one to split the implementation to correctly cover both rules. Also please do let me know if I should do the same for the other frameworks (ex. the Mockito.verify(mock) and the fest ones), or if that should be a separate ticket.

[Johnnei]

Hey @m-g-sonar, I've rebased and reworked the commit to split the checks over the two rules. Thanks for the tip about NameCriteria, made the code a lot simpler to work with!

[m-g-sonar]

Okay @Johnnei, nice work. I created ticket SONARJAVA-1830 in order to handle the issue with separation of responsibilities between the 2 rules. I also updated title of SONARJAVA-1793 to highlight the fact that both rules have been modified.

I'm going to open a new PR with your commit in order trigger QA and PR analysis to check that everything is alright (that's boring, but we are working on it!), then I'll merge it! Note that it looks good to me.

[m-g-sonar]

QA green.
PR analysis reported issue on line 64 : Define a constant instead of duplicating this literal "assert" 3 times.
I'll let you fix that and I'll merge.

[Johnnei]

@m-g-sonar Whoops, looks like I failed to run a quick SonarLint analysis. Will fix this later today!
(If I find a moment to do this at office then it will be a non-signed commit as I don't store a PGP pair on my office laptop)

[m-g-sonar]

No worries, take your time. We are not in a hurry.

[Johnnei]

@m-g-sonar, Fixed the flaw.

[m-g-sonar]

Merged! Thanks for your contribution! :)

[Johnnei]

Glad to be of help!