(#106) Introduces Mismatches Matcher to test Matchers #131
Conversation
|
Job #131 is now in scope, role is |
|
This pull request #131 is assigned to @vzurauskas/z, here is why; the budget is 15 minutes, see §4; please, read §27 and when you decide to accept the changes, inform @llorllale/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 no monetary reward for this job |
Codecov Report
@@ Coverage Diff @@
## master #131 +/- ##
============================================
+ Coverage 97.45% 97.65% +0.19%
- Complexity 103 108 +5
============================================
Files 22 23 +1
Lines 236 256 +20
Branches 3 3
============================================
+ Hits 230 250 +20
Misses 6 6
Continue to review full report at Codecov.
|
|
@vzurauskas ping |
|
@0crat refuse - you're giving me too many PRs in the agenda. |
@vzurauskas The user @vzurauskas/z resigned from #131, please stop working. Reason for job resignation: Order was cancelled |
|
Tasks refusal is discouraged, see §6: -15 point(s) just awarded to @vzurauskas/z |
|
@vzurauskas ping? |
|
@vzurauskas sorry, didn't see you resigned, ignore me :) |
|
@llorllale any way to get a review there maybe? |
|
@victornoel please leave a todo for adding a ctor that only accepts the args without the strings |
|
@llorllale are we sure we want to do that? The point is to test that a matcher is complete in its implementation, descriptions included, if we do this we expose ourselves to undertested matchers then. |
|
This pull request #131 is assigned to @scristalli/z, here is why; the budget is 15 minutes, see §4; please, read §27 and when you decide to accept the changes, inform @llorllale/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 no monetary reward for this job |
|
@victornoel right, disregard my previous comment |
|
@rultor merge |
@llorllale OK, I'll try to merge now. You can check the progress of the merge here |
@llorllale @victornoel Oops, I failed. You can see the full log here (spent 3min) |
|
@llorllale is there anyway to bypass the checks? Because there are |
There was a problem hiding this comment.
@victornoel review done, left some comments. I know ARC has already approved and tried to merge, but 0crat assigned REV to me and I thought I'd complete it anyway.
| * Assertion is working and throwing errors. | ||
| * @checkstyle ProtectedMethodInFinalClassCheck (200 lines) | ||
| */ | ||
| public final class Mismatches<X> extends TypeSafeDiagnosingMatcher<Matcher<X>> { |
There was a problem hiding this comment.
@victornoel since there is a task for refactoring all matchers to extend MatcherEnvelope, isn't it better to do so from the start here?
There was a problem hiding this comment.
@scristalli I could, but this would make the Mismatches class very hard to read I believe. When there will be extra code for solving the @todos of the class, it will be even harder. MatcherEnvelope is nice when the 3 implementations of the methods are very simple.
There was a problem hiding this comment.
@scristalli see also #135
There was a problem hiding this comment.
@victornoel I see your point, and read the new issue you opened. I still think this creates somewhat of a special case for #129 (and similar) though, should be either clarified or avoided. I think ARC has the final decision here.
|
|
||
| // @todo #106:30min Add a description in case the mismatch fails. | ||
| // And then introduce some tests to validate that the description | ||
| // is properly constructed. |
There was a problem hiding this comment.
@victornoel just a (totally minor) style concern: I always see puzzles with /* */ instead of //. Although this is valid, I think that sticking to the same formatting everywhere can be better for the project.
There was a problem hiding this comment.
@scristalli good point, but if I use /* */ I get the following error from qulice:
Checkstyle: src/main/java/org/llorllale/cactoos/matchers/Mismatches.java[145]: Overridden methods should not have Javadoc (NoJavadocForOverriddenMethodsCheck)
victornoel
force-pushed
the
106-mismatches
branch
from
1da23d5 to
7ec1800
Compare
|
@scristalli thanks for the review, I've answered your comments and pushed some changes that also I hope improve coverage |
| "does not match lines that do not contain the given strings", | ||
| String.format("Tom%nJohn%n"), | ||
| new HasLines("Tom", "Mike") | ||
| "must not match lines that do not contain the given strings", |
There was a problem hiding this comment.
@victornoel [EDITED] Pardon me, I made a major mistake on one thing I thought had happened (force-pushing messing up review comments, which were only hidden). I guess I shouldn't comment late at night.
[EDITED 2,3] Still, I think force-pushing makes tracking a lot harder. If I just look at the code from the new commit on GitHub, the comment is just gone (because of the line change). If I look at the review (which I must do at this point not to get confused), I have to CTRL+F for "resolved", because I could easily miss the minuscule UI element (see image), even if I wasn't a noob (reason I made a mistake this time). For instance, I could have a lot of REV tasks and not remember making a particular comment.
Not your fault at all, but I just think that with separate commits I'd have less chances to get confused and miss stuff (all comments where I put them even if cropped in UI as "resolved", clean and separate history (see my rant on the "force-pushed" link in cactoos#1154)).
There was a problem hiding this comment.
@scristalli yep, I hate the way github works because only the author can resolve comments: in gitlab for example, anybody can mark the comment as resolved and so usually, it is the reviewer that resolve comments, not the PR author. In the future I think I won't resolve any comment anymore to avoid such confusion.
There was a problem hiding this comment.
@scristalli I've raised the discussion on telegram if you want to join (https://t.me/zerocracy/7175), I think my position is not as strong as I would think :)
|
@llorllale I approve the changes (altough I would point your attention on the discussion on edits on |
victornoel
force-pushed
the
106-mismatches
branch
from
7ec1800 to
c2c9417
Compare
|
@scristalli @llorllale I've added one more test so that coverage is improved (I was missing the failing case). It's a bit strange because I used mismatch to test mismatch, but I think it's ok for now |
victornoel
force-pushed
the
106-mismatches
branch
from
c2c9417 to
3a33a27
Compare
|
@llorllale ping |
|
@llorllale could we get this merged please? |
|
@rultor merge |
@llorllale OK, I'll try to merge now. You can check the progress of the merge here |
@llorllale Done! FYI, the full log is here (took me 3min) |
|
There is an unrecoverable failure on my side. Please, submit it here: 0.48.2: CID: f7145179-e29c-4c15-8bc7-a38593179a31, Type: "Close job" |
|
There is an unrecoverable failure on my side. Please, submit it here: 0.48.2: CID: f7145179-e29c-4c15-8bc7-a38593179a31, Type: "Close job" |
|
@llorllale 0crat is failing here, is it possible to manually issue |
|
The job #131 is now out of scope |
|
Code review was too long (15 days), architects (@llorllale) were penalized, see §55 |
|
@sereshqua/z please review this job completed by @scristalli/z, as in §30; the job will be fully closed and all payments will be made when the quality review is completed |
|
@scristalli please make sure that you will find at least three problems in the code during next CR (ones, that will be accepted and fixed) |
|
Payment to |
|
@sereshqua please double check, I think I challenged |
|
@scristalli how many were actually fixed? :) i discussed with Kirill (architect in zerocracy) and he told that the actual fixed ones should be counted |
|
@sereshqua I see, I wasn't aware of that. Regarding your first comment: I always try to challenge |
|
@scristalli yeap :) it all makes sense what you are saying, agree with you, but that is the part of the job :) |
|
@0crat quality acceptable |
|
Order was finished, quality is "acceptable": +15 point(s) just awarded to @scristalli/z |
|
Quality review completed: +4 point(s) just awarded to @sereshqua/z |
This is for #106.