RFR: 8068958: Timestamp.from(Instant) should throw when conversion is not possible [v2]

[Eamonn McManus]

On Fri, 22 Dec 2023 09:07:31 GMT, Raffaello Giulietti <rgiulietti at 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