RFR: 8378118: Test tools/jimage/JImageBadFileTest.java failed on Windows
Removed the manual file tidyup on the expectation that jtreg will sort that all out. Switched to using Utils.createTempFile() while I was there. ------------- Commit messages: - Remove file cleanup and switch to Utils.createTempFile() Changes: https://git.openjdk.org/jdk/pull/29783/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=29783&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8378118 Stats: 46 lines in 1 file changed: 2 ins; 23 del; 21 mod Patch: https://git.openjdk.org/jdk/pull/29783.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/29783/head:pull/29783 PR: https://git.openjdk.org/jdk/pull/29783
On Wed, 18 Feb 2026 10:42:01 GMT, David Beaumont <duke@openjdk.org> wrote:
Removed the manual file tidyup on the expectation that jtreg will sort that all out. Switched to using Utils.createTempFile() while I was there.
test/jdk/tools/jimage/JImageBadFileTest.java line 82:
80: throws IOException { 81: int remaining = maxLen >= 0 ? maxLen : Integer.MAX_VALUE; 82: Path dst = Utils.createTempFile("modules-" + label, "");
Just curious why this is changed. test/jdk/tools/jimage/JImageBadFileTest.java line 100:
98: } catch (IOException e) { 99: Files.deleteIfExists(dst); 100: throw e;
Good, this allows it to be saved in the event of a test failure too. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/29783#discussion_r2826103532 PR Review Comment: https://git.openjdk.org/jdk/pull/29783#discussion_r2826106775
On Thu, 19 Feb 2026 07:33:16 GMT, Alan Bateman <alanb@openjdk.org> wrote:
Removed the manual file tidyup on the expectation that jtreg will sort that all out. Switched to using Utils.createTempFile() while I was there.
test/jdk/tools/jimage/JImageBadFileTest.java line 82:
80: throws IOException { 81: int remaining = maxLen >= 0 ? maxLen : Integer.MAX_VALUE; 82: Path dst = Utils.createTempFile("modules-" + label, "");
Just curious why this is changed.
It's just the more idiomatic way to make temp files in tests, since it hides any semantics related to knowing that the current working directory is the correct place for temporary files. Fewer details in the test code itself. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/29783#discussion_r2827045289
On Wed, 18 Feb 2026 10:42:01 GMT, David Beaumont <duke@openjdk.org> wrote:
Removed the manual file tidyup on the expectation that jtreg will sort that all out. Switched to using Utils.createTempFile() while I was there.
Good to clean up the CI. test/jdk/tools/jimage/JImageBadFileTest.java line 145:
143: // Self test that the file copying isn't itself corrupting anything. 144: Path tempJimage = writeModifiedJimage("good_file", -1, b -> {}); 145: jimage("info", tempJimage.toString()).assertSuccess();
I know its a bit late to complain now, but "jimage" is not a very nor a very descriptive name of a central utility method. ------------- Marked as reviewed by rriggs (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/29783#pullrequestreview-3841592785 PR Review Comment: https://git.openjdk.org/jdk/pull/29783#discussion_r2841339788
On Mon, 23 Feb 2026 15:00:11 GMT, Roger Riggs <rriggs@openjdk.org> wrote:
Removed the manual file tidyup on the expectation that jtreg will sort that all out. Switched to using Utils.createTempFile() while I was there.
test/jdk/tools/jimage/JImageBadFileTest.java line 145:
143: // Self test that the file copying isn't itself corrupting anything. 144: Path tempJimage = writeModifiedJimage("good_file", -1, b -> {}); 145: jimage("info", tempJimage.toString()).assertSuccess();
I know its a bit late to complain now, but "jimage" is not a very nor a very descriptive name of a central utility method.
It's a method on JImageCliTest, the super class of the tests for the jimage tool. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/29783#discussion_r2841476475
On Mon, 23 Feb 2026 15:25:21 GMT, Alan Bateman <alanb@openjdk.org> wrote:
test/jdk/tools/jimage/JImageBadFileTest.java line 145:
143: // Self test that the file copying isn't itself corrupting anything. 144: Path tempJimage = writeModifiedJimage("good_file", -1, b -> {}); 145: jimage("info", tempJimage.toString()).assertSuccess();
I know its a bit late to complain now, but "jimage" is not a very nor a very descriptive name of a central utility method.
It's a method on JImageCliTest, the super class of the tests for the jimage tool.
Yep, I tracked it down, its still not a good name. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/29783#discussion_r2841877698
On Mon, 23 Feb 2026 16:41:54 GMT, Roger Riggs <rriggs@openjdk.org> wrote:
It's a method on JImageCliTest, the super class of the tests for the jimage tool.
Yep, I tracked it down, its still not a good name.
I'm happy to refactor it in a followup PR if you want? Just say the word. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/29783#discussion_r2841884715
On Mon, 23 Feb 2026 16:43:21 GMT, David Beaumont <duke@openjdk.org> wrote:
Yep, I tracked it down, its still not a good name.
I'm happy to refactor it in a followup PR if you want? Just say the word.
Leave it for now, its been stable since 2016. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/29783#discussion_r2842187445
On Wed, 18 Feb 2026 10:42:01 GMT, David Beaumont <duke@openjdk.org> wrote:
Removed the manual file tidyup on the expectation that jtreg will sort that all out. Switched to using Utils.createTempFile() while I was there.
Marked as reviewed by alanb (Reviewer). ------------- PR Review: https://git.openjdk.org/jdk/pull/29783#pullrequestreview-3841971250
On Wed, 18 Feb 2026 10:42:01 GMT, David Beaumont <duke@openjdk.org> wrote:
Removed the manual file tidyup on the expectation that jtreg will sort that all out. Switched to using Utils.createTempFile() while I was there.
@david-beaumont Your change (at version be66cf1d81b63bd427ba7eb23126d1b87eb530f0) is now ready to be sponsored by a Committer. ------------- PR Comment: https://git.openjdk.org/jdk/pull/29783#issuecomment-3945925131
On Wed, 18 Feb 2026 10:42:01 GMT, David Beaumont <duke@openjdk.org> wrote:
Removed the manual file tidyup on the expectation that jtreg will sort that all out. Switched to using Utils.createTempFile() while I was there.
This pull request has now been integrated. Changeset: 75f8f08f Author: David Beaumont <david.beaumont@oracle.com> Committer: David Holmes <dholmes@openjdk.org> URL: https://git.openjdk.org/jdk/commit/75f8f08f3b6b4e3b211553ed78862ab03ca5d216 Stats: 46 lines in 1 file changed: 2 ins; 23 del; 21 mod 8378118: Test tools/jimage/JImageBadFileTest.java failed on Windows Reviewed-by: rriggs, alanb ------------- PR: https://git.openjdk.org/jdk/pull/29783
participants (4)
-
Alan Bateman
-
David Beaumont
-
duke
-
Roger Riggs