-
Notifications
You must be signed in to change notification settings - Fork 14
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
Added assertions for LocalDate #20
Conversation
* isAfter * isAfterOrEqualTo * isBefore * isBeforeOrEqualTo * isEqualTo * isIn * isNotEqualTo * isNotIn * hasYear * hasMonthOfYear * hasDayOfYear
@joel-costigliola, please review |
} | ||
|
||
@Test | ||
public void test_isAfterOrEqual_assertion_error_message() { |
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.
Since assertj-joda-time relies on assertj-core 3.x, we can take advantage to catchThrowable
, that will make the test easier to read and not having to use failBecauseExpectedAssertionErrorWasNotThrown
.
I'm keen on using a BBD style test (GIVEN, WHEN THEN).
This comment applies to any other test checking for an expected AssertionError
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.
Refactored to assertThatThrownBy() everywhere it is applicable
|
||
@Test | ||
public void should_fail_if_both_actual_and_parameter_are_null() { | ||
expectException(AssertionError.class, actualIsNull()); |
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.
no need to use catchThrowable
, the test is short enough to be readable (even though I'm not a big fan of expectException
coming as the first instruction making the test a THEN, GIVEN, WHEN instead of GIVEN, WHEN, THEN).
* Verifies that the month of the actual {@code LocalDate} is equal to the given month | ||
* <p> | ||
* Example : | ||
* <pre><code class='java'> assertThat(new LocalDate(2018,1,1)).hasDayOfMonth(1);</code></pre> |
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.
let's use a month != 1 to make it crystal clear that we check the day of month and not the month.
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.
Fixed
|
||
/** | ||
* Same assertion as {@link #isAfter(LocalDate)} but the {@link LocalDate} is built from given a String that | ||
* must follow ISO8601 format (yyyy-MM-dd)to allow calling {@link LocalDate#LocalDate(Object) LocalDate(Object)} |
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.
missing space (yyyy-MM-dd)to
-> (yyyy-MM-dd) to
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.
Fixed
@gd-estrepetov thanks for the PR, first round of review done! |
@joel-costigliola fixed all comments |
Integrated, many thanks @gd-estrepetov! |
I have added following assertions for LocalDate