#721: Add matcher for TeeInput class #731
Conversation
|
Job #731 is now in scope, role is |
Codecov Report
@@ Coverage Diff @@
## master #731 +/- ##
============================================
+ Coverage 77.03% 77.87% +0.83%
- Complexity 1147 1165 +18
============================================
Files 222 223 +1
Lines 3349 3367 +18
Branches 191 191
============================================
+ Hits 2580 2622 +42
+ Misses 722 698 -24
Partials 47 47
Continue to review full report at Codecov.
|
|
@neonailol ping :) |
| */ | ||
| public TeeInputHasResult( | ||
| final Text expected, | ||
| final Text copied) { |
There was a problem hiding this comment.
formatting is not consistent
| */ | ||
| public TeeInputHasResult( | ||
| final String expected, | ||
| final Text copied) { |
There was a problem hiding this comment.
formatting is not consistent
| public boolean matchesSafely(final TeeInput item) { | ||
| this.actual = new ComparableText(new TextOf(item)); | ||
| return | ||
| this.expected.compareTo(this.actual) == 0 |
There was a problem hiding this comment.
Please use org.cactoos.scalar.Equals here
| new FormattedText( | ||
| TeeInputHasResult.DESCRIPTION, | ||
| new UncheckedText(this.expected).asString(), | ||
| new UncheckedText(this.expected).asString() |
There was a problem hiding this comment.
Duplicated line new UncheckedText(this.expected).asString() possible something other should be here
|
@neonailol Thanks for your comments - they were useful. I've fixed all of them except a comment about inconsistent formatting: I've changed another ctor where the closing bracket |
|
@proshin-roman thanks looks good now |
|
@rultor good to merge |
@neonailol Thanks for your request. @yegor256 Please confirm this. |
|
@yegor256 ping |
|
@yegor256 Please confirm this. |
|
@proshin-roman to be honest, I don't like this at all. Why do we need this matcher? What's wrong with existing |
|
@yegor256 well, TeeInput class has a lot of ctors (about 50) and all of them should be covered by tests (there is a ticket for that). It means double-assert will be copy-pasted for all these test cases. Another problem of double-assert: you have to trigger them in exact order otherwise they fail. This new matcher allows to avoid both of these problems. Finally, it's a goal of the ticket - you can decline this feature at all as the Arch of the project. And then I will have a reason to use double-assert in my other pull request. |
|
@neonailol/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. |
|
@rultor merge |
|
@yegor256 damn, the pull request is going to be merged but the ticket is already resigned from me. I would be very happy if you assign it back to me, please :) |
|
@elenavolokhova/z please review this job completed by @neonailol/z, as in §30; the job will be fully closed and all payments will be made when the quality review is completed |
|
The job #731 is now out of scope |
|
@neonailol According to our Policy:
Only two issues were fixed and actually improved the code quality in this case. |
|
@elenavolokhova i confirm |
|
@0crat quality acceptable |
|
Order was finished, quality is "acceptable": +15 points just awarded to @neonailol/z |
|
Quality review completed: +8 points just awarded to @elenavolokhova/z |
It is for issue #721: adds a new matcher that checks result of TeeInput class: compares both TeeInput's results (direct and copied) to the expected value. As well this new matcher has been applied to existing tests of TeeInput class.