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

(#160) Improve messages in Assertion and Mismatches #231

Merged
merged 1 commit into from Feb 17, 2021

Conversation

victornoel
Copy link
Collaborator

This is for #160. I tried to cleanup a bit how Assertion and Mismatches behave and so that it's consistent with all the matchers.

@codecov
Copy link

codecov bot commented Feb 14, 2021

Codecov Report

Merging #231 (e3590a9) into master (e454e48) will increase coverage by 0.05%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #231      +/-   ##
============================================
+ Coverage     98.86%   98.91%   +0.05%     
  Complexity      147      147              
============================================
  Files            28       28              
  Lines           352      370      +18     
  Branches          7        7              
============================================
+ Hits            348      366      +18     
  Misses            4        4              
Impacted Files Coverage Δ Complexity Δ
...java/org/llorllale/cactoos/matchers/Assertion.java 100.00% <100.00%> (ø) 3.00 <0.00> (ø)
.../java/org/llorllale/cactoos/matchers/HasLines.java 100.00% <100.00%> (ø) 9.00 <2.00> (ø)
...java/org/llorllale/cactoos/matchers/HasValues.java 100.00% <100.00%> (ø) 5.00 <2.00> (ø)
.../llorllale/cactoos/matchers/HasValuesMatching.java 100.00% <100.00%> (ø) 4.00 <2.00> (ø)
...a/org/llorllale/cactoos/matchers/IsApplicable.java 100.00% <100.00%> (ø) 5.00 <2.00> (ø)
...n/java/org/llorllale/cactoos/matchers/IsBlank.java 100.00% <100.00%> (ø) 4.00 <1.00> (ø)
...in/java/org/llorllale/cactoos/matchers/IsTrue.java 100.00% <100.00%> (ø) 4.00 <1.00> (ø)
...ava/org/llorllale/cactoos/matchers/Mismatches.java 100.00% <100.00%> (ø) 6.00 <1.00> (ø)
.../org/llorllale/cactoos/matchers/RunsInThreads.java 87.50% <100.00%> (ø) 8.00 <0.00> (ø)
...va/org/llorllale/cactoos/matchers/TextMatcher.java 100.00% <100.00%> (ø) 9.00 <1.00> (ø)

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 e454e48...e3590a9. Read the comment docs.

@victornoel
Copy link
Collaborator Author

@0crat status

@0crat
Copy link
Collaborator

0crat commented Feb 15, 2021

@0crat status (here)

@victornoel This is what I know about this job in C63314D6Z, as in §32:

Copy link
Contributor

@baudoliver7 baudoliver7 left a comment

Choose a reason for hiding this comment

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

@victornoel Please, see comments below.

new Assertion<>(
"Must fail to mismatch",
new Mismatches<>(new TextOf("a"), new TextOf("expected")),
"Must mismatches",
Copy link
Contributor

Choose a reason for hiding this comment

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

@victornoel Must mismatch without es.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@baudoliver7 arg, again :) thx!

"describes the matcher",
description.toString(),
new IsEqual<>("\nExpected: Text with value \"e\"\n but was: Text with value \"e\"")
"Must mismatches",
Copy link
Contributor

Choose a reason for hiding this comment

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

@victornoel Must mismatch without es.

"no description when mismatches with right message",
description.toString(),
new IsEqual<>("")
"Must mismatches",
Copy link
Contributor

Choose a reason for hiding this comment

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

@victornoel Must mismatch without es.

@@ -65,7 +65,9 @@ public HasValues(final Iterable<X> expected) {
actual -> new ListOf<>(actual).containsAll(new ListOf<>(expected)),
desc -> desc.appendText("contains ")
.appendValue(new TextOf(expected)),
Copy link
Contributor

Choose a reason for hiding this comment

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

@victornoel Could you change .appendValue(new TextOf(expected)) to .appendValue(expected) as you did below ? I think that we do not need to use new TextOf(...) here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@baudoliver7 good find, and I looked around and didn't find any similar thing in the codebase too :)

@victornoel
Copy link
Collaborator Author

@baudoliver7 it should be ok now

@baudoliver7
Copy link
Contributor

@victornoel Thanks, it's ok for me :)

@victornoel
Copy link
Collaborator Author

@rultor merge

@rultor
Copy link
Collaborator

rultor commented Feb 17, 2021

@rultor merge

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

@rultor rultor merged commit e3590a9 into llorllale:master Feb 17, 2021
@rultor
Copy link
Collaborator

rultor commented Feb 17, 2021

@rultor merge

@victornoel Done! FYI, the full log is here (took me 8min)

@0crat 0crat added qa and removed 0crat/scope labels Feb 17, 2021
@0crat
Copy link
Collaborator

0crat commented Feb 17, 2021

@sereshqua/z please review this job completed by @baudoliver7/z, as in §30; the job will be fully closed and all payments will be made when the quality review is completed

@victornoel victornoel deleted the assertion-mismatches branch February 17, 2021 20:13
@sereshqua
Copy link

@0crat quality good

@0crat 0crat added quality/good and removed qa labels Feb 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants