Skip to content
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

Merged
merged 2 commits into from
May 22, 2018

Conversation

pbenety
Copy link
Contributor

@pbenety pbenety commented May 16, 2018

Turn AndInThreadsTest stable for #829

@0crat 0crat added the scope label May 16, 2018
@0crat
Copy link
Collaborator

0crat commented May 16, 2018

Job #874 is now in scope, role is REV

@codecov-io
Copy link

codecov-io commented May 16, 2018

Codecov Report

Merging #874 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            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.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4cf47f1...688d6d5. Read the comment docs.

@0crat
Copy link
Collaborator

0crat commented May 16, 2018

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

Copy link
Contributor

@paulodamaso paulodamaso left a 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")
Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor

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)
Copy link
Contributor

@paulodamaso paulodamaso May 17, 2018

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.

Copy link
Contributor Author

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?

Copy link
Contributor

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)
);
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

@pbenety pbenety May 22, 2018

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)
);
Copy link
Contributor

@paulodamaso paulodamaso May 17, 2018

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?

Copy link
Contributor Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pbenety Same as above.

Copy link
Contributor Author

@pbenety pbenety May 22, 2018

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)
);
Copy link
Contributor

@paulodamaso paulodamaso May 17, 2018

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?

Copy link
Contributor Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pbenety Same as above.

Copy link
Contributor Author

@pbenety pbenety May 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@paulodamaso @todo added.

@pbenety pbenety force-pushed the 829_make_AndInThreads_test_stable branch from 5a48ed9 to 3a1327f Compare May 22, 2018 01:39
@paulodamaso
Copy link
Contributor

paulodamaso commented May 22, 2018

@pbenety Please see Travis build details and correct build failure cause.

@pbenety pbenety force-pushed the 829_make_AndInThreads_test_stable branch from 3a1327f to 30501cc Compare May 22, 2018 02:14
@pbenety pbenety force-pushed the 829_make_AndInThreads_test_stable branch from 30501cc to 688d6d5 Compare May 22, 2018 02:41
@pbenety
Copy link
Contributor Author

pbenety commented May 22, 2018

@paulodamaso done

@paulodamaso
Copy link
Contributor

@pbenety Everything looks good now, thank you for the fixes and the patience!

Copy link
Contributor

@paulodamaso paulodamaso left a 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.

@paulodamaso
Copy link
Contributor

@rultor good to merge

@rultor
Copy link
Collaborator

rultor commented May 22, 2018

@rultor good to merge

@paulodamaso Thanks for your request. @llorllale Please confirm this.

@llorllale
Copy link
Contributor

@rultor merge

@rultor
Copy link
Collaborator

rultor commented May 22, 2018

@rultor merge

@llorllale OK, I'll try to merge now. You can check the progress of the merge here

@rultor rultor merged commit 688d6d5 into yegor256:master May 22, 2018
@rultor
Copy link
Collaborator

rultor commented May 22, 2018

@rultor merge

@llorllale Done! FYI, the full log is here (took me 9min)

@0crat
Copy link
Collaborator

0crat commented May 22, 2018

@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

@0crat 0crat removed the scope label May 22, 2018
@0crat
Copy link
Collaborator

0crat commented May 22, 2018

The job #874 is now out of scope

@0crat
Copy link
Collaborator

0crat commented May 22, 2018

Payment to ARC for a closed pull request, as in §28: +10 point(s) just awarded to @llorllale/z

@elenavolokhova
Copy link

@0crat quality good

@0crat
Copy link
Collaborator

0crat commented May 22, 2018

Order was finished, quality is "good": +20 point(s) just awarded to @paulodamaso/z

@0crat
Copy link
Collaborator

0crat commented May 22, 2018

Quality review completed: +8 point(s) just awarded to @elenavolokhova/z

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants