1343 - replacing static MatcherAssert.assertThat with Assertion object #1344
Conversation
There was a problem hiding this comment.
@lxpdd here are two comments. Also I see you fixed this for many packages while the todo was about the io package.
And is there any more MatcherAssert in the cactoos code? If yes please add a new todo for the remaining ones, if no please add MatcherAssert.assertThat to the forbidden api file as explained in the todo.
In any case, you need to remove the todo you fixed too.
| @@ -52,7 +52,8 @@ public void copiesFromCharArrayWithCharsetToFile() throws IOException { | |||
| final String input = | |||
| "Hello, товарищ file #1 äÄ üÜ öÖ and ß"; | |||
| final File output = this.folder.newFile(); | |||
| MatcherAssert.assertThat( | |||
| new Assertion<>( | |||
There was a problem hiding this comment.
@lxpdd you need to call affirm() on all the Assertion objects for the assertion to be verified. Currently those tests do nothing.
| @@ -52,7 +52,8 @@ public void copiesFromCharArrayWithCharsetToFile() throws IOException { | |||
| final String input = | |||
| "Hello, товарищ file #1 äÄ üÜ öÖ and ß"; | |||
| final File output = this.folder.newFile(); | |||
| MatcherAssert.assertThat( | |||
| new Assertion<>( | |||
| "char array can't be copied to the file with charset UTF_8", | |||
There was a problem hiding this comment.
@lxpdd please use the positive form with must (e.g., "char array must be copied to the file with charset UTF_8") everywhere
|
@0crat status |
|
@victornoel This is what I know about this job in C63314D6Z, as in §32:
|
|
@lxpdd ping |
2) change message in test to positive form 3) add new TODO
|
@victornoel I fixed that problem in multiple packages because there was only 3 classes in io packages. |
| @@ -52,7 +52,8 @@ public void copiesFromCharArrayWithCharsetToFile() throws IOException { | |||
| final String input = | |||
| "Hello, товарищ file #1 äÄ üÜ öÖ and ß"; | |||
| final File output = this.folder.newFile(); | |||
| MatcherAssert.assertThat( | |||
| new Assertion<>( | |||
There was a problem hiding this comment.
@lxpdd you are missing affirm() on this one
| @@ -25,6 +25,10 @@ | |||
| /** | |||
| * Scalars, tests. | |||
| * | |||
| * @todo #1350:30min Continue replacing usage of MatcherAssert.assertThat with | |||
There was a problem hiding this comment.
@lxpdd the bug you are fixing is not 1350 but 1343 I believe, no?
There was a problem hiding this comment.
Yes, but how could I add another TODO if I didn't replace all MatcherAssert.assertThat in the project?
There was a problem hiding this comment.
@lxpdd the principle is the following: when you add a todo, you start it with #xxx:30min with xxx being the ticket currently being solved. so that when someone solve your todo, then know where does it come from.
So, currently you are solving the ticket #1343, then because we consider that there is more work to finish the ticket you are doing, you remove the previous todo and you need to add a new todo starting with #1343:30min so that someone else can continue the work until there are no MatcherAssert.assertThat in the project.
There was a problem hiding this comment.
okay, I fixed that, could you check it, please?
|
@lxpdd see my last two comments |
Codecov Report
@@ Coverage Diff @@
## master #1344 +/- ##
============================================
+ Coverage 89.60% 89.61% +0.01%
- Complexity 1653 1654 +1
============================================
Files 273 274 +1
Lines 3933 3937 +4
Branches 211 211
============================================
+ Hits 3524 3528 +4
+ Misses 376 375 -1
- Partials 33 34 +1
Continue to review full report at Codecov.
|
|
@lxpdd thanks :) @paulodamaso good to merge |
|
@paulodamaso ping |
|
@iiylll @victornoel Thank you guys |
|
@rultor merge |
@paulodamaso OK, I'll try to merge now. You can check the progress of the merge here |
@paulodamaso Done! FYI, the full log is here (took me 9min) |
|
@sereshqua/z please review this job completed by @victornoel/z, as in §30; the job will be fully closed and all payments will be made when the quality review is completed |
|
Code review was too long (8 days), architects (@paulodamaso) were penalized, see §55 |
|
@victornoel as @iiyll user's acc does not exist already, i still have to put quality acceptable here, because he was not starting his comments with the name of the user, see |
|
@0crat quality acceptable |
|
@sereshqua its user account name just changed after the PR was merged :) I updated my comments anyway now. Can you re-assert quality? it didn't seem to work |
|
@0crat quality acceptable |
1343
and I've changed it, now it calls what it supposed to call.