On Tue, 2 Sep 2025 20:22:50 GMT, Naoto Sato <[email protected]> wrote:
>> Justin Lu has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - Nested class rename
>> - Add JDK regression test
>
> test/jdk/java/text/Format/DecimalFormat/DFSSerializationTest.java line 140:
>
>> 138: }
>> 139:
>> 140: // Previous versions of DFS could contain a null locale
>
> "Previous" suggests it would not allow null with this change, which is not
> the case.
Instances created from the current standard _public_ API **do not allow** a
null locale, hence the clarification that previous versions **can contain** a
null locale.
i.e. We do not expect to see null locales with up-to-date versions of DFS that
are not tampered with, so I think the comment is accurate.
> test/jdk/java/text/Format/DecimalFormat/DFSSerializationTest.java line 224:
>
>> 222: ObjectInputStream ois = new
>> ObjectInputStream(byteArrayInputStream)) {
>> 223: return (DecimalFormatSymbols) ois.readObject();
>> 224: }
>
> I guess if an error occured in this try block, the test should fail. Just
> wondering the reason it did not use `assertDoesNotThrow()` method.
I did not swallow the error with `assertDoesNotThrow()` here becasue
`disagreeingTextTest` expects to see an `InvalidObjectException.class`. Future
tests/changes can easily check for expected de-serialization failures with this
setup.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/27008#discussion_r2317106675
PR Review Comment: https://git.openjdk.org/jdk/pull/27008#discussion_r2317106647
