-
Notifications
You must be signed in to change notification settings - Fork 77
Simple crash_tests for yaml_edit #2285
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
A set of YAML files that we will load and then use yaml_edit to perform arbitrary edits, attempting to trigger internal errors. This works because `YamlEditor._performEdit` will perform the semantic mutation on the existing YamlNode tree, and then modify the YAML string re-parse it and compare it to the semantically expected YamlNode tree. This ensures that `YamlEditor` always produces valid YAML, and that the YAML has the expected semantics. The internal check in `YamlEditor` does not ensure that comments are preserved or that formatting is reasonable. But it does ensure that semantics are as expected, and that the YAML produced is valid. These tests simply exercises these mechancs. I've already added quite a few files that we need to immediately skip because we cannot perform certain mutations on them correctly. Is is particularly likely when comments are inserted in weird locations. How many issues we actually have is hard to quantify, it's quite likely that many of the bugs are the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a valuable set of crash tests for yaml_edit. The approach of fuzzing the YamlEditor by applying various mutations to different YAML structures is excellent for uncovering edge cases and ensuring robustness. The test implementation is well-structured, using modern Dart features effectively. I have one suggestion to improve the test code's clarity. Overall, this is a great addition to the test suite.
PR HealthLicense Headers ✔️
All source files should start with a license header. Unrelated files missing license headers
This check can be disabled by tagging the PR with API leaks ✔️The following packages contain symbols visible in the public API, but not exported by the library. Export these symbols or remove them from your publicly visible API.
This check can be disabled by tagging the PR with Breaking changes ✔️
This check can be disabled by tagging the PR with Coverage ✔️
This check for test coverage is informational (issues shown here will not fail the PR). This check can be disabled by tagging the PR with Changelog Entry ✔️
Changes to files need to be accounted for in their respective changelogs. This check can be disabled by tagging the PR with |
Package publishing
Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation. |
Co-authored-by: Sigurd Meldgaard <sigurdm@google.com>
Pull Request Test Coverage Report for Build 20336075955Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
A set of YAML files that we will load and then use yaml_edit to perform arbitrary edits, attempting to trigger internal errors.
This works because
YamlEditor._performEditwill perform the semantic mutation on the existing YamlNode tree, and then modify the YAML string re-parse it and compare it to the semantically expected YamlNode tree.This ensures that
YamlEditoralways produces valid YAML, and that the YAML has the expected semantics. The internal check inYamlEditordoes not ensure that comments are preserved or that formatting is reasonable. But it does ensure that semantics are as expected, and that the YAML produced is valid.These tests simply exercises these mechancs.
I've already added quite a few files that we need to immediately skip because we cannot perform certain mutations on them correctly. Is is particularly likely when comments are inserted in weird locations.
How many issues we actually have is hard to quantify, it's quite likely that many of the bugs are the same.
@kekavc24, I think this is an easy to way to add tests, and at-least make sure we don't have internal errors. I tried it with #2240 and it did seem to fix some of the issues. But feel free to add more for files and/or tests. It might be worth while adding small files that show specifically what we've fixed with each PR.
hmm, looking at it, I suspect I could also try to insert maps and lists in flow and block mode at each JSON path in these crash tests. Maybe, that's an exercise for later 🤣