-
-
Notifications
You must be signed in to change notification settings - Fork 55
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
#42: Added 'Condition' and wrote a few tests for AssertRequest #48
Conversation
* Added new 'Condition' class to support our tests * AssertRequest now accepts Condition instead of Predicate<HttpRequest>
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.
@llorllale Looks good, just a few comments
*/ | ||
public Predicate<HttpRequest> test() { | ||
return this.test; | ||
} |
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.
@llorllale This method could be changed a little, like this:
public void test(final HttpRequest request) {
if(!this.predicate.test(request)){
Assert.fail(this.msg);
};
}
Then you could also get rid of the msg()
accessor and also refactor method check
from AssertRequest, it could simply be:
private void check(final HttpRequest request) {
this.conditions.forEach(cond -> {cond.test(request)});
}
I know you said you didn't want a lot of design into this support class, but it's too good not to do it :D
You can also leave a puzzle for it and I'll do it later, if you spent all the time already.
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.
@amihaiemil done
/** | ||
* The test that the http request must satisfy. | ||
*/ | ||
private final Predicate<HttpRequest> test; |
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.
@llorllale Can you rename this to predicate
or something else? Too much test
around :D
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.
@amihaiemil ok
/** | ||
* Tests for {@link AssertRequest}. | ||
* | ||
* @author George Aristy (george.aristy@gmail.com) |
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.
@llorllale Don't forget the @since 0.0.1
, and @version $Id$
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.
@amihaiemil ok
As per PR review: * Inversion of control: the condition class itself now performs the actual test
As per PR review: * Added missing javadoc tags * Renamed an instance variable to avoid name clashing
@amihaiemil please check now |
@llorllale looks nice, thanks! |
@rultor merge it |
@amihaiemil OK, I'll try to merge now. You can check the progress of the merge here |
@amihaiemil Done! FYI, the full log is here (took me 2min) |
@amihaiemil/z please, pay attention to this pull request |
Job #48 is now in scope, role is |
The job #48 is now out of scope |
This PR:
Condition
class to support our testsAssertRequest
now acceptsCondition
instead ofPredicate<HttpRequest>
AssertRequest