Add conformance testing DSL #826

nirosys · 2024-08-28T08:04:15Z

Issue #, if available: n/a

Description of changes:
This PR adds an implementation of the conformance DSL, along with a runner for testing conformance tests during CI/CD, and a separate runner that can be used to target files more ad-hoc. The ion-tests submodule has also been updated in order to pull in the conformance tests, which also required some updates to the test_resource paths for 1.0.

Most of the code is modeled directly after the grammar. Documents contain fragments and a continuation. Expectations, which are a subset of continuations, act as terminal nodes and trigger the evaluation of the test. Context tracks the fragments, version, and encoding, for the test's evaluation path.

CI/CD Runner

$ cargo test -F experimental --test conformance_tests
   Compiling ion-rs v1.0.0-rc.7 (/Users/glitch/Code/ion-rust)
    Finished `test` profile [unoptimized + debuginfo] target(s) in 3.65s
     Running tests/conformance_tests.rs (target/debug/deps/conformance_tests-6a032b27940772cd)

running 8 tests
test ion_tests::conformance_ion_tests_conformance_null_ion ... ok
test implementation::test_encoding ... ok
test ion_tests::conformance_ion_tests_conformance_core_string_symbol_ion ... ok
test implementation::test_timestamps ... ok
test ion_tests::conformance_ion_tests_conformance_core_empty_document_ion ... ok
test implementation::test_simple_docs ... ok
test ion_tests::conformance_ion_tests_conformance_core_typed_null_ion ... ok
test ion_tests::conformance_ion_tests_conformance_core_toplevel_produces_ion ... ok

test result: ok. 8 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.02s

$

CLI Runner

$ cat test.ion
(ion_1_1 "Test Bytes" (text "{{aGVsbG8=}}") (denotes (Blob "68 65 6c 6c 6f")))
(ion_1_1 "Test Bytes (Fail)" (text "{{aGVsbG8=}}") (denotes (Blob "68 65 6c 6c 5f")))

(ion_1_1 "Test Clob" (text "{{ \"hello\" }}") (denotes (Clob "68 65 6c 6c 6f")))
(ion_1_1 "Test Clob (Fail)" (text "{{ \"hello\" }}") (denotes (Clob "68 65 6c 6c 5f")))

$ cargo test -F experimental --test conformance -- ./test.ion
   Compiling ion-rs v1.0.0-rc.7 (/Users/glitch/Code/ion-rust)
    Finished `test` profile [unoptimized + debuginfo] target(s) in 1.92s
     Running tests/conformance.rs (target/debug/deps/conformance-62318380ee1917b3)
Testing 1 conformance collections.


Running tests: ./test.ion ========================
   Test Bytes         ...  [OK]
   Test Bytes (Fail)  ...  [FAILED]
   Test Clob          ...  [OK]
   Test Clob (Fail)   ...  [FAILED]
-------------------------
File: ./test.ion
Test: Test Bytes (Fail)
Error: ConformanceError(ConformanceErrorImpl { file: "", test_name: "", kind: MismatchedDenotes })
-------------------------
File: ./test.ion
Test: Test Clob (Fail)
Error: ConformanceError(ConformanceErrorImpl { file: "", test_name: "", kind: MismatchedDenotes })
thread 'main' panicked at tests/conformance.rs:51:9:
Conformance test(s) failed
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
error: test failed, to rerun pass `--test conformance`
$

Notes

The MacTab fragment is implemented as the spec describes, expanding into a toplevel fragment with named module for defining the macro table. Adding a new named module is not supported in ion-rust yet. Macros can still be added using the encoding fragment.
Absent symbols are currently not supported.

I'll follow up after this PR to make errors reporting better. I also have some plans for refactoring and simplifying some of the implementation, post merge.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

…rgettable test

… text clause

codecov · 2024-08-28T18:26:36Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.74%. Comparing base (fa3167d) to head (a8e2538).
Report is 94 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #826      +/-   ##
==========================================
- Coverage   80.60%   77.74%   -2.86%     
==========================================
  Files         140      134       -6     
  Lines       28209    32623    +4414     
  Branches    28209    32623    +4414     
==========================================
+ Hits        22737    25363    +2626     
- Misses       3641     5299    +1658     
- Partials     1831     1961     +130

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…ws version can match

zslayton

This looks good! Please do a documentation pass. It would be helpful to connect implementation sections to the part of the DSL spec they implement. Having an explanation of the motivation for top-level types like ModelValue would also be nice.

zslayton · 2024-08-29T20:17:13Z

src/lazy/binary/raw/v1_1/value.rs

+            let ion_type = if self.encoded_value.header.ion_type_code == OpcodeType::TypedNull {
+                let body = self.value_body();
+                ION_1_1_TYPED_NULL_TYPES[body[0] as usize]
+            } else {
+                IonType::Null
+            };
+            return Ok(ValueRef::Null(ion_type));


Why was this needed? ion_type() should return the correctly interpreted type from the opcode and optional typed null byte. (return Ok(ValueRef::Null(self.ion_type())) also appears below on line 227).

When parsing the Opcode we don't have the byte containing the true ion type (here), so the actual ion type was originally deferred until the value is read (here). Right now ion_type() just returns the type from the opcode, but you're right, it's a better place to be handling this logic.

