On Fri, 6 Mar 2026 23:33:30 GMT, David Beaumont <[email protected]> wrote:
> 95% automated migration (search and replace).
> A handful of slightly note-worthy tweaks (called out with inline comments).
>
> The majority/automated changes are:
> * Convert to JUnit assertions (oddly, very few places)
> * Convert to JUnit data providers (almost all tests)
> * Convert test to use assertThrows (many tests)
> * Make util methods static (needed for data providers, but done everywhere
> for consistency)
> * Organise imports
test/jaxp/javax/xml/jaxp/functional/catalog/CatalogReferCircularityTest.java
line 53:
> 51:
> 52: @DataProvider(name = "catalogName")
> 53: public Object[][] catalogName() {
This was not pairwise data, so can just be `@ValueSource`.
test/jaxp/javax/xml/jaxp/functional/catalog/DefaultFeaturesTest.java line 46:
> 44: public void testDefaultFeatures(Feature feature, String expected) {
> 45: CatalogFeatures defaultFeature = CatalogFeatures.defaults();
> 46: assertEquals(expected, defaultFeature.get(feature));
assertNull(xx) and assertEquals(null, xx) are not worth the code complexity to
separate.
test/jaxp/javax/xml/jaxp/functional/catalog/DeferFeatureTest.java line 58:
> 56: public void testDeferFeature(Catalog catalog, int catalogCount)
> 57: throws Exception {
> 58: Assert.assertEquals(loadedCatalogCount(catalog), catalogCount);
Usual flipping of actual & expected.
test/jaxp/javax/xml/jaxp/functional/catalog/PreferFeatureTest.java line 53:
> 51: @MethodSource("data")
> 52: public void testPreferFeature(String prefer, String systemId, String
> publicId) {
> 53: CatalogResolver resolver = createResolver(prefer);
Better to pull out the resolver creation in all these tests because that's not
the code under test.
test/jaxp/javax/xml/jaxp/libs/catalog/CatalogTestUtils.java line 118:
> 116: // Gets the paths of the specified catalogs.
> 117: static URI getCatalogPath(String catalogName) {
> 118: return CATALOG_DIR.resolve(catalogName).toUri();
This needed to be changed to avoid calling into the utils class, which depends
on testng APIs which are no longer available for tests running via JUnit. Also,
the util method was very over-complicated for what was needed here.
test/jaxp/javax/xml/jaxp/libs/catalog/ResolutionChecker.java line 107:
> 105: * external identifier.
> 106: */
> 107: static <T extends Throwable> void expectExceptionOnExtId(
All these methods that do asserts, should themselves be called assertXxx.
I'm leaving alone for now, but it would improve maintainability if they were
made consistent.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/30124#discussion_r2898374836
PR Review Comment: https://git.openjdk.org/jdk/pull/30124#discussion_r2898377809
PR Review Comment: https://git.openjdk.org/jdk/pull/30124#discussion_r2898405249
PR Review Comment: https://git.openjdk.org/jdk/pull/30124#discussion_r2898414230
PR Review Comment: https://git.openjdk.org/jdk/pull/30124#discussion_r2898428983
PR Review Comment: https://git.openjdk.org/jdk/pull/30124#discussion_r2898424140
