On Tue, 10 Mar 2026 12:31:22 GMT, David Beaumont <[email protected]> wrote:
>> Refactor tests in test/jaxp/javax/xml/jaxp/functional/javax/xml to use
>> junit, along with ancillary utilities and a handful of related tests
>> elsewhere.
>>
>> The difficulty in these refactorings is the use of common utilities which
>> themselves depend on TestNG classes, which are not available when running
>> JUnit tests. Thus, several bits of functionality in utility classes (esp.
>> classes in `jaxp.library`) has had to be re-implemented and inlined. This
>> isn't terrible, since most of these were one line functions (or complex
>> functions which could be replaced with one line).
>>
>> Another complexity is accounting for the differences in test lifecycle
>> management between TestNG and JUnit. A few classes needed to exploit the
>> lifecycle and execution modes for single setup and single threaded operation.
>>
>> Finally, the move from "expected exceptions" on the whole test method to
>> using `assertThrows` threw up a lot of issues where tests weren't testing
>> what they claimed (the exception was not being thrown by the final statement
>> in the test). For these tests I've simplified them (rewriting them entirely
>> in some cases) but they now test what they claim to be testing and are much
>> clearer about where assertions are made.
>>
>> However, because these tests affect the thrown exceptions in test method
>> signatures, I ran a tidyup to remove all unused exceptions from method
>> signatures (rather than spending hours manually removing the effected
>> subset).
>
> David Beaumont has updated the pull request incrementally with one additional
> commit since the last revision:
>
> tidy up static imports for assertions
`grep -i testng -R test/jaxp/javax/xml/jaxp/functional/javax/xml` returns no
hits.
I suspect there might be more `assert*` statements whose arguments needing a
swap:
git grep -nE 'assert(Equals|NotEquals|Same|NotSame|ArrayEquals)\s*(' --
test/jaxp/javax/xml/jaxp/functional/javax/xml | \
while IFS=: read -r file line text; do \
committer=$(git blame -L"$line,$line" --porcelain "$file" | awk
'/^committer / {print $2}'); \
[ $committer != "David" ] && echo "$file:$line:^Cext"; \
done
test/jaxp/javax/xml/jaxp/functional/catalog/CatalogReferCircularityTest.java
line 1:
> 1: /*
Copyright year needs to be updated.
Since there are no semantic changes in this file, you can also consider
reverting your changes here.
test/jaxp/javax/xml/jaxp/functional/javax/xml/datatype/ptests/DurationTest.java
line 201:
> 199: */
> 200: @ParameterizedTest
> 201: @MethodSource("getEqualDurations")
Since you've already done similar changes, you can consider inlining this using
`@CsvSource`.
There are more places you can exercise `@CsvSource`, I won't mention them all.
test/jaxp/javax/xml/jaxp/functional/javax/xml/datatype/ptests/DurationTest.java
line 477:
> 475: int hash1 = duration1.hashCode();
> 476: int hash2 = duration2.hashCode();
> 477: assertEquals(hash1, hash2, " generated hash1 : " + hash1 + "
> generated hash2 : " + hash2);
Since you've switched from `assertTrue` to `assertEquals`, I doubt if the
message carries any extra value anymore.
test/jaxp/javax/xml/jaxp/functional/javax/xml/datatype/ptests/XMLGregorianCalendarTest.java
line 106:
> 104: // expected success
> 105:
> 106: assertEquals(calendar2.getMillisecond(), ms);
Needs a swap?
test/jaxp/javax/xml/jaxp/functional/javax/xml/parsers/ptests/SAXParserTest.java
line 153:
> 151: @MethodSource("getParser")
> 152: public void testParse09(SAXParser saxparser) {
> 153: assertThrows(IOException.class, () ->
> saxparser.parse("no-such-file", DEFAULT_HANDLER));
So `SAXException` in `expectedExceptions` was superfluous?
test/jaxp/javax/xml/jaxp/functional/javax/xml/transform/ptests/URIResolverTest.java
line 52:
> 50: * @test
> 51: * @library /javax/xml/jaxp/libs
> 52: * @run junit/othervm javax.xml.transform.ptests.URIResolverTest
Since you touch all `@run` lines anyway, you can also amending `@run ...
<class-name>` with `@run ... ${test.main.class}`.
-------------
PR Review: https://git.openjdk.org/jdk/pull/30165#pullrequestreview-3943851028
PR Review Comment: https://git.openjdk.org/jdk/pull/30165#discussion_r2931035812
PR Review Comment: https://git.openjdk.org/jdk/pull/30165#discussion_r2931060826
PR Review Comment: https://git.openjdk.org/jdk/pull/30165#discussion_r2931170351
PR Review Comment: https://git.openjdk.org/jdk/pull/30165#discussion_r2931271795
PR Review Comment: https://git.openjdk.org/jdk/pull/30165#discussion_r2931334048
PR Review Comment: https://git.openjdk.org/jdk/pull/30165#discussion_r2931427099
