Fix blocker issues on Sonar #508 #810
Conversation
| public void testName() throws Exception { | ||
| // given | ||
| String name = "Foobar"; | ||
| assertThat(simpleObject.getName()).isNull(); |
There was a problem hiding this comment.
We should use assertNull(simpleObject.getName()) here.
There was a problem hiding this comment.
What I feel is, it's good that AssertJ was used for assertions. It makes assertions readable, I was planning to either use Hamcrest or AssertJ in all modules. So I think we should keep assertion as is. And I will raise other issue to discuss assertion options.
There was a problem hiding this comment.
What I feel is, it's good that AssertJ was used for assertions. It makes assertions readable, I was planning to either use Hamcrest or AssertJ in all modules. So I think we should keep assertion as is. And I will raise other issue to discuss assertion options.
There was a problem hiding this comment.
What I feel is, it's good that AssertJ was used for assertions. It makes assertions readable, I was planning to either use Hamcrest or AssertJ in all modules. So I think we should keep assertion as is. And I will raise other issue to discuss assertion options.
There was a problem hiding this comment.
What I feel is, it's good that AssertJ was used for assertions. It makes assertions readable, I was planning to either use Hamcrest or AssertJ in all modules. So I think we should keep assertion as is. And I will raise other issue to discuss assertion options.
| simpleObject.setName(name); | ||
|
|
||
| // then | ||
| assertThat(simpleObject.getName()).isEqualTo(name); |
There was a problem hiding this comment.
Use assertEquals(name, simpleObject.getName()) here.
|
@staillebois we should use |
|
Ok I will do it today, thanks for the review. You just want I replace Assertj with JUnit that's right ? |
|
Done. |
|
@staillebois @IAmPramod I am confused. Were sonar blockers regarding the use of AssertJ? I personally feel that AssertJ and Hamcrest assertions read much better as compared to JUnit's. I was even planning to include them in all modules. @iluwatar Any thoughts on this? |
|
What I feel is, it's good that AssertJ was used for assertions. It makes assertions readable, I was planning to either use Hamcrest or AssertJ in all modules. So I think we should keep assertion as is. And I will raise other issue to discuss assertion options. |
|
Hi @npathai , I am agree with you AssertJ produce better logs. I replaced it only because of @IAmPramod 's comment and I was thinking that it was for remove the dependency with AssertJ. |
staillebois
changed the title
|
@staillebois It's all right now that we have already removed it. We can work on incorporating AssertJ or Hamcrest or Fest in all modules via other issue. Looks good 👍 |
|
@npathai good idea to improve the assertions in separate issue |
|
@staillebois Thanks for the contribution 👍 |
4 participants
Fix blocker issues on sonar
Solves #508
naked-objects-dom
naked-objects-integtests