test: compare the ported ConstraintSetParser against the oracle - #358
Merged
Conversation
…rash path observed
… documents
minimumPopulated counted the Populated case, not what was in it: a generator
that regressed to emitting {} for every spec would have both sides return
Populated("", "") for all 2000 seeds, clearing the 1800 floor while comparing
nothing. Add a geometryRows accumulator and a 5000-row floor -- under half the
10647 measured against the current corpus -- which a corpus of substance-free
documents can never reach.
AxisLivenessTest mutates one axis of the generated document model at a time and requires the rendered outcome to differ, so a field that varies without reaching the parser (the failure mode a sibling harness shipped silently for weeks) shows up immediately instead of hiding inside "everything is green". alpha, the rotations, the scales, the translations and the pivots land on WidgetFrame, a sibling of the solver's box, not on left/top/width/height - GeometryRow widens to carry them, folded into the existing one-line-per-widget row so ConstraintSetDifferentialTest's newline-counting geometry-row floor stays meaningful unchanged. Two independent findings came out of making every axis prove itself, both left as @ignore'd tests rather than deleted, per this repo's own precedent (NestedLayout's @ignore, pinned in NestedContainerTest): - hBias, vBias, hRtlBias, centerVertically, chainStyle, hWeight and vWeight all reach ConstraintReference correctly, but never change the observed box: bias and chain-run resolution live in ConstraintWidgetContainer's dependency-graph analysis, which only runs via measure(...) with a real Measurer - neither subject calls it, confirmed by an isolated widget giving the identical position for hBias = 0.1 and hBias = 0.9. This is a harness gap, not a dead axis, and fixing it (teaching both subjects to measure()) is a bigger, riskier change than this task's scope. - VariableSpec.Num and VariableSpec.Generator are stored into LayoutVariables by parseVariables and then never read again: nothing in this document model lets any field reference a variable by name, so both are genuinely dead as currently generated. Full investigation in the task report.
Two rounds of fix. First round's @ignore diagnoses turned out to need a real fix rather than acceptance, per the coordinator - both landed, so every axis in AxisLivenessTest now passes for real: 42/42, zero skipped. hBias/vBias/hRtlBias/centerVertically/chainStyle/hWeight/vWeight: added a `measure` entry point to ConstraintSetSubject (mirroring solver.OracleSolver's Measurer pattern) alongside the untouched `parse`. Bisecting outside the harness (raw ConstraintWidget, then State directly, no JSON) found the actual defect was never layout() vs measure() as first suspected - it was that neither entry point ever told State the document's root size, so State defaulted the root to WRAP_CONTENT. That default is invisible to a single-anchor widget, which is why parse-backed cases were unaffected, but it silently disables ConstraintWidget.applyConstraints' bias-centering equation and the chain/MATCH_CONSTRAINT machinery, both of which special-case an unresolved parent. Fixed by adding state.setWidth/setHeight to `measure` only; `parse` is untouched (verified via git diff: zero removed lines in either subject), so ConstraintSetDifferentialTest's corpus is unaffected. variableNum/variableGenerator: added FloatValue (Literal | Named) and changed WidgetSpec.alpha to carry one, so a document can write `alpha: 'v0'` instead of a number - matching how ConstraintSetParser.applyAttribute actually reads every transform/bias attribute. Scenarios.generate now draws its variables before its widgets so a widget can reference one by name, rarely (1 in 20) and only among variables the same document declares. Re-measured the differential corpus after reordering the seed's draw sequence: 1876 populated / 10576 geometry rows, versus 1889/10647 before - a ~0.7% shift, comfortably clear of both floors. Full investigation, including the wrong first diagnosis and how it was narrowed to the real one, is in the task report.
AxisLivenessTest certified hBias/vBias/hRtlBias/centerVertically/chainStyle/ hWeight/vWeight live through ConstraintSetSubject.measure, but the 2000-seed differential test - the only thing that actually compares oracle against port at scale - only ever called parse. A liveness certificate for a path nothing checks for correctness said nothing about whether the port agrees with the oracle on bias or chain resolution, which is exactly the region Fix 1 just finished unlocking. Rewrote ConstraintSetDifferentialTest following solver.SolverDifferentialTest's Entry-enum structure: both parse and measure run for every seed, each compared only against its own counterpart (never parse against measure, a different contract), with per-entry populated/geometry-row floors and per-entry-keyed divergence examples so one entry's failures can't crowd the other's out of the report. parse is untouched - this is the only file this fix needed to change. Measured both entries before setting floors, per seed 1..2000: populated=1876, geometryRows=10576, divergences=0 for BOTH parse and measure. The identical counts aren't a mistake - row count depends only on how many widgets/guidelines/barriers a document materialises, which parse and measure agree on; they disagree (or, per this run, don't) on the numbers inside each row. Zero divergences on measure is the first corpus-scale confirmation that the port's bias, chain-style and chain-weight resolution actually agrees with the oracle - previously only spot-checked one hand-built document at a time. Full investigation in the task report.
parseDesignElementsJSON is broken identically on both sides (two indexing bugs present verbatim in the vendored upstream Java and reproduced line-for-line in the port), so the differential equality check here is a real but weak signal: it proves the port matches upstream, not that the entry point does anything useful. Floors are calibrated against an instrumented 500-seed run and cover both failure directions (generator decaying toward "nothing produces" or "nothing throws"), documented in the class kdoc.
Closes out the review's cleanup wave on the constraintset differential harness: strengthens the liveness check to also require the mutated document lay out (so a leak/crash can't masquerade as proof an axis is live), corrects four kdocs that stated something false about the parser or the type they described, notes a real coverage gap in the generator's anchor targeting, pins JsonEmitter's Bounded dimension shape with a test, extracts the byte-identical observation loop duplicated across both subjects, removes the one unforced asymmetry between the two subject files, and puts the previously-dead ConstraintSetSubject.name to use in an assertion message. No production code changes; parity module only.
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.
A fourth differential harness in
:parity, comparing the ported KotlinConstraintSetParseragainst the vendored upstream Java copy over generated ConstraintSet JSON documents.Before this,
ConstraintSetParser— 2040 lines, the largest file incore/stateand the hot path for every JSON-drivenConstraintLayoutandMotionLayoutuse — was covered by one test file of 101 lines containing two tests that print to stdout and share a singleassertNotNullbetween them.Nothing under
compose/src/is touched.Results
:parity:testHow it works
A syntax-free
ConstraintSetSpecdescribes a document;JsonEmitterrenders it to DSL text; two subjects parse that identical string with their own package's classes, apply the resultingStateto a container, and render the geometry plus every custom attribute. The differential test requires the two renderings to be equal. Same shape as thesolver/andmotion/harnesses.The subjects drive
populateState(CLParser.parse(json), state, LayoutVariables())rather than the publicparseJSONwrapper. The wrapper catchesCLParsingExceptionand prints it, leaving theStatehalf-populated with no signal — faithful to upstream, which does the same, but blind as an observation point.The part worth reading
Nine of the forty-two axes were dead on the first pass. Seven — biases, weights, chain style,
centerVertically— were generated and emitted while the observed result did not depend on them at all. The cause turned out to be deeper than a missingmeasure()call: neither subject toldStatethe document's root size, so the root defaulted toWRAP_CONTENT, which silently disables bias-centering and chain/MATCH_CONSTRAINTresolution. The other two were variables, which the parser resolves throughlayoutVariables[...]for every transform and bias attribute — the emitter simply never referenced one by name.Without that check, the headline "zero divergences over 2000 documents" would have been reported over a corpus where a large slice of the generated variety never reached the code. This is the same failure the
motion/harness shipped a month ago with five dead axes, green the whole time.The first fix landed one level short.
setWidth/setHeightwent into the newmeasureentry, but the differential test called onlyparse— so seven axes were certified live through a path the 2000-seed comparison never used. The test now runs both entries, comparing each against its own counterpart, asSolverDifferentialTestalready does. Measured independently:parseandmeasureproduce different geometry for 1862 of 1876 laid-out documents, so the second entry is real coverage rather than a copy.The harness is shown able to fail before being trusted to pass. Making
parseDimensionModereturncreateWrap()for"spread"turns the differential test red on 405 of 2000 seeds; reverting turns it green. The guards are derived from measurement, not guessed:minimumPopulatedcatches wholesale rejection,minimumGeometryRowscatches wholesale vacuity — a corpus of{}documents would satisfy the first and fail the second.Upstream defects found, and deliberately not fixed
Three, all present verbatim in the vendored Java, so the port is faithful and correcting any of them would create a divergence:
parseDesignElementsJSONindexes its inner loop with the outer loop's variable and labels every element with the literal string"Design"instead of its own id. Three separate agents reported this as a port defect during development; it is not.parseColorString's KDoc promises-1on unparseable input but throwsNumberFormatExceptionfor#-prefixed values that are not six hex digits.parseVariablesreads numbers throughCLNumber.getInt(), which doescontent().toInt()and throws on any fractional literal, inconsistently withgetFloat()in the same class.The generator deliberately draws inputs that reach the last two at a low rate, because a crash both sides produce identically is legitimate differential coverage of an error path.
What this does not cover
No generated document anchors a widget to a guideline, a barrier or a chain — the generator builds widgets first, so anchor targets can only name widgets. Guidelines and barriers do appear in the corpus and their own positions are compared, but the interaction they exist for is compared nowhere. This is recorded in the generator's KDoc so the coverage claim is not read as broader than it is.
parseMotionSceneJSONandparseTransitionsare out of scope; they reachTransition.kt(1247 lines) and need their own scenario model. Natural next increment.parseDesignElementsJSON's differential test necessarily passes, because the function is broken identically on both sides. Its own KDoc says so plainly: a pass there confirms the port matches upstream, not that the function does anything useful.