#838 close body in RsPrint #879
Conversation
|
Job #879 is now in scope, role is |
Codecov Report
@@ Coverage Diff @@
## master #879 +/- ##
============================================
- Coverage 75.45% 75.45% -0.01%
Complexity 977 977
============================================
Files 220 220
Lines 4718 4717 -1
Branches 368 368
============================================
- Hits 3560 3559 -1
Misses 1003 1003
Partials 155 155
Continue to review full report at Codecov.
|
|
This pull request #879 is assigned to @g4s8/z, here is why; the budget is 15 minutes, see §4; please, read §27 and when you decide to accept the changes, inform @paulodamaso/z (the architect) right in this ticket; if you decide that this PR should not be accepted ever, also inform the architect; this blog post will help you understand what is expected from a code reviewer; there will be a monetary reward for this job |
| new RsPrint(new RsText(input)) | ||
| .printBody(output); | ||
| } catch (final IOException ex) { | ||
| if (!ex.equals(exception)) { |
There was a problem hiding this comment.
@olenagerasimova it's better to use assertions in tests instead of exception throwing:
MatcherAssert.assertThat("wrong exception", ex, new IsEquals(exception));| * Have input been closed? | ||
| * @return True, if input wes closed, false - otherwise | ||
| */ | ||
| public boolean haveClosed() { |
There was a problem hiding this comment.
@olenagerasimova it would be more conventional to call method in present simple: haveClosed -> isClosed, because it describes current object state.
| throw ex; | ||
| } | ||
| } | ||
| MatcherAssert.assertThat( |
There was a problem hiding this comment.
@olenagerasimova it's a good practice to add description text to assertThat if we have more than one assertions in test.
|
@g4s8 thanks for comments, fixed |
|
@rultor merge |
| new RsPrint(new RsText(input)) | ||
| .printBody(output); | ||
| } catch (final IOException ex) { | ||
| MatcherAssert.assertThat( |
There was a problem hiding this comment.
@olenagerasimova What are we asserting with this? We should have just one assertion per test, so please remove it and create another test method for it if this assertion is relevant.
|
@paulodamaso updated |
|
@olenagerasimova Right, thanks. Just for clarification, that exception is not relevant to that test case, right? |
|
@paulodamaso I don't think it's relevant, this particular test method is not about exception, it's about flush. |
|
@olenagerasimova Great, thanks |
|
@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 23min) |
|
@ypshenychka/z please review this job completed by @g4s8/z, as in §30; the job will be fully closed and all payments will be made when the quality review is completed |
|
The job #879 is now out of scope |
|
Payment to |
|
@0crat quality good |
|
Quality review completed: +8 point(s) just awarded to @ypshenychka/z |
For #838 I've made
RsPrint.printBody()close body and added corresponding test.