Re: [PR] Enable SurrogatePairLengthTest [xerces-j]

Copilot commented on code in PR #116:
URL: https://github.com/apache/xerces-j/pull/116#discussion_r3610584336


##########
tests/schema/config/SurrogatePairLengthTest.java:
##########
@@ -27,56 +28,63 @@
  */
 public class SurrogatePairLengthTest extends BaseTest {
 
-    // Can only test when the property is set
-    static {
+    private Field codePointCountField;
+    private boolean originalCodePointCount;
+
+    protected void setUp() throws Exception {
+        super.setUp();
         System.setProperty("org.apache.xerces.impl.dv.xs.useCodePointCountForStringLength", "true");
+        Field f = TypeValidator.class.getDeclaredField("USE_CODE_POINT_COUNT_FOR_STRING_LENGTH");
+        f.setAccessible(true);
+        codePointCountField = f;
+        originalCodePointCount = f.getBoolean(null);
+        f.set(null, true);
+    }
+
+    protected void tearDown() throws Exception {
+        System.clearProperty("org.apache.xerces.impl.dv.xs.useCodePointCountForStringLength");
+        if (codePointCountField != null) {
+            codePointCountField.set(null, originalCodePointCount);
+        }
+        super.tearDown();
     }

Review Comment:
   setUp/tearDown mutate a global system property and a static field but do not restore the previous property value (only clears it), and if an exception occurs part-way through setUp the property can leak into subsequent tests. Capture and restore the original property value, and ensure tearDown always calls super.tearDown() via finally.



##########
src/org/apache/xerces/impl/dv/xs/TypeValidator.java:
##########
@@ -39,7 +39,7 @@
  */
 public abstract class TypeValidator {
     
-    private static final boolean USE_CODE_POINT_COUNT_FOR_STRING_LENGTH = AccessController.doPrivileged(new PrivilegedAction() {
+    private static boolean USE_CODE_POINT_COUNT_FOR_STRING_LENGTH = AccessController.doPrivileged(new PrivilegedAction() {

Review Comment:
   USE_CODE_POINT_COUNT_FOR_STRING_LENGTH is now mutable (no longer final) and is being modified reflectively by tests. If it can change at runtime, it should be volatile to guarantee visibility across threads and prevent the JIT from treating it as effectively constant.



##########
build.xml:
##########
@@ -835,25 +835,27 @@ Authors:
              <include name="schema/config/IgnoreXSIType_A_C_Test.class"/>
              <include name="schema/config/IgnoreXSIType_C_A_Test.class"/>
              <include name="schema/config/RootSimpleTypeDefinitionTest.class"/>
-             <include name="schema/config/RootTypeDefinitionTest.class"/>
-             <include name="schema/config/UseGrammarPoolOnly_False_Test.class"/>
-          <!-- These tests are failing. Fix them.
-             <include name="schema/config/IgnoreXSIType_C_AC_Test.class"/>
-             <include name="schema/config/IgnoreXSIType_C_CA_Test.class"/>
-             <include name="schema/config/IgnoreXSIType_C_C_Test.class"/>
-             <include name="schema/config/SurrogatePairLengthTest.class"/>
-             <include name="schema/config/UseGrammarPoolOnly_True_Test.class"/>
-             <include name="schema/config/UnparsedEntityCheckingTest.class"/>
-            -->             
+              <include name="schema/config/RootTypeDefinitionTest.class"/>
+              <include name="schema/config/UseGrammarPoolOnly_False_Test.class"/>
+           <!-- These tests are failing. Fix them.
+              <include name="schema/config/IgnoreXSIType_C_AC_Test.class"/>
+              <include name="schema/config/IgnoreXSIType_C_CA_Test.class"/>
+              <include name="schema/config/IgnoreXSIType_C_C_Test.class"/>
+              <include name="schema/config/UseGrammarPoolOnly_True_Test.class"/>
+              <include name="schema/config/UnparsedEntityCheckingTest.class"/>
+             -->
+              <!-- SurrogatePairLengthTest uses reflection to modify a private
+                   static field in TypeValidator. This test is NOT safe for
+                   parallel execution and must run sequentially.                -->
+              <include name="schema/config/SurrogatePairLengthTest.class"/>

Review Comment:
   The PR description says SurrogatePairLengthTest is run in its own JVM via a separate <junit> task (forkmode="perTest") before the main batchtest so the system property is read during TypeValidator class initialization. In build.xml, the test is still included in the existing shared-JVM junit (forkmode="once"), and the PR instead relies on reflection plus making TypeValidator’s flag mutable. If the intended solution is per-test forking, the build should isolate this test and avoid mutating production code / private static state via reflection.



-- 
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]