Enable SurrogatePairLengthTest by elharo · Pull Request #116 · apache/xerces-j

elharo · 2026-07-17T15:03:11Z

SurrogatePairLengthTest requires setting a system property (org.apache.xerces.impl.dv.xs.useCodePointCountForStringLength) before TypeValidator.class loads. Since the class reads this property as a static final constant via AccessController.doPrivileged(Boolean.getBoolean(...)), it cannot run in the shared-JVM main <batchtest> (forkmode="once") — other tests load TypeValidator first, and the property is never read.

Added a separate <junit> task with forkmode="perTest" before the main batchtest to run this test in its own JVM, where the static initializer can set the property before any other class references TypeValidator.

Removed SurrogatePairLengthTest from the commented-out failure list since it now passes.

elharo · 2026-07-17T15:20:51Z

An alternative approach that avoids the separate forked JVM: instead of reading the system property once in a static final field, change TypeValidator.getDataLength() to call Boolean.getBoolean(...) directly. The system property is the public API — no new setter needed. The performance impact of reading a system property on each string-length check is negligible for a rarely-used debug flag.

// TypeValidator.java
public int getDataLength(Object value) {
    if (value instanceof String) {
        final String str = (String)value;
        if (!isCodePointCountEnabled()) {
            return str.length();
        }
        return getCodePointLength(str);
    }
    return -1;
}

private static boolean isCodePointCountEnabled() {
    return AccessController.doPrivileged((PrivilegedAction<Boolean>) () ->
        Boolean.getBoolean("org.apache.xerces.impl.dv.xs.useCodePointCountForStringLength")
    );
}

Then SurrogatePairLengthTest would work in the shared JVM — just set the property via System.setProperty() and let it be read lazily.

Change TypeValidator.getDataLength() to read the system property at runtime instead of caching it in a static final field. The test now uses setUp/tearDown to set and reset the property.

elharo · 2026-07-19T12:23:01Z

Updated approach: instead of a separate forked JVM, TypeValidator.getDataLength() now reads the system property at runtime via Boolean.getBoolean(). The test uses setUp()/tearDown() to set and reset the property. Tests run sequentially within the shared JVM, so this is safe. Comments added to both the test and build.xml documenting the sequential-execution dependency.

The previous approach added a runtime method with AccessController to TypeValidator, which is undesirable. Instead, we remove the 'final' keyword from the existing private static field (no other change to the initialization logic), then use simple Field.setAccessible/set in the test's setUp/tearDown to toggle the flag. No AccessController, no forked JVM, no JVM --add-opens flags needed.

