Increase abstractassert test coverage #2527
Conversation
|
Looks good. No mutations were possible for these changes. |
There was a problem hiding this comment.
thanks a lot @sarajuhosova for this great PR.
I have a few comments that are mainly stylistic and for simplifying the code a bit more.
| // THEN | ||
| List<Throwable> errorsCollected = softly.errorsCollected(); | ||
| assertThat(errorsCollected).hasSize(1); | ||
| assertThat(errorsCollected.get(0)).hasMessageStartingWith("IllegalArgumentException should have been thrown"); |
There was a problem hiding this comment.
You can simplify the assertion with singleElement()
There was a problem hiding this comment.
It would be nice to refactor the other tests in this file too ;-)
...test/java/org/assertj/core/api/inputstream/InputStreamAssert_asString_with_charset_Test.java
Outdated
Show resolved
Hide resolved
.../java/org/assertj/core/api/optional/OptionalAssert_contains_usingDefaultComparator_Test.java
Show resolved
Hide resolved
.../java/org/assertj/core/api/optional/OptionalAssert_contains_usingDefaultComparator_Test.java
Show resolved
Hide resolved
.../java/org/assertj/core/api/optional/OptionalAssert_contains_usingDefaultComparator_Test.java
Outdated
Show resolved
Hide resolved
|
Thanks, @joel-costigliola, for the feedback, I'll fix the style issues asap. I was also planning to add tests for the more complex classes that I've left out for now, should I add it to this PR or make a new one to make reviewing a bit easier? |
|
thanks for the quick reply, I would prefer another PR so that we can finish this one and it's easier to review shorter PRs. |
|
I've fixed the commented things, hopefully this is what you meant! |
|
Looks good. No mutations were possible for these changes. |
|
Integrated thanks @sarajuhosova! |
Check List:
Following the contributing guidelines will make it easier for us to review and accept your PR.