[refactor] Extract assertion-evaluation fixes from #49#61
Merged
Conversation
Test catalog CDATA often has formatting newlines (e.g., after </result> before ]]>) that become spurious text nodes inside the ignorable-wrapper element, causing child count mismatches like "Expected child nodelist length '1' but was '2'". Trim the expected XML string before wrapping it in the ignorable-wrapper element to eliminate these false failures. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…dard namespaces
The runner was extracting only the local part of XPathException error
codes, losing the namespace URI. QT4 test catalogs use EQName notation
(e.g., Q{http://expath.org/ns/file}is-dir) for extension module error
codes. Now error codes from non-standard namespaces (not xqt-errors or
exist-xqt-errors) are formatted as Q{ns}local for proper matching.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…rors When an XPath assertion (assert, assert-eq, assert-deep-eq, assert-permutation, assert-serialization) raises a query error during evaluation (e.g., XPTY0004 type mismatch), this indicates the assertion failed — not that the runner itself errored. Previously these were reported as ErrorResults, inflating the error count and masking the real nature of the failure. Now they are reported as FailureResults with the error details in the message. Fixes fn-parse-json-717 and fn-parse-json-731 which errored due to type mismatches in assertion XPath evaluation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…tServer Add sequenceToStringRaw() for serialization-matches assertions where the exact serialized output must be preserved without newline replacement. Refactor sequenceToString into a shared implementation with a sanitize flag. Add serializationProperties field to Result to capture query context serialization options (e.g., declare option output:method "json") for use in assertion evaluation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Change getBroker() to authenticate("admin", "") to get an admin-level
broker instead of a guest broker. Required for modules that need
filesystem or privileged access (e.g., EXPath File module operations).
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…tion When a query returns an error and the expected result is an AllOf assertion containing an Error assertion, match the error code against the Error inside the AllOf. Previously, only direct Error and AnyOf assertions were matched; AllOf was not handled, causing false failures for tests like EXPath File read-binary bounds checking where the catalog wraps the expected error in <all-of>. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…hitespace Fix two related issues in findDifferences (used by assertXml): 1. The DiffBuilder.compare() and withTest() arguments were swapped, causing 'Expected' and 'but was' labels in error messages to be reversed (the actual was treated as the expected and vice versa). 2. Add a node filter that drops whitespace-only text nodes before comparison, so insignificant whitespace in test catalog CDATA does not produce spurious child-count or text-mismatch failures. Combined extraction of the assertion-correctness portions of upstream commits 9a316ac and ea771f6. The normalizeWhitespace plumbing from ea771f6 (specific to XQFTTS) is deferred to the suite-additions wave; this commit keeps the runner XQ 3.1-only and surgical. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
XQTS assert expressions like ?columns and ?get(1,1) use unary lookup which requires a context item. Previously the result was only available as the $result variable but not as the context item, causing XPDY0002 errors for any assertion using the ? lookup operator. Set context only for single-item results (e.g., maps from parse-csv) to avoid per-item evaluation for multi-item sequences such as csv-to-arrays output. Extracted from upstream commit c6f3caa; assertion-only. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
3 tasks
line-o
approved these changes
May 30, 2026
Member
line-o
left a comment
There was a problem hiding this comment.
Pulling this in and cutting a RC2 afterwards
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
[This response was co-authored with Claude Code. -Joe]
Summary
Small, focused extraction of the assertion-evaluation bug fixes from #49, per @line-o's request to keep this round's changes phone-reviewable and to unblock an RC2 cut. Also folds in the admin-authentication change from #49 (commit 5 below), which is a prerequisite for the in-flight EXPath File built-in extension PR (eXist-db/exist#6257).
8 commits, touching only
TestCaseRunnerActor.scalaandExistServer.scala. None overlap with #58. Compile is clean against currentdevelop. XQTS HEAD measured before and after with a companion eXist fix (eXist-db/exist#6409) in place.What's in scope
[bugfix] Trim expected XML in assertXml to avoid whitespace mismatchesassertXmlfailures from formatting newlines in catalog CDATA[bugfix] Preserve namespace URI in error code comparison for non-standard namespacesQ{http://expath.org/ns/file}is-dir) instead of bare local name[bugfix] Treat assertion evaluation errors as failures, not runner errors[refactor] Add raw serialization and serialization properties to ExistServer[bugfix] Use admin authentication for embedded server connections[bugfix] Handle AllOf assertions containing Error in test case evaluationErrorinside<all-of>(previously only directErrorand<any-of>were handled)[bugfix] Fix XMLUnit comparison argument order and filter ignorable whitespace[bugfix] Pass result as context item for single-item assert evaluations?lookup operator on single-item results (e.g.?columnson a parse-csv map)Commits 1–5 (in chronological order: trim-XML, preserve-NS-URI, errors-as-failures, raw-serialization, admin-auth) cherry-pick cleanly from #49. Commits 6–8 (AllOf-with-Error, XMLUnit-fix, context-item) are hand-rewrites against current develop: their original #49 SHAs modify file regions that exist only on the QT4/Update/XQFTTS base branch and don't apply directly. Each hand-rewrite is the assertion-relevant portion of its source commit; the structural QT4/Update bits were dropped.
What was considered but deferred
[feature] Implement XQTS sandpit support for writable test directories[bugfix] Propagate static base URI to assertion evaluation context9a316ac)runUpdateTestCase(XQUF)normalizeWhitespace()plumbing (XQFTTS half ofea771f6)[bugfix] Propagate testCase environment namespaces to assertion queriesdevelopvia #52 (different design — parameter passing instead of actor-private var) — confirmed redundantVerification
XQTS HEAD measured before and after using the same shaded exist-core. The "after" measurement is the runner PR + the companion eXist fix #6409 in place:
Test movement (transitions where both runs have data):
fail→passare the genuine fixes from commits 1, 6, 7, 8.error→failare the intentional reclassification from commit 3.pass→failare detailed below; all 5 are accounted for.Other verification:
sbt compileclean (only pre-existing 23 Scala warnings, none introduced)mvn test -pl exist-coreon develop unchanged by this PR (the runner is consumed by theexist-xqtsappassembler bootstrap, not by exist-core's test JVM); 6,592 tests / 0 failures / 0 errors / 106 skipped, 3:39 wall.Admin-auth behavior changes (commit 5)
Commit 5 changes the embedded server connection from
getBroker()(guest) toauthenticate("admin", "")(admin). This is a prerequisite for the in-flight EXPath File built-in PR (#6257) — privileged modules need admin to operate. The change surfaces 11 pass→fail transitions in XQ 3.1 HEAD, in three clusters:Cluster A —
fn:environment-variableis dba-only (4 tests):fn:environment-variablereturns the empty sequence for non-dba users. Tests likefn-environment-variable-005use a defensive guard:Under guest the guard's first clause is true (env vars hidden, function returns
()), so the test passes regardless of fixtures. Under admin the function returns the real env vars, the guard's first clause is false, and the test correctly reports false because the test fixture (QTTEST=42) isn't set in the runner's environment.These are spec-correct failures revealing a runner-side test-fixture gap, not eXist behavior changes. Affected tests:
fn-available-environment-variables-011,fn-environment-variable-005,-006,-007.Cluster B — lone-slash predicate with admin context (6 tests):
All six are "lone slash" tests of the form
fn:count(.[5 * /])against a single-element source doc. The cause is an eXist optimizer bug whereRootNodedoesn't declareCONTEXT_ITEMas a dependency; the predicate gets hoisted out of the iteration context,RootNode.evalfalls through to the static-context default, and under admin returns content from/db/system/security/. Diagnosed by instrumentingOpNumeric.eval:rseqcontains atomized fields fromadmin.xml, including two RIPEMD160 password hashes. Fixed by #6409 — once that merges, all 6 of these tests flip back to pass.Affected tests:
prod-PathExpr/PathExpr-1, -2, -15andprod-StepExpr/Steps-leading-lone-slash-15, -17, -1a.Cluster C —
fn-transform-43(1 test, unrelated):fn-transform-43is anfn:transformtest that callsparse-xmlon a serialized result. Fails in the batched XQTS run only — passes when runningfn-transformalone, orfn-transform-43in isolation. The diff is cross-test-set JVM state pollution (Saxon configuration cache, XSLT compilation cache, or similar), not anything tied to admin auth or this PR's changes. Predates these changes; not blocking; worth a separate look later.Net assessment with #6409 applied: Cluster A is correct behavior surfaced (4 tests, document); Cluster B is fully fixed (6 tests recovered); Cluster C is a 1-test pre-existing batched-run intermittency. Keeping commit 5 in this PR unblocks the path to EXPath File integration (#6257) and ships strictly positive on the XQTS measurement once #6409 lands.
Suggested merge order
developexist-coresnapshot to publish via the runner's Maven resolutionexist-xqtsdep-bump followRelated