elharo · 2026-07-19T12:39:22Z

    static {
-        System.setProperty("org.apache.xerces.impl.dv.xs.useCodePointCountForStringLength", "true");
+        try {
+            Field f = TypeValidator.class.getDeclaredField("USE_CODE_POINT_COUNT_FOR_STRING_LENGTH");


why is this all in a static block instead of setUp?

elharo · 2026-07-19T12:40:09Z

+
+    protected void tearDown() throws Exception {
+        System.clearProperty(PROPERTY);
+        codePointCountField.set(null, false);


do we know false was the original value here?

elharo · 2026-07-19T12:41:25Z

+              <include name="schema/config/UseGrammarPoolOnly_True_Test.class"/>
+              <include name="schema/config/UnparsedEntityCheckingTest.class"/>
+             -->
+             <!-- SurrogatePairLengthTest sets a system property in setUp() that


This needs to indicate that this test is not safe for parallel execution.

- Inline PROPERTY and LENGTH_ERROR constants per review feedback - Move all reflection setup from static block to setUp() - Save original field value before modifying, restore in tearDown - Update build.xml comment: explicitly state not safe for parallel execution

Copilot

Pull request overview

This PR aims to make SurrogatePairLengthTest pass reliably by ensuring the org.apache.xerces.impl.dv.xs.useCodePointCountForStringLength setting takes effect before TypeValidator uses it, so surrogate-pair length is counted in Unicode code points during validation.

Changes:

Updated SurrogatePairLengthTest to set the system property during setUp() and to force-enable the internal TypeValidator flag via reflection (restored in tearDown()).
Changed TypeValidator.USE_CODE_POINT_COUNT_FOR_STRING_LENGTH from static final to mutable static so it can be altered reflectively.
Re-enabled SurrogatePairLengthTest in build.xml and added a note about sequential execution constraints.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File	Description
tests/schema/config/SurrogatePairLengthTest.java	Sets the property in setup and uses reflection to toggle/restore `TypeValidator`’s internal behavior for surrogate-pair length counting.
src/org/apache/xerces/impl/dv/xs/TypeValidator.java	Makes the internal “use code point length” flag mutable (no longer `final`).
build.xml	Includes `SurrogatePairLengthTest` in the test batch and documents its (non-parallel-safe) behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

+    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();
    }


 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() {


garydgregory

Note that a better way to manage system properties in unit tests can be achieved more cleanly using JUnit Pioneer:
https://junit-pioneer.org/docs/system-properties/

Revert the reflection-based approach. Instead, SurrogatePairLengthTest now runs in its own forked JVM via a separate <junit> task in build.xml. The test uses a static initializer to set the system property before TypeValidator loads, so its private static final field picks it up. No changes to TypeValidator.java. No reflection, no AccessController.

TypeValidator's private static final field reads the system property at class load time, so it must be set before the class loads. The <junit> task now sets it via <sysproperty> so it's available before any class loading in the forked JVM. The test uses setUp/tearDown to save and restore the property value for correct test hygiene.

TypeValidator loads during BaseTest.setUp() (via sf.newSchema()). Setting the system property before calling super.setUp() ensures it is available when TypeValidator's static final field is initialized. No static initializer, no sysproperty in build.xml, no reflection. Property strings inlined, value saved and restored in tearDown.

elharo · 2026-07-19T15:43:57Z

PTAL I think this is finally in the best shape it can be for now

Enable SurrogatePairLengthTest in its own forked JVM

c37c1eb

elharo marked this pull request as draft July 17, 2026 15:14

elharo added 2 commits July 17, 2026 23:45

Merge branch 'main' into fix-surrogate-test

3baa279

Enable SurrogatePairLengthTest without forked JVM

36364ca

Change TypeValidator.getDataLength() to read the system property at runtime instead of caching it in a static final field. The test now uses setUp/tearDown to set and reset the property.

elharo changed the title ~~Enable SurrogatePairLengthTest in a separate forked JVM~~ Enable SurrogatePairLengthTest without forked JVM Jul 19, 2026

elharo commented Jul 19, 2026

View reviewed changes

elharo marked this pull request as ready for review July 19, 2026 12:50

elharo requested review from Copilot and garydgregory July 19, 2026 12:50

elharo changed the title ~~Enable SurrogatePairLengthTest without forked JVM~~ Enable SurrogatePairLengthTest Jul 19, 2026

Copilot started reviewing on behalf of elharo July 19, 2026 12:50 View session

Copilot AI reviewed Jul 19, 2026

View reviewed changes

garydgregory approved these changes Jul 19, 2026

View reviewed changes

elharo added 2 commits July 19, 2026 13:03

elharo commented Jul 19, 2026

View reviewed changes

Comment thread build.xml Outdated

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Enable SurrogatePairLengthTest - #116

Enable SurrogatePairLengthTest#116
elharo wants to merge 8 commits into
mainfrom
fix-surrogate-test

elharo commented Jul 17, 2026

Uh oh!

elharo commented Jul 17, 2026

Uh oh!

elharo commented Jul 19, 2026

Uh oh!

Uh oh!

elharo Jul 19, 2026

Uh oh!

elharo Jul 19, 2026

Uh oh!

elharo Jul 19, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

garydgregory left a comment

Uh oh!

Uh oh!

elharo commented Jul 19, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

elharo commented Jul 17, 2026

Uh oh!

elharo commented Jul 17, 2026

Uh oh!

elharo commented Jul 19, 2026

Uh oh!

Uh oh!

elharo Jul 19, 2026

Choose a reason for hiding this comment

Uh oh!

elharo Jul 19, 2026

Choose a reason for hiding this comment

Uh oh!

elharo Jul 19, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

garydgregory left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

elharo commented Jul 19, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants