-
Notifications
You must be signed in to change notification settings - Fork 195
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
AbstractHmBody doesn't allow to use different matchers for text bodies #813
Conversation
Job #813 is now in scope, role is |
Codecov Report
@@ Coverage Diff @@
## master #813 +/- ##
============================================
+ Coverage 76.54% 76.87% +0.32%
- Complexity 956 971 +15
============================================
Files 215 219 +4
Lines 4567 4610 +43
Branches 366 370 +4
============================================
+ Hits 3496 3544 +48
+ Misses 918 910 -8
- Partials 153 156 +3
Continue to review full report at Codecov.
|
@g4s8 ping |
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.
@t-izbassar looks good, just few comments
* @param expected String to test against | ||
*/ | ||
public HmRqTextBody(final String expected) { | ||
this(Matchers.containsString(expected)); |
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.
@t-izbassar I think it's not clear, because default matcher in Hamcrest library usually is Matchers.equalTo()
, so as a user of HmRqTextBody
I'd expect that body is equal to constructor parameter string
|
||
@Override | ||
public final void describeTo(final Description description) { | ||
description.appendDescriptionOf(this.body); |
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.
@t-izbassar it will be not really informative, it should include not only expected result, but also actual value and some text description. Please see how it is implemented in AbstractHmHeader
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.
@g4s8 updated puzzle description to cover that issue.
public void testsBodyValueContainsText() { | ||
MatcherAssert.assertThat( | ||
new RsWithBody("<html><h1>Hello</h1></html>"), | ||
new HmRsTextBody("<h1>Hello</h1>") |
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.
@t-izbassar this test is not so obvious, please look at previous comment about default matcher in constructor
@g4s8 done. |
@yegor256 I accept |
@yegor256 can you merge this one? |
@rultor merge |
@elenavolokhova/z please review this job, as in §30; the job will be fully closed and all payments will be made when the quality review is completed |
@0crat quality good |
@elenavolokhova You can't do that, unless you have one of these roles: |
|
@elenavolokhova The job #813 is now out of scope |
Fix for #794.
I've added
AbstractHmTextBody
matcher, that converts body contents toString
and use that to match the contents with provided matcher. Added text body matchers forRequest
andResponse
with relevant test cases.However, there still not covered descriptions of the fails and
describeMismatchSafely
is not implemented. Left puzzle to address this issue.This PR partially solves original issue. Left puzzle to address what was left (converting
AbstractHmBody
toAbstractHmBytesBody
) as those are beyond 30 mins mark.