On Thu, 17 Apr 2025 21:35:08 GMT, Justin Lu <[email protected]> wrote:
>> Please review this PR which improves the _ValidateISO4217_ Currency test by
>> adding testing of future currencies after the transition date.
>>
>> This is done by creating a patched version of Currency that replaces
>> `System.currentTimeMillis()` calls with a mocked value equivalent to
>> `Long.MAX_VALUE`. A module patch is then applied to supply the new Currency
>> class files.
>>
>> The mocked time behavior is tested by using the `currency.properties`
>> override in a separate invocation.
>
> Justin Lu has updated the pull request incrementally with one additional
> commit since the last revision:
>
> simple/special case in check invocation
Good to see the future currency testing.
test/jdk/java/util/Currency/ValidateISO4217.java line 157:
> 155: // "check" invocation only runs the main method (and not any tests)
> to determine if the
> 156: // future time checking is correct
> 157: public static void main(String[] args) {
It would probably helpful to check if the patched Currency class does exist or
not. Same for the `MOCKED.TIME=true` case.
test/jdk/java/util/Currency/ValidateISO4217.java line 203:
> 201: CodeTransform codeTransform = (codeBuilder, e) -> {
> 202: switch (e) {
> 203: case InvokeInstruction i when
> i.name().stringValue().equals("currentTimeMillis") ->
`equalsString()` may be used. Regardless, is there a way to tell this call is
indeed `System.currentTimeMillis()`? Might do that check in case a method on
Currency with the same name is introduced (not likely though).
-------------
PR Review: https://git.openjdk.org/jdk/pull/24701#pullrequestreview-2777025817
PR Review Comment: https://git.openjdk.org/jdk/pull/24701#discussion_r2049702515
PR Review Comment: https://git.openjdk.org/jdk/pull/24701#discussion_r2049704312
