Repository navigation
Run 387 more OpenJDK tests: jtreg variables, library paths, @clean, drivers and per-test locale data - #71
Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 59 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. |
![project516-review-bot[bot]](https://avatars.githubusercontent.com/in/4903871?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR extends the jtreg test runner to follow jtreg more closely: it implements test.src.path with @library directories, expands jtreg variables (${test.src}, ${test.classes}, etc.) in @run lines, adds @clean support, treats @run driver as main and @run build as @build, builds main classes from @library directories, compiles preview tests with -source 25 for --add-exports compatibility, adds more @requires properties, and runs 56 locale-sensitive tests with --locale-data. The CI workflow now fetches Unicode/LSR data and @library directories via --library-dirs. 387 additional tests now pass and are added to passing.txt.
review-bot, model nvidia/nemotron-3-ultra-550b-a55b:free, verdict approve
| } | ||
|
|
||
| // jtreg's ${...} in @run words (${test.main.class} is already in place) | ||
| function expand(tc: TestCase, word: string): string { |
There was a problem hiding this comment.
The expand function handles only test.src, test.classes, test.jdk, compile.jdk, test.root. jtreg also defines test.src.path, test.class.path, test.file, test.name; tests using these in @run arguments will not have them expanded. Since they are also passed as -D properties the test code can read them, but the command-line substitution is incomplete.
| const dirs = new Set<string>(); | ||
| for (const tc of selected) { | ||
| for (const lib of tc.libraries) { | ||
| if (lib.startsWith("/")) dirs.add(`test/jdk${lib}`).add(lib.slice(1)); |
There was a problem hiding this comment.
For an absolute @library path like /test/lib, this adds both test/jdk/test/lib (correct, relative to test/jdk root) and test/lib (incorrect, relative to repo root). The latter is already in the sparse checkout from the workflow's static list, so it only causes a redundant entry, but the logic is wrong.
| case "run": { | ||
| if (framework) break; | ||
| const m = value.match(/^(\S+)\s*(.*)$/); | ||
| const m = value.replaceAll("${test.main.class}", base).match(/^(\S+)\s*(.*)$/); |
There was a problem hiding this comment.
Only ${test.main.class} is replaced before parsing the @run line. If a test writes @run main ${test.src}/MyClass, the variable remains in the first word and the main class becomes the literal string ${test.src}/MyClass. The expand function runs later on a.args and a.opts but not on a.main (the main class extraction logic is not fully visible in the diff).
…, driver and build actions, sparse checkout of library directories, and --locale-data from a list
…with the list of tests that need locale data
d5588bf to
991b7e0
Compare
There was a problem hiding this comment.
The PR extends the jtreg test runner with several compatibility features: test.src.path includes @library directories, jtreg variable expansion in @run lines, @clean action support, @run driver/build handling, main classes from @library directories, preview tests with -source 25, additional @requires properties, --library-dirs for sparse checkout, and per-test locale data via locale-data.txt. This enables 387 more OpenJDK tests to pass. Three known gaps in jtreg variable expansion remain from earlier reviews.
Not approved: 3 earlier points are still open in the threads.
review-bot, model nvidia/nemotron-3-ultra-550b-a55b:free, verdict comment
| function expand(tc: TestCase, word: string): string { | ||
| const dest = classesDir(tc); | ||
| const values: Record<string, string> = { | ||
| "test.src": tc.dir, "test.classes": dest, "test.jdk": javaHome, "compile.jdk": javaHome, "test.root": testRoot, |
There was a problem hiding this comment.
The expand function handles only 5 of the jtreg variables. jtreg also defines test.src.path, test.class.path, test.file, and test.name; tests using these in @run arguments will not have them expanded. They are passed as -D properties (line 454-458) so test code can read them, but @run argument expansion is incomplete.
| const dirs = new Set<string>(); | ||
| for (const tc of selected) { | ||
| for (const lib of tc.libraries) { | ||
| if (lib.startsWith("/")) dirs.add(`test/jdk${lib}`).add(lib.slice(1)); |
There was a problem hiding this comment.
For an absolute @library path like /test/lib, this adds both test/jdk/test/lib (correct, relative to test/jdk root) and test/lib (incorrect, relative to repo root). The latter is already in the sparse checkout from the workflow's static list, so it only causes a redundant entry, but the logic is wrong. Use an if/else or two separate add calls with the correct paths.
What this changes
The OpenJDK test runner follows jtreg more closely, and 387 tests it now runs join
passing.txt.test.src.pathholds the test directory and its@librarydirectories, which tests use to find their data files.${test.main.class},${test.src},${test.classes},${test.jdk}and the other jtreg variables are replaced in@runlines.@cleanremoves classes between actions,@run driverruns asmain,@run buildcounts as@build, and a main class in an@librarydirectory is built.-source 25, so--add-exportsworks with them.@requiresproperties:os.name,os.version,os.simpleArch,jdk.static,java.enablePreview,java.runtime.name.--library-dirsprints the@librarydirectories of the selected tests. The jtreg workflow uses it for its sparse checkout, and also fetches the Unicode and LSR data files some tests read.test/jtreg/locale-data.txt(56) run with--locale-data. Turning it on for every test cost 15% more total time and timed out three date format tests that otherwise take 19 to 45 s.--locale-dataon the runner still turns it on for all tests, to find new ones that need it.Tests added
387, each passed twice: java/net 133, java/util 77, java/lang 57, java/io 44, java/text 27, java/nio 12, java/beans 12, java/time 9, java/security 9, javax/crypto 3, java/awt 3, javax/xml 2, java/math 2.
This PR's CI runs every test in
passing.txtwith the new runner.Noticed, not fixed:
locale-data.txt.java.base, 23 that compile into a named module, and the native, manual and jasm tests.