fix(YAML::PP): stop CPAN suite crashes; add remaining-work plan#525
Merged
fix(YAML::PP): stop CPAN suite crashes; add remaining-work plan#525
Conversation
…align cyclic-ref message Investigated failures in `./jcpan -t YAML::PP` (YAML-PP v0.39.0). Previously the test suite hit SIGTERM partway through because several load_string() calls threw uncaught Java exceptions and Dubious-failed entire test programs. Changes to YAMLPP.java: * Catch NumberFormatException and YamlEngineException around load.loadAllFromString(). SnakeYAML Engine wraps NFEs from its Int/Float resolvers in a YamlEngineException whose message is "java.lang.NumberFormatException: For input string: ...". Translate these to a clean "YAML::PP: invalid numeric value: ..." die so tests like `!!int .inf`, `!!int 1_000`, `!!float x` continue instead of aborting the whole test file. * Handle Set<?> results from !!set tags: convert to a Perl hashref with undef values (previously fell through to default and stringified to "[a, b, c]"). * Handle byte[] results from !!binary tags: base64-encode instead of returning the JVM identity hash of the byte array. * Change cyclic-reference message to "Found cyclic reference in YAML structure" so it matches the `(?i:found cyclic ref)` regex used by t/32.cyclic-refs.t. * Parse `schema => ['Name', 'key=value', ...]` options. Only validate known key=value pairs (currently `empty=null|str`); non-key=value entries (e.g. secondary schema names like `'Perl'`) are accepted for backward compatibility with the existing unit test. Net effect on the CPAN t/ suite: * Whole suite now runs to completion (previously SIGTERM'd). * 2 more test files pass (t/36.debug.t, t/37.schema-catchall.t). * t/32.cyclic-refs.t: 4 failures -> 1. * t/31.schema.t no longer crashes on `!!int .inf`; now reports real subtest failures (bool/YAML1.1 not implemented) instead of dying. Remaining known limitations (not addressed here): YAML 1.1 boolean tags (!!bool yes/on/TRUE/...), !!omap ordering, load_file() of open filehandles, and many of snakeyaml-engine's parser limitations vs YAML::PP's Perl parser (tabs, %TAG directives, flow-mapping colon edge cases, etc.). Generated with [Devin](https://cli.devin.ai/docs) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Records current CPAN test baseline, what was fixed in the preceding commit, and groups remaining failures by the infrastructure change they need (parser strictness, YAML 1.1 schema, dump-side tagging, Perl-side API surface, etc.). Generated with [Devin](https://cli.devin.ai/docs) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
… msg
Follow-up to the previous YAML::PP fix. Picks up the low-hanging
remaining CPAN-test failures:
* load_file/DumpFile now accept an already-open Perl filehandle
(GLOB or IO::Handle-ish object). Implemented in the Perl wrapper
src/main/perl/lib/YAML/PP.pm so we can reuse existing Perl-side
filehandle machinery. DumpFile(path) also opens the file itself so
it can produce the "Could not open ..." error string that the
CPAN tests expect.
* load_string in scalar context now returns the first document
(matches CPAN YAML::PP's contract); list context still yields all
documents. load_file gets the same scalar/list handling.
* convertYamlToRuntimeScalar splits its identity map into an
"in-progress" set (real cycle detection) and a "finished" cache
(DAG memoization). Shared references created by anchor redefinition
+ alias (e.g. &NODEA ... &NODEA ... *NODEA) are no longer
misidentified as cyclic.
* SnakeYAML's "found duplicate key X" is rewritten to also contain
"Duplicate key 'X'" so t/57.dup-keys.t's qr{Duplicate key 'a'}
regex matches.
* cyclic_refs => 'nonsense' now dies with "Invalid value for
cyclic_refs: ..." instead of silently falling through to FATAL
(fixes t/32.cyclic-refs.t last subtest).
CPAN suite result for YAML-PP v0.39.0:
* 30/44 -> 26/44 programs failed.
* Newly passing: t/19.file.t, t/30.legacy.t, t/32.cyclic-refs.t,
t/57.dup-keys.t.
* dev/modules/yaml_pp.md updated with the new status table.
Generated with [Devin](https://cli.devin.ai/docs)
Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
fglock
force-pushed
the
fix/yaml-pp-cpan-tests
branch
from
ed2aa19 to
eaa7728
Compare
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
Investigation of
./jcpan -t YAML::PPuncovered several crashes that were truncating the CPAN test suite via SIGTERM. This PR fixes the crashes and documents the remaining failures.Fixes (
YAMLPP.java)NumberFormatExceptionandYamlEngineException.snakeyaml-enginewraps NFEs from its Int/Float resolvers inside aYamlEngineExceptionwhose message is literally"java.lang.NumberFormatException: For input string: ...". Translate these to a clean Perl-level die"YAML::PP: invalid numeric value: ...". Previously these killed whole test files (e.g.!!int .inf,!!int 1_000).!!settag: JavaSet<?>is now converted to a Perl hashref with undef values (matches YAML::PP convention). Previously fell through totoString()→"[a, b, c]".!!binarytag:byte[]is now base64-encoded. Previously returned the JVM identity hash ("[B@494c8f29")."Found cyclic reference in YAML structure"sot/32.cyclic-refs.t's regex(?i:found cyclic ref)matches.schemaoption parsing: secondary list entries are now read.key=valueoptions are validated (empty=null|stronly); invalid ones raise"Invalid option: ...". Non-key=valueentries (e.g.'Perl') are accepted to stay compatible with the existing unit test.Results
t/32.cyclic-refs.tt/36.debug.t,t/37.schema-catchall.tThe absolute failure count rose because the whole suite now runs; the pre-fix total only counted failures seen before the crash.
Plan for Remaining Work
Added
dev/modules/yaml_pp.mdwith a per-file status table and groups the outstanding failures by infrastructure change needed:t/12.load-json.t)load_file/DumpFileon open filehandles!!null, tokenizer, header/footer)Test plan
make(all unit tests pass, includingunit/yaml_pp.t)./jcpan -t YAML::PPruns to completion instead of being killedt/32.cyclic-refs.tregression fixed (4 → 1 failure)t/36.debug.t,t/37.schema-catchall.tGenerated with Devin