On Wed, 18 Feb 2026 18:25:37 GMT, Alan Bateman <[email protected]> wrote:
>> David Beaumont has updated the pull request with a new target base due to a
>> merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains five additional
>> commits since the last revision:
>>
>> - Merge branch 'lworld' into jdk_8377162_exploded_modules/squashed
>> - revert unnecessary filter in ModulePatcher
>> - Latest version
>> - Limit awareness of preview resources to system modules only.
>>
>> * limit preview handling
>> * Patch ModuleReaderTest from mainline
>> - Preview mode for exploded system module readers
>>
>> * with better flag plumbing
>> * Simpler approach with linked hash set
>> * Revert weird import change
>> * First cut at a preview aware module reader for exploded builds
>> * Test for partiy of JRT file-system and class resources in preview mode
>
> test/jdk/java/lang/module/ModuleReader/ModuleReaderTest.java line 289:
>
>> 287: @Test
>> 288: public void testModularJar() throws IOException {
>> 289: Path dir = Utils.createTempDirectory("mlib");
>
> Just curious why you changed this as this test should not need to the Utils.
I strongly prefer not having to reason about auxiliary details when reading
tests, especially if I'm not familiar with the tests/code involved. Test code
is the canonical representation of the expectations for the code under test,
and isn't itself tested. Thus, the fewer questions a test raises when reading
it, the easier it is to focus on what the test is testing, rather than how the
test is setting up its environment etc.
The original version of this test used a mix of "temporary directories created
in `user.dir`" and "relative paths from the current working directory". When
reading this I had to stop and ask "is there anything special about `user.dir`
rather than just using the current working directory?"
Whether a test reads the `user.dir` property, of uses '.' for the directory
inside which temporary files are created is a detail of how tests are done, and
the knowledge that these are (at the moment) functionally identical might not
be something everyone reading the tests knows. So the more it's obvious that
"this is just a normal temporary file and where it actually resides isn't
important", the less a reader of the code has to pause and spend mental energy
thinking about it.
Yes it's a small thing on its own, but consistency across tests for lots of
these small details adds up to less "mental friction".
Of course now I look again, I realise that this is a JUnit test, and could be
using `@TempDir` to be even more idiomatic.
-------------
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2032#discussion_r2827214962
