ThrowsOnFalse #1692
ThrowsOnFalse #1692
Conversation
|
@yegor256 take a look, please |
|
@victornoel what do you think? |
There was a problem hiding this comment.
@h1alexbel thank you for your contribution, here are a few comments.
| * @param sclr Scalar | ||
| * @param msg Message | ||
| */ | ||
| public ThrowsOnFalse(final Scalar<Boolean> sclr, final String msg) { |
There was a problem hiding this comment.
@h1alexbel can you also add a constructor ThrowsOnFalse(final Scalar<Boolean> sclr, final Scalar<Exception> exception) to allow the user to choose which exception will be thrown
| void throwsOnFalse() { | ||
| final String msg = "test message"; | ||
| final String message = | ||
| Assertions.assertThrows( |
There was a problem hiding this comment.
@h1alexbel please use the Assertion class to write assertions, see other tests for some examples.
|
@victornoel fixed, check again, please |
| ) { | ||
| this( | ||
| sclr, () -> new IllegalArgumentException(message) | ||
| ); |
There was a problem hiding this comment.
@h1alexbel please put the constructor in this order in the file: first secondary constructor (the one taking a String) and last primary constructor (the one taking the two scalars).
| ); | ||
| @Test(expected = IllegalArgumentException.class) | ||
| public void throwsOnFalse() throws Exception { | ||
| new ThrowsOnFalse( |
There was a problem hiding this comment.
@h1alexbel please use org.llorllale.cactoos.matchers.Throws with Assertion to check for exception (instead of using expected = IllegalArgumentException.class)
| @Test(expected = IllegalArgumentException.class) | ||
| public void throwsOnFalse() throws Exception { | ||
| new ThrowsOnFalse( | ||
| () -> false, "test message" |
There was a problem hiding this comment.
@h1alexbel also check for the error message itself, not only that the exception is thrown
|
@victornoel fixed, take a look again, please |
|
@h1alexbel looks good, thank you :) |
|
@rultor merge |
@h1alexbel @victornoel Can't merge it. Some CI checks were failed. Apparently, the pull request is not ready to be merged since it has some problems. Please, fix them first. |
|
@yegor256 not sure why we have to codacy checks, but they make no sense with how we write tests, the error is:
How can we disable them? |
|
@victornoel disabled. Once something new is pushed to this branch, CI checks will be re-run (without codacy) |
|
@h1alexbel please could you push again to this branch, for example squash all commits as one, it would be perfect. |
|
@victornoel sure, squashed, new pr is here: #1696 |
|
Job |
closes #1175