Copilot commented on code in PR #114: URL: https://github.com/apache/commons-secure-xml/pull/114#discussion_r4222492028
########## src/test/java/org/apache/commons/xml/osgi/OsgiIT.java: ########## @@ -0,0 +1,204 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.commons.xml.osgi; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotNull; + +import java.io.PrintWriter; +import java.io.StringWriter; +import java.net.URL; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; +import java.util.Enumeration; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.ServiceLoader; +import java.util.stream.Collectors; +import java.util.stream.Stream; + +import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.DynamicTest; +import org.junit.jupiter.api.TestFactory; +import org.junit.platform.engine.FilterResult; +import org.junit.platform.engine.discovery.DiscoverySelectors; +import org.junit.platform.engine.support.descriptor.MethodSource; +import org.junit.platform.launcher.PostDiscoveryFilter; +import org.junit.platform.launcher.TagFilter; +import org.junit.platform.launcher.core.LauncherDiscoveryRequestBuilder; +import org.junit.platform.launcher.core.LauncherFactory; +import org.junit.platform.launcher.listeners.SummaryGeneratingListener; +import org.junit.platform.launcher.listeners.TestExecutionSummary; +import org.osgi.framework.Bundle; +import org.osgi.framework.BundleContext; +import org.osgi.framework.Constants; +import org.osgi.framework.launch.Framework; +import org.osgi.framework.launch.FrameworkFactory; + +/** + * Runs the unit test suite inside an OSGi framework, against the built bundle. + * + * <p> + * The tests catch class-loading assumptions that only hold on a flat classpath: an OSGi bundle class loader sees its own classes, the packages it imports and + * {@code java.*}, but not JDK-internal packages such as {@code com.sun.*}. The built jar is installed with its generated manifest, so the bundle also proves + * that its {@code Import-Package} header resolves. + * </p> + * <p> + * The test output directory, which the build gives a fragment manifest, is attached to that bundle, so the tests share its class loader and keep their + * package-private access. The test harness (JUnit, Mockito, JAXB) is not under test and is boot-delegated to the class path. Only the JDK's built-in JAXP implementations are used, as in the + * {@code test-stockjdk} execution: provider lookups cannot see other bundles without a mediator such as SPI Fly. + * </p> + * <p> + * The framework is whichever single {@link FrameworkFactory} the class path provides; the build runs this class once with Apache Felix and once with Eclipse + * Equinox. + * </p> + */ +class OsgiIT { + + /** + * Package roots the framework delegates to the class path: the test harness only, never the library or JDK internals. + */ + private static final String[] HARNESS_PACKAGES = {"org.junit", "org.opentest4j", "org.apiguardian", "org.mockito", "net.bytebuddy", "org.objenesis", + "javax.xml.bind", "com.sun.xml.bind", "org.glassfish.jaxb", "com.sun.istack", "javax.activation", "jakarta.activation"}; + + /** + * Symbolic name of the library bundle. + */ + private static final String HOST_SYMBOLIC_NAME = "org.apache.commons.xml.secure"; + + /** + * Test classes that cannot run inside a framework: {@code ShadingFootprintTest} reads {@code target/classes} directly. + */ + private static final List<String> NOT_IN_OSGI = Collections.singletonList("org.apache.commons.xml.secure.ShadingFootprintTest"); + + /** + * Test methods that cannot run inside a framework. + * + * <p> + * They name a test class as the JAXP factory, which JAXP loads through the context class loader: that is the class path, which holds a second copy of the + * test classes. + * </p> + */ + private static final List<String> METHODS_NOT_IN_OSGI = Arrays.asList("SecureDocumentBuilderFactoryTest#newDocumentBuilderWrapsDeclaredExceptions", + "SecureSAXParserFactoryTest#newXmlReaderWrapsDeclaredExceptions"); + + /** + * Location of the test classes inside the fragment. + */ + private static final String TEST_PACKAGE = HOST_SYMBOLIC_NAME.replace('.', '/'); + + private static Framework framework; + + private static Bundle host; + + private static Path requiredPath(final String property) { + final String value = System.getProperty(property); + assertNotNull(value, "System property '" + property + "' must be set by the build"); + return Paths.get(value).toAbsolutePath(); + } + + @BeforeAll + static void startFramework() throws Exception { + final List<FrameworkFactory> factories = new ArrayList<>(); + ServiceLoader.load(FrameworkFactory.class).forEach(factories::add); + assertEquals(1, factories.size(), () -> "Expected exactly one OSGi framework on the class path, found " + factories); + final Path buildJar = requiredPath("buildJar"); + final Path work = requiredPath("osgiWorkDirectory"); + Files.createDirectories(work); + final Map<String, String> config = new HashMap<>(); + config.put(Constants.FRAMEWORK_STORAGE, work.resolve("storage").toString()); + config.put(Constants.FRAMEWORK_STORAGE_CLEAN, Constants.FRAMEWORK_STORAGE_CLEAN_ONFIRSTINIT); + config.put(Constants.FRAMEWORK_BUNDLE_PARENT, Constants.FRAMEWORK_BUNDLE_PARENT_FRAMEWORK); + // A trailing ".*" only matches subpackages, so each root is listed twice. + // sun.reflect, jdk.internal.reflect: the JDK defines generated reflection accessors under the bundle loader. + // leaked: XSLTC rejects the host of Felix bundle:// URLs (for example "5.0"), so the XML fixtures resolve as file: URLs instead. + config.put(Constants.FRAMEWORK_BOOTDELEGATION, Stream.concat(Stream.of(HARNESS_PACKAGES).flatMap(p -> Stream.of(p, p + ".*")), + Stream.of("sun.reflect", "jdk.internal.reflect", "leaked")).collect(Collectors.joining(","))); + // Both frameworks can fall back to the parent loader on their own; disable it so only the explicit list above is delegated. + config.put("felix.bootdelegation.implicit", "false"); + config.put("osgi.compatibility.bootdelegation", "false"); + config.put("osgi.context.bootdelegation", "false"); + framework = factories.get(0).newFramework(config); + framework.start(); + final BundleContext context = framework.getBundleContext(); + host = context.installBundle(buildJar.toUri().toString()); + // The test output directory carries the fragment manifest written by maven-bundle-plugin; install it in place. + context.installBundle("reference:" + requiredPath("testClassesDirectory").toUri()); + host.start(); + assertEquals(Bundle.ACTIVE, host.getState(), "library bundle must start"); + } + + @AfterAll + static void stopFramework() throws Exception { + if (framework != null) { + framework.stop(); + framework.waitForStop(10_000); + } + } Review Comment: `framework.waitForStop(10_000)` returns a `FrameworkEvent` that can indicate a timeout or error; ignoring it can leave the framework running (and threads/resources leaked), which is especially problematic in Failsafe where the same fork may continue executing. Consider checking the returned event type and failing the test on timeout/error (and/or increasing configurability of the timeout). ########## src/main/java/org/apache/commons/xml/secure/SecureXMLInputFactory.java: ########## @@ -285,7 +285,8 @@ public static XMLInputFactory newDefaultFactory() { } try { // Java 8: the method does not exist, and XMLInputFactory has no class-name-taking lookup; instantiate the JDK's built-in default directly. - return secure((XMLInputFactory) Class.forName(JDK_XML_INPUT_FACTORY).getConstructor().newInstance()); + // Load it from the bootstrap loader: our own loader may not see com.sun.* (for example, an OSGi bundle class loader). + return secure((XMLInputFactory) Class.forName(JDK_XML_INPUT_FACTORY, true, null).getConstructor().newInstance()); } catch (final ReflectiveOperationException e) { Review Comment: This reflective path can also throw `SecurityException` (e.g., under a `SecurityManager` on Java 8) which is not a `ReflectiveOperationException` and will currently escape as an unchecked exception. If the intended contract is to surface provider/instantiation problems as `FactoryConfigurationError` (as in the catch below), consider also catching `SecurityException` here and wrapping it similarly so callers get a consistent failure mode. ########## src/test/java/org/apache/commons/xml/osgi/OsgiIT.java: ########## @@ -0,0 +1,204 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.commons.xml.osgi; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotNull; + +import java.io.PrintWriter; +import java.io.StringWriter; +import java.net.URL; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; +import java.util.Enumeration; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.ServiceLoader; +import java.util.stream.Collectors; +import java.util.stream.Stream; + +import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.DynamicTest; +import org.junit.jupiter.api.TestFactory; +import org.junit.platform.engine.FilterResult; +import org.junit.platform.engine.discovery.DiscoverySelectors; +import org.junit.platform.engine.support.descriptor.MethodSource; +import org.junit.platform.launcher.PostDiscoveryFilter; +import org.junit.platform.launcher.TagFilter; +import org.junit.platform.launcher.core.LauncherDiscoveryRequestBuilder; +import org.junit.platform.launcher.core.LauncherFactory; +import org.junit.platform.launcher.listeners.SummaryGeneratingListener; +import org.junit.platform.launcher.listeners.TestExecutionSummary; +import org.osgi.framework.Bundle; +import org.osgi.framework.BundleContext; +import org.osgi.framework.Constants; +import org.osgi.framework.launch.Framework; +import org.osgi.framework.launch.FrameworkFactory; + +/** + * Runs the unit test suite inside an OSGi framework, against the built bundle. + * + * <p> + * The tests catch class-loading assumptions that only hold on a flat classpath: an OSGi bundle class loader sees its own classes, the packages it imports and + * {@code java.*}, but not JDK-internal packages such as {@code com.sun.*}. The built jar is installed with its generated manifest, so the bundle also proves + * that its {@code Import-Package} header resolves. + * </p> + * <p> + * The test output directory, which the build gives a fragment manifest, is attached to that bundle, so the tests share its class loader and keep their + * package-private access. The test harness (JUnit, Mockito, JAXB) is not under test and is boot-delegated to the class path. Only the JDK's built-in JAXP implementations are used, as in the + * {@code test-stockjdk} execution: provider lookups cannot see other bundles without a mediator such as SPI Fly. + * </p> + * <p> + * The framework is whichever single {@link FrameworkFactory} the class path provides; the build runs this class once with Apache Felix and once with Eclipse + * Equinox. + * </p> + */ +class OsgiIT { + + /** + * Package roots the framework delegates to the class path: the test harness only, never the library or JDK internals. + */ + private static final String[] HARNESS_PACKAGES = {"org.junit", "org.opentest4j", "org.apiguardian", "org.mockito", "net.bytebuddy", "org.objenesis", + "javax.xml.bind", "com.sun.xml.bind", "org.glassfish.jaxb", "com.sun.istack", "javax.activation", "jakarta.activation"}; + + /** + * Symbolic name of the library bundle. + */ + private static final String HOST_SYMBOLIC_NAME = "org.apache.commons.xml.secure"; + + /** + * Test classes that cannot run inside a framework: {@code ShadingFootprintTest} reads {@code target/classes} directly. + */ + private static final List<String> NOT_IN_OSGI = Collections.singletonList("org.apache.commons.xml.secure.ShadingFootprintTest"); + + /** + * Test methods that cannot run inside a framework. + * + * <p> + * They name a test class as the JAXP factory, which JAXP loads through the context class loader: that is the class path, which holds a second copy of the + * test classes. + * </p> + */ + private static final List<String> METHODS_NOT_IN_OSGI = Arrays.asList("SecureDocumentBuilderFactoryTest#newDocumentBuilderWrapsDeclaredExceptions", + "SecureSAXParserFactoryTest#newXmlReaderWrapsDeclaredExceptions"); + + /** + * Location of the test classes inside the fragment. + */ + private static final String TEST_PACKAGE = HOST_SYMBOLIC_NAME.replace('.', '/'); + + private static Framework framework; + + private static Bundle host; + + private static Path requiredPath(final String property) { + final String value = System.getProperty(property); + assertNotNull(value, "System property '" + property + "' must be set by the build"); + return Paths.get(value).toAbsolutePath(); + } + + @BeforeAll + static void startFramework() throws Exception { + final List<FrameworkFactory> factories = new ArrayList<>(); + ServiceLoader.load(FrameworkFactory.class).forEach(factories::add); + assertEquals(1, factories.size(), () -> "Expected exactly one OSGi framework on the class path, found " + factories); + final Path buildJar = requiredPath("buildJar"); + final Path work = requiredPath("osgiWorkDirectory"); + Files.createDirectories(work); + final Map<String, String> config = new HashMap<>(); + config.put(Constants.FRAMEWORK_STORAGE, work.resolve("storage").toString()); + config.put(Constants.FRAMEWORK_STORAGE_CLEAN, Constants.FRAMEWORK_STORAGE_CLEAN_ONFIRSTINIT); + config.put(Constants.FRAMEWORK_BUNDLE_PARENT, Constants.FRAMEWORK_BUNDLE_PARENT_FRAMEWORK); + // A trailing ".*" only matches subpackages, so each root is listed twice. + // sun.reflect, jdk.internal.reflect: the JDK defines generated reflection accessors under the bundle loader. + // leaked: XSLTC rejects the host of Felix bundle:// URLs (for example "5.0"), so the XML fixtures resolve as file: URLs instead. + config.put(Constants.FRAMEWORK_BOOTDELEGATION, Stream.concat(Stream.of(HARNESS_PACKAGES).flatMap(p -> Stream.of(p, p + ".*")), + Stream.of("sun.reflect", "jdk.internal.reflect", "leaked")).collect(Collectors.joining(","))); + // Both frameworks can fall back to the parent loader on their own; disable it so only the explicit list above is delegated. + config.put("felix.bootdelegation.implicit", "false"); + config.put("osgi.compatibility.bootdelegation", "false"); + config.put("osgi.context.bootdelegation", "false"); + framework = factories.get(0).newFramework(config); + framework.start(); + final BundleContext context = framework.getBundleContext(); + host = context.installBundle(buildJar.toUri().toString()); + // The test output directory carries the fragment manifest written by maven-bundle-plugin; install it in place. + context.installBundle("reference:" + requiredPath("testClassesDirectory").toUri()); + host.start(); + assertEquals(Bundle.ACTIVE, host.getState(), "library bundle must start"); + } + + @AfterAll + static void stopFramework() throws Exception { + if (framework != null) { + framework.stop(); + framework.waitForStop(10_000); + } + } + + /** + * Runs one test class, loaded through the library bundle, and fails with the inner failures if any. + */ + private static void runTestClass(final Class<?> testClass) { + final SummaryGeneratingListener listener = new SummaryGeneratingListener(); + final PostDiscoveryFilter notInOsgi = descriptor -> descriptor.getSource() + .filter(MethodSource.class::isInstance) + .map(MethodSource.class::cast) + .filter(source -> METHODS_NOT_IN_OSGI.contains(source.getJavaClass().getSimpleName() + "#" + source.getMethodName())) + .map(source -> FilterResult.excluded("cannot run inside an OSGi framework")) + .orElseGet(() -> FilterResult.included(null)); + // The context class loader stays the class path's, as in a plain framework launcher. + LauncherFactory.create().execute(LauncherDiscoveryRequestBuilder.request() + .selectors(DiscoverySelectors.selectClass(testClass)) + // Same exclusion as the test-stockjdk execution: xpath3 needs Saxon. + .filters(TagFilter.excludeTags("xpath3"), notInOsgi) + .build(), listener); + final TestExecutionSummary summary = listener.getSummary(); + if (summary.getTotalFailureCount() > 0) { + final StringWriter failures = new StringWriter(); + summary.printFailuresTo(new PrintWriter(failures), 25); + throw new AssertionError(summary.getTotalFailureCount() + " failure(s) in " + testClass.getName() + " under " + framework.getSymbolicName() + + System.lineSeparator() + failures); + } + } + + @TestFactory + Stream<DynamicTest> unitTestsPassInsideFramework() throws ClassNotFoundException { + final List<DynamicTest> tests = new ArrayList<>(); + final Enumeration<URL> entries = host.findEntries(TEST_PACKAGE, "*Test.class", false); + assertNotNull(entries, "test fragment must be attached to the library bundle"); + while (entries.hasMoreElements()) { + final String path = entries.nextElement().getPath(); + final String className = path.substring(path.indexOf(TEST_PACKAGE), path.length() - ".class".length()).replace('/', '.'); + if (!NOT_IN_OSGI.contains(className)) { Review Comment: `host.findEntries(..., false)` only scans the immediate `TEST_PACKAGE` directory. Any tests located in subpackages will be silently skipped, so this IT won’t actually run the full unit test suite as intended. Consider using a recursive scan (`recurse = true`) or switching discovery to package-based selection so subpackages are included. Also, `assertNotNull(entries, \"test fragment must be attached...\")` is misleading because `findEntries` can return null when there are simply no matches; consider emitting an error that distinguishes 'fragment not attached' from 'no *Test.class found'. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
