fix: add cyclic reference detection to TOML renderer and sync upstream test suites#961
Closed
He-Pin wants to merge 1 commit into
Closed
fix: add cyclic reference detection to TOML renderer and sync upstream test suites#961He-Pin wants to merge 1 commit into
He-Pin wants to merge 1 commit into
Conversation
…m test suites
Motivation:
`std.manifestTomlEx({a: {b: $}}, " ")` caused OutOfMemoryError because the TOML
renderer had no cycle detection, unlike JSON/YAML renderers which go through the
Materializer's protection. Additionally, upstream test suites (google/go-jsonnet)
had new cyclic reference tests and updated golden files that needed syncing.
Modification:
- Add depth limit and IdentityHashMap-based cycle detection to
ManifestTomlEx.renderTableInternal, matching the Materializer's approach
- Wrap ManifestTomlEx.evalRhs in StackOverflowError/OutOfMemoryError catch
- Fix refresh_golden_outputs.sh to strip Java runtime WARNING lines from
golden files (sun.misc.Unsafe deprecation warnings on JDK 21+)
- Fix sync_test_suites.sh to regenerate error golden files for new test files
using sjsonnet's error format instead of upstream's
- Add 3 new cyclic reference tests from go-jsonnet (JSON/TOML/YAML manifest)
- Update .sync_ignore with documented entries for path-dependent tests
(std.thisFile) and behavioral differences (float rendering, pow6, parseYaml)
Result:
- std.manifestTomlEx now correctly detects cyclic references instead of OOM
- All 103 tests pass including 3 new cyclic reference tests
- Test sync infrastructure properly handles Java warnings and error format
- Golden files accurately reflect sjsonnet's behavior vs upstream differences
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
1 participant
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.
Summary
Add cyclic reference detection to
std.manifestTomlEx: The TOML renderer had no cycle/depth protection, causingOutOfMemoryErroron cyclic objects like{a: {b: $}}. JSON and YAML renderers already had this protection via theMaterializer. This fix adds both anIdentityHashMap-based cycle detector and a depth limit check, with aStackOverflowError/OutOfMemoryErrorsafety net inevalRhs.Sync upstream test suites from google/go-jsonnet: 3 new cyclic reference tests were added from upstream, plus infrastructure fixes for golden file generation.
Fix golden file generation scripts: Strip Java runtime
WARNING:lines (sun.misc.Unsafe deprecation on JDK 21+) from golden files, and regenerate error goldens for new test files using sjsonnet's error format.Changes
ManifestModule.scaladepth+IdentityHashMapcycle detection torenderTableInternal; wrapevalRhsinStackOverflowError/OutOfMemoryErrorcatchrefresh_golden_outputs.shsed '/^WARNING:/d'after each golden generationsync_test_suites.shGOLDEN_REFRESH_FILES.sync_ignorestd.thisFile*) and behavioral differences (1.42rendering,pow6,parseYaml)manifestJsonEx,manifestTomlEx,manifestYamlDocBehavioral differences documented in
.sync_ignorebuiltin_escapeStringJson1.421.4199999999999999pow6parseYaml("---")null[null]std.thisFile*testdata/prefixTest plan
std.manifestTomlEx({a: {b: $}}, " ")now produces error instead of OOMstd.manifestJsonEx({a: $}, " ")andstd.manifestYamlDoc({a: $})error tests pass