-
Notifications
You must be signed in to change notification settings - Fork 170
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
#829 Turn AndInThreadsTest Stable #874
#829 Turn AndInThreadsTest Stable #874
Conversation
Job #874 is now in scope, role is |
Codecov Report
@@ Coverage Diff @@
## master #874 +/- ##
=========================================
Coverage 86.07% 86.07%
Complexity 1445 1445
=========================================
Files 259 259
Lines 3770 3770
Branches 212 212
=========================================
Hits 3245 3245
Misses 475 475
Partials 50 50 Continue to review full report at Codecov.
|
This pull request #874 is assigned to @paulodamaso/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 was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pbenety Thanks for the PR, I've made some remarks, please take a look at them.
); | ||
MatcherAssert.assertThat( | ||
list, | ||
Matchers.containsInAnyOrder("hello", "world") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pbenety We shouldn't use these static functions from Matchers
, please fix it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@paulodamaso What should I use instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pbenety Using IsIterableContainingInAnyOrder<>
from hamcrest will do the trick.
list.size(), | ||
Matchers.equalTo(2) | ||
list, | ||
Matchers.contains(1, 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pbenety We shouldn't use these static functions from Matchers
, please fix it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@paulodamaso What should I use instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pbenety Using IsIterableContainingInAnyOrder<>
from hamcrest will do the trick.
final List<String> list = new LinkedList<>(); | ||
final List<String> list = Collections.synchronizedList( | ||
new ArrayList<String>(2) | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pbenety What do you think about using cactoos SyncCollection
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@paulodamaso I thought about it but SyncCollection
does not implement add
method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pbenety Right, it needs to be mutable for the test, so that's ok for now. Just open a @todo
about removing these static methods access from here so we can think about it in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@paulodamaso @todo
added.
final List<String> list = new LinkedList<>(); | ||
final List<String> list = Collections.synchronizedList( | ||
new ArrayList<String>(2) | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pbenety What do you think about using cactoos SyncCollection
here too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@paulodamaso I thought about it but SyncCollection
does not implement add
method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pbenety Same as above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@paulodamaso @todo
added.
final List<Integer> list = new LinkedList<>(); | ||
final List<Integer> list = Collections.synchronizedList( | ||
new ArrayList<Integer>(2) | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pbenety What do you think about using cactoos SyncCollection
here, again?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@paulodamaso I thought about it but SyncCollection
does not implement add
method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pbenety Same as above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@paulodamaso @todo
added.
5a48ed9
to
3a1327f
Compare
@pbenety Please see Travis build details and correct build failure cause. |
3a1327f
to
30501cc
Compare
30501cc
to
688d6d5
Compare
@paulodamaso done |
@pbenety Everything looks good now, thank you for the fixes and the patience! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pbenety All good now. thanks.
@rultor good to merge |
@paulodamaso Thanks for your request. @llorllale Please confirm this. |
@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 9min) |
@elenavolokhova/z please review this job completed by @paulodamaso/z, as in §30; the job will be fully closed and all payments will be made when the quality review is completed |
The job #874 is now out of scope |
Payment to |
@0crat quality good |
Order was finished, quality is "good": +20 point(s) just awarded to @paulodamaso/z |
Quality review completed: +8 point(s) just awarded to @elenavolokhova/z |
Turn AndInThreadsTest stable for #829