On Fri, 22 Dec 2023 09:07:31 GMT, Raffaello Giulietti <rgiulie...@openjdk.org>
wrote:
>> Eamonn McManus has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Address review comments about the new test.
>
> test/jdk/java/sql/testng/test/sql/TimestampTests.java line 649:
>
>> 647: // The latest Instant that can be converted to a Timestamp.
>> 648: Instant instant1 = Instant.ofEpochSecond(Long.MAX_VALUE / 1000,
>> 999_999_999);
>> 649: assertEquals(instant1, Timestamp.from(instant1).toInstant());
>
> The 1st arg to `assertEquals()` should be the actual value, the 2nd should be
> the expected result. I guess you expect `instant1` to be the expected result.
> (TestNG and JUnit have opposite conventions...)
Thanks, I hadn't realized that.
> test/jdk/java/sql/testng/test/sql/TimestampTests.java line 658:
>
>> 656: } catch (IllegalArgumentException expected) {
>> 657: }
>> 658:
>
> TestNG supports a better way for expected exceptions, as an attribute of the
> @Test annotation. Maybe it can be used here for the expected failing cases?
I'm not very familiar with TestNG, as you'll have noticed. But I see it has a
better way yet, namely `expectThrows`, like JUnit's `assertThrows`. So I've
updated the code to use that. Thanks for the hint!
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/17181#discussion_r1435375742
PR Review Comment: https://git.openjdk.org/jdk/pull/17181#discussion_r1435376042
