#630 ReaderOf test class is incomplete #676
Conversation
|
Job #676 is now in scope, role is |
Codecov Report
@@ Coverage Diff @@
## master #676 +/- ##
===========================================
+ Coverage 70.03% 73.6% +3.57%
- Complexity 1005 1088 +83
===========================================
Files 215 222 +7
Lines 3304 3361 +57
Branches 190 190
===========================================
+ Hits 2314 2474 +160
+ Misses 943 841 -102
+ Partials 47 46 -1
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
@proshin-roman Looks good overall, but I had a few comments. And please, link the parent issue in the PR's description.
| @@ -40,18 +41,317 @@ | |||
| * @checkstyle JavadocMethodCheck (500 lines) | |||
| * @checkstyle ClassDataAbstractionCouplingCheck (500 lines) | |||
| */ | |||
| @SuppressWarnings({"PMD.TooManyMethods", "PMD.AvoidDuplicateLiterals"}) | |||
There was a problem hiding this comment.
@proshin-roman Can you please avoid the duplicate literals? As I see, it's mostly the 'abc' string. Please write different chars for them to avoid this error.
In the tests where the same String appears twice, I would declare it in a final local variable.
There was a problem hiding this comment.
@amihaiemil I've extracted test content into a final static (!) field and got rid of other string constants.
| MatcherAssert.assertThat( | ||
| "Can't read content", | ||
| new InputOf(new ReaderOf('a', 'b', 'c')), | ||
| new InputHasContent("abc") |
There was a problem hiding this comment.
@proshin-roman Is it not possible to use Hamcrest matchers here? It seems strange to me, to use matchers from cactoos itself -- what if the matcher will have a bug in the future? Tests will break for that reason and it will be misleading.
I would use trusted Hamcrest matchers, even if you have to call asString() or something, on InputOf
Furthermore, it would be nice if we could test ReaderOf without having to wrap it inside InputOf -- what if the test fails because there is a bug in InputOf and ReaderOf is actually fine? Would that be too complicated?
There was a problem hiding this comment.
@amihaiemil I've tried to do that using one private method that does the assertion.
| ), | ||
| new InputHasContent("Hello, товарищ!") | ||
| ); | ||
| } |
There was a problem hiding this comment.
@proshin-roman Can you add a few tests with empty and null content? What happens if ReaderOf tries to read from an empty/null source?
There was a problem hiding this comment.
@amihaiemil I've added tests for null and empty string.
| ), | ||
| new InputHasContent("Hello, товарищ!") | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
@proshin-roman Can you also add a test for german characters? It is quite a spoken language, so we should be able to read german diacritics. The german diacritics are: äÄ üÜ öÖ and ß
There was a problem hiding this comment.
@amihaiemil That was done too.
|
@proshin-roman Looks ok to me, but I believe the architect will complain about that Let's see what he says though :) |
|
@rultor Good to merge |
@amihaiemil Thanks for your request. @yegor256 Please confirm this. |
|
@proshin-roman And please, edit all your comments (including review comments) and address them to me (they should start with |
|
@amihaiemil Exactly! I know about "no static" rule, but I believe it's ok for this test as it's really same content for all the test cases. Theoretically, we can avoid that by using something like |
|
@yegor256 ping |
| * Test content for all the tests in this class. | ||
| */ | ||
| private static final String CONTENT = | ||
| "Hello, товарищ äÄ üÜ öÖ and ß"; |
There was a problem hiding this comment.
@proshin-roman it's a bad idea, see http://www.yegor256.com/2016/05/03/test-methods-must-share-nothing.html
|
@proshin-roman see above |
|
@yegor256 @amihaiemil I've updated pull request: no static fields now. Please review it again. |
|
@amihaiemil/z this job was assigned to you 5 days ago. It will be taken away from you soon, unless you close it, see §8. Read this and this, please. |
|
@yegor256 we can merge this now, I think |
|
@rultor merge |
|
@ypshenychka/z please review this job, as in §30; the job will be fully closed and all payments will be made when the quality review is completed |
|
@amihaiemil/z this job was assigned to you 8 days ago. It will be taken away from you soon, unless you close it, see §8. Read this and this, please. |
|
@0crat quality good |
|
|
|
Order was finished, quality was "good": +20 points just awarded to @amihaiemil/z, total is +980 |
|
@ypshenychka You can't do that, unless you have one of these roles: |
|
@0crat status |
|
@0crat status |
|
@neonailol This is what I know about this job, as in §32:
|
|
@yegor256 this job was assigned to me by mistake |
|
@0crat refuse |
|
@0crat out |
Pull request for the issue #630. It increases test coverage of ReaderOf class up to 100%. All ctors are covered by successful tests now. No other changes.