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

Fix describeMismatchSafely in ScalarHasValue #14

Merged
merged 1 commit into from
Jun 1, 2018

Conversation

victornoel
Copy link
Collaborator

#7

This add tests for ScalarHasValue and implements properly describeMismatchSafely.
Note that I had to ignore a checkstyle error so that the ScalarHasValue class does not expose internal methods to the outside, see yegor256/qulice#901 for a rationale.

Also add a @todo for doing the same work on other matchers.

@0crat
Copy link
Collaborator

0crat commented May 19, 2018

Job #14 is now in scope, role is REV

@@ -40,6 +40,11 @@
* @author Yegor Bugayenko ([email protected])
* @version $Id$
* @since 0.11
* @todo #7:30min Tests for all matchers should be added and in particular
Copy link
Owner

Choose a reason for hiding this comment

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

@victornoel tests for all matchers is being handled starting in #1

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@llorllale yes, I noticed that the other day, but then I thought about leaving this todo because it concerns mainly describeMismatchSafely in the end.
I propose to reformulate it without the "tests for all matchers should be added", what do you think?

Copy link
Owner

Choose a reason for hiding this comment

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

@victornoel yes, please just leave out the part about "tests for all matchers should be added". The rest of the puzzle is fine

@codecov
Copy link

codecov bot commented May 28, 2018

Codecov Report

Merging #14 into master will increase coverage by 4%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master      #14   +/-   ##
=========================================
+ Coverage     77.55%   81.55%   +4%     
- Complexity       27       29    +2     
=========================================
  Files             7        7           
  Lines            98      103    +5     
  Branches          1        1           
=========================================
+ Hits             76       84    +8     
+ Misses           22       19    -3
Impacted Files Coverage Δ Complexity Δ
...rg/llorllale/cactoos/matchers/InputHasContent.java 75% <ø> (ø) 4 <0> (ø) ⬇️
...org/llorllale/cactoos/matchers/ScalarHasValue.java 100% <100%> (+30%) 5 <2> (+2) ⬆️

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 a620407...d3cd1e5. Read the comment docs.

@victornoel
Copy link
Collaborator Author

@llorllale I've rebased the branch on master since #15 was merged and I fixed the @todo, but codecov is not happy, I will take care of that soon.

@victornoel
Copy link
Collaborator Author

@llorllale everything is good now, thanks for waiting

@llorllale
Copy link
Owner

@rultor merge

@rultor
Copy link
Collaborator

rultor commented Jun 1, 2018

@rultor merge

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

@rultor rultor merged commit d3cd1e5 into llorllale:master Jun 1, 2018
@rultor
Copy link
Collaborator

rultor commented Jun 1, 2018

@rultor merge

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

@0crat
Copy link
Collaborator

0crat commented Jun 1, 2018

The job #14 is now out of scope

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.

5 participants