RFR: CODETOOLS-7903091: Convert jtreg to use NIO
Iris Clark
iris at openjdk.java.net
Tue Feb 8 07:31:18 UTC 2022
On Mon, 7 Feb 2022 22:49:29 GMT, Jonathan Gibbons <jjg at openjdk.org> wrote:
> Please review a medium-big, conceptually simple but occasionally tricky update to jtreg, to use `java.nio.file.Path` instead of `java.io.File` where reasonable.
>
> While most of the changes are formulaic and follow the obvious patterns, there were some noteworthy points:
>
> * The most complicated class to change was `Locations`, which does the most work to derive new paths. In particular, the semantics of `new File(a, b)` do not always match `Path.of(a).resolve(b)` for corner cases like when `a` is`null` or empty, or when `b` is absolute.
>
> * The NIO equivalent for some operations on `File` throw checked IO exceptions. These methods were wrapped to to throw a custom unchecked IO exception.
>
> * The NIO equivalent for creating a new file (`Path`) throws an unchecked `InvalidPathException` exception. The use sites are too numerous to handle individually. The solution is to catch exceptions like these (and the ones described in the previous bullet) at strategic points where they can be handled or wrapped back into (existing) checked exceptions.
>
> The development has been iterative, frequently running the jtreg internal tests locally (on a Mac) and occasionally building on a CI system (Linux). We should do more exhaustive testing with JDK before this work ever gets promoted.
I think this is a great first pass at modernizing jtreg! I only had a few minor comments.
Due to the nature of this change, I'd be reluctant to recommend cutting over to this version of jtreg without wide communication and extensive testing on the existing test suite. For testing, minimally, you should confirm that, for a given set of tests, you have the same number of pass/fail tests using both the old and new versions of the tool and the failures themselves are consistent (i.e. test failures occur because of the same problem in the system, not jtreg file/path handling). Recommend that this test be repeated across multiple platforms.
src/share/classes/com/sun/javatest/regtest/tool/OptionDecoder.java line 198:
> 196: // public void addFile(File file) throws BadArgs {
> 197: // fileOption.process(null, file.getPath());
> 198: // }
Retained intentionally or to-be-converted?
src/share/classes/com/sun/javatest/regtest/util/FileUtils.java line 1:
> 1: package com.sun.javatest.regtest.util;
No legal notices (copyright or license)?
src/share/classes/com/sun/javatest/regtest/util/FileUtils.java line 63:
> 61:
> 62: /**
> 63: * Returns the size of a file, in bytes.
Extra space before "bytes", also at line 66.
src/share/classes/com/sun/javatest/regtest/util/FileUtils.java line 99:
> 97: * @return {@code -1}, {@code 0} or {@code 1} according to whether the last modified time of the first file
> 98: * is less than, equal to, or greater than the last modified time of the second file
> 99: *
Not sure I understand the spacing convention, but the presence of this empty line and those at lines 109, 111, 113, and 128 may be inconsistent.
-------------
PR: https://git.openjdk.java.net/jtreg/pull/50
More information about the jtreg-dev
mailing list