zslayton · 2024-08-29T20:22:57Z

tests/conformance_dsl/context.rs

+            v => v,
+        };
+
+        // Ok(Reader::new(AnyEncoding, data_slice)?.read_all_elements()?)


Old comment?

Suggested change

// Ok(Reader::new(AnyEncoding, data_slice)?.read_all_elements()?)

// Ok(Reader::new(AnyEncoding, data_slice)?.read_all_elements()?)

zslayton · 2024-08-29T20:28:01Z

tests/conformance_dsl/model.rs

+use std::collections::HashMap;
+
+#[derive(Debug, Clone, Eq, Hash, PartialEq)]
+pub(crate) enum SymTok {


I suggest spelling out the words:

Suggested change

pub(crate) enum SymTok {

pub(crate) enum SymbolToken {

zslayton · 2024-08-29T20:30:05Z

tests/conformance_tests.rs

+    #[test_resources("ion-tests/conformance/core/string_symbol.ion")]
+    #[test_resources("ion-tests/conformance/core/empty_document.ion")]
+    #[test_resources("ion-tests/conformance/core/toplevel_produces.ion")]
+    // #[test_resources("ion-tests/conformance/ivm.ion")]


Suggested change

// #[test_resources("ion-tests/conformance/ivm.ion")]

// #[test_resources("ion-tests/conformance/ivm.ion")]

?

Yea, I had commented this out because it currently doesn't pass due to the symbol syntax that's needed. Removing it for now.

…is not a fragment

zslayton · 2024-09-03T18:15:02Z

src/lazy/binary/raw/v1_1/value.rs

+        // Handle retrieving the type for a typed null.
+        if self.encoded_value.header.type_code() == OpcodeType::TypedNull {
+            let body = self.value_body();
+            ION_1_1_TYPED_NULL_TYPES[body[0] as usize]
+        } else {
+            self.encoded_value.ion_type()
+        }


Could you put this functionality in a self.read_null() method instead of having it happen in ion_type()?

There are three other raw value types besides LazyRawBinaryValue_1_1:

LazyRawBinaryValue_1_0

LazyRawTextValue_1_1

LazyRawTextValue_1_0

Each of them has an ion_type() method and I'm pretty sure at least some of them return IonType::Null if the value is a null value, even if it's a typed null. The behavior you've implemented is probably more correct, but if we make this change here we'd need to make it in the other three places too.

Added #828 to track this.

ack, ty, I will follow up with that today.

…ests

nirosys added 6 commits August 26, 2024 02:17

Add conformance testing DSL implementation, CI/CD runner, and user ta…

318a8ea

…rgettable test

Fix remaining paths for ion 1.0 iontestdata

2c3808d

Handle typed nulls in read_resolved

ad2b1ac

Add support for more data model clauses

1743ea4

Remove left over comments; fix byte range; support multiple string in…

a951e27

… text clause

Update 1.1 skip list to address ion-test path changes

a8e2538

nirosys added 6 commits August 28, 2024 11:32

Address clippy checks, remove unused test

703bba0

Drop path separators from path replacement so the canonicalized windo…

cf5dd98

…ws version can match

Remove conformance cli from default tests

6811fbf

Add encoding and mactab fragments

51b75f3

Add timestamp to denote data model

5204862

Missed some uses when --patch'ing

2a69f4e

nirosys marked this pull request as ready for review August 29, 2024 11:26

zslayton reviewed Aug 29, 2024

View reviewed changes

Address PR feedback; Remove unused Each variant of Fragment since it …

499e649

…is not a fragment

nirosys requested a review from zslayton September 3, 2024 10:12

zslayton approved these changes Sep 3, 2024

View reviewed changes

zslayton mentioned this pull request Sep 3, 2024

Make ion_type() correctly reflect typed nulls #828

Open

Merge remote-tracking branch 'origin/main' into rgiliam/conformance_t…

138f6b2

…ests

nirosys merged commit 6f04a8e into amazon-ion:main Sep 3, 2024
25 of 26 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add conformance testing DSL #826

Add conformance testing DSL #826

nirosys commented Aug 28, 2024 •

edited

Loading

codecov bot commented Aug 28, 2024

zslayton left a comment

zslayton Aug 29, 2024

nirosys Aug 29, 2024

zslayton Aug 29, 2024

zslayton Aug 29, 2024

zslayton Aug 29, 2024

nirosys Sep 3, 2024

zslayton Sep 3, 2024

zslayton Sep 3, 2024

nirosys Sep 3, 2024

	// Ok(Reader::new(AnyEncoding, data_slice)?.read_all_elements()?)
	// Ok(Reader::new(AnyEncoding, data_slice)?.read_all_elements()?)

	// #[test_resources("ion-tests/conformance/ivm.ion")]
	// #[test_resources("ion-tests/conformance/ivm.ion")]

Add conformance testing DSL #826

Add conformance testing DSL #826

Conversation

nirosys commented Aug 28, 2024 • edited Loading

CI/CD Runner

CLI Runner

Notes

codecov bot commented Aug 28, 2024

Codecov Report

zslayton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nirosys commented Aug 28, 2024 •

edited

Loading