Update ion-tests and start cataloging conformance issues. #911
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #911 +/- ##
==========================================
+ Coverage 77.53% 77.73% +0.19%
==========================================
Files 137 137
Lines 34283 34283
Branches 34283 34283
==========================================
+ Hits 26583 26651 +68
+ Misses 5717 5655 -62
+ Partials 1983 1977 -6 ☔ View full report in Codecov by Sentry. |
|
Unit test failures on windows due to skip-list not allowing windows separators. As much as I try to forget windows exists.. it's still there. |
| self.name_width = collection.iter().fold(0, |acc, d| { | ||
| std::cmp::max(acc, d.name.as_ref().map_or(0, |n| n.len())) | ||
| }); |
| /// A test reporter that renders the results of all tests to the terminal in a human readable way, | ||
| /// mimicking the output of cargo test. | ||
| #[derive(Default)] | ||
| struct PlainReporter { |
| .collect::<Vec<String>>(); | ||
| let emit_ion = Some("-i") == options.get(0).map(|v| v.as_str()); | ||
|
|
||
| let mut reporter: Box<dyn TestReporter> = if emit_ion { |
There was a problem hiding this comment.
This may not need to be Boxed? It seems like &mut dyn might work.
| let old_hook = std::panic::take_hook(); | ||
| // Limit the span that we've hijack'd the panic hook, so that if something goes wrong | ||
| // with the unit test outside of our conformance eval, we don't conflate the two. | ||
| std::panic::set_hook({ |
There was a problem hiding this comment.
It's been so long since I tried to catch a panic that I didn't realize you could set a handler for the current thread. o_O
| #[test_resources("ion-tests/conformance/ion_encoding/module/macro/template/*.ion")] | ||
| #[test_resources("ion-tests/conformance/ion_encoding/module/macro/trivial/*.ion")] | ||
| #[test_resources("ion-tests/conformance/system_macros/*.ion")] | ||
| #[test_resources("ion-tests/conformance/tdl/*")] |
There was a problem hiding this comment.
Does this macro support ** syntax for nesting at any depth? (Or maybe you're enumerating the subdirectories to leave something out?)
| println!("Testing: {}", file_name); | ||
| let collection = TestCollection::load(file_name).expect("unable to load test file"); | ||
| // Test for skip list. Convert windows '\\' separators into '/' to match skiplist. | ||
| if !GLOBAL_CONFORMANCE_SKIPLIST.iter().any(|f| *f == file_name.replace("\\", "/")) { |
There was a problem hiding this comment.
This works; here's another place we solved the same issue differently:
ion-rust/tests/detect_incomplete_text.rs
Lines 16 to 30 in 0943766
Issue #, if available: #902
Description of changes:
This PR adds a skip list to the conformance tests run during CI/CD, while also adding all of the current conformance tests to the generated unit tests.
The skip list is implemented to skip test files, which may contain multiple tests. This was done because tests are not required to have a name, and many of them do not. This means that a test file cannot be removed from the skip list until each test within passes. I'll be tracking all of the individual tests, and can post a more detailed list, which I will also be sharing with the team shortly. The comments captured in the skip list are mostly the first error to trigger the test failure. I'll keep them updated as I work through them, but the list I'll share will be the most accurate summary of issues.
In addition the conformance CLI has been updated to catch panics and recover, so that we can run all tests within a file even if one of the tests hits a
todo!, or fatal error.The CLI has also been updated to allow a
-ioption, which can be specified before any files in order to output the test results in an ion format for easier processing.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.