Skip to content
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

Updates ion-tests, conformance test runner bugfixes #880

Merged
merged 8 commits into from
Dec 13, 2024
Merged

Conversation

zslayton
Copy link
Contributor

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

Copy link

codecov bot commented Dec 13, 2024

Codecov Report

Attention: Patch coverage is 41.66667% with 14 lines in your changes missing coverage. Please review.

Project coverage is 77.84%. Comparing base (df7ba6c) to head (1c5c6cc).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/element/mod.rs 0.00% 11 Missing ⚠️
src/element/sequence.rs 0.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #880      +/-   ##
==========================================
+ Coverage   77.81%   77.84%   +0.03%     
==========================================
  Files         136      136              
  Lines       34656    34679      +23     
  Branches    34656    34679      +23     
==========================================
+ Hits        26966    26996      +30     
+ Misses       5690     5683       -7     
  Partials     2000     2000              

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

Copy link
Contributor Author

@zslayton zslayton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗺️ PR tour 🧭

Comment on lines +141 to +145
impl From<Sequence> for Vec<Element> {
fn from(value: Sequence) -> Self {
value.elements
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🪧 The test runner converts between Element and its custom data types, which frequently requires this conversion. It's generally useful anyway.

Comment on lines +133 to +141
impl<'a> AnnotationSeq<'a> for &'a Annotations {
fn into_annotations_vec(self) -> AnnotationsVec<'a> {
let mut annotations = AnnotationsVec::new();
for token in self {
annotations.push(token.into());
}
annotations
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🪧 This allows you to call .with_annotations(element.annotations()).

Comment on lines -69 to +75
self.annotations = Some(symbol_ids);
// If this was called with an empty iterator, act as though it was never called at all.
// This prevents writing out an empty annotations sequence in binary, which is illegal.
self.annotations = if !symbol_ids.is_empty() {
Some(symbol_ids)
} else {
None
};
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🪧 This guard rail was already present for value_writer().with_annotations(...), but not for containers like: value_writer().list_writer().with_annotations(...).

Bytes,
Binary,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🪧 Updated per amazon-ion/ion-tests#129.

@@ -2,11 +2,11 @@
//! of the test document when read) and Extensions (clauses that allow the chaining, or
//! permutations for document creation).
use super::*;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🪧 RustRover recently enabled rustfmt by default, which means it did a pass without me knowing it. Unfortunately, from here on there are many superficial rustfmt changes interleaved with a few logic changes.

I'll point out anything substantive -- if there's no comment near it, it's just rustfmt.

Comment on lines -85 to +187
let type_elem = elems.get(1).ok_or(ConformanceErrorKind::ExpectedModelValue)?;
let type_str = type_elem.as_symbol()
.and_then(|s| s.text())
.ok_or(ConformanceErrorKind::ExpectedModelValue)?;
let type_str = match elems.get(1) {
Some(type_element) => type_element
.as_symbol()
.ok_or(ConformanceErrorKind::ExpectedSymbolType)?
.text()
.ok_or(ConformanceErrorKind::ExpectedSymbolType)?,
// If no symbol is specified after `Null`, default to `null` to produce `null.null`.
None => "null",
};
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🪧 This was rejecting (Null) because it required a second child expression. It now accepts (Null) as though it were (Null null), producing null.null.

Comment on lines -169 to +294
let (first, second) = (seq.get(0), seq.get(1));
let field_sym = first.map(SymbolToken::try_from).ok_or(ConformanceErrorKind::ExpectedSymbolType)?.unwrap();
let value = match second.map(|e| e.ion_type()) {
Some(IonType::String) => {
let string = second.unwrap().as_string().unwrap();
ModelValue::String(string.to_string())
}
Some(IonType::Int) => {
let int_val = second.unwrap().as_i64().unwrap();
ModelValue::Int(int_val)
}
Some(IonType::SExp) => {
let seq = second.unwrap().as_sequence().unwrap();
ModelValue::try_from(seq)?
}
_ => return Err(ConformanceErrorKind::ExpectedModelValue),
};

let (first, second) = (seq.get(0).unwrap(), seq.get(1).unwrap());
let field_sym = SymbolToken::try_from(first)?;
let value = ModelValue::try_from(second)?;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🪧 This block was obviated by the new TryFrom implementation.

Comment on lines -264 to +385
let mut is_equal = true;
let mut strukt_iter = strukt.iter();
let mut expected_iter = expected_fields.iter();

while is_equal {
let actual = strukt_iter.next();
let expected = expected_iter.next();

match (actual, expected) {
(Some(actual), Some((expected_field, expected_val))) => {
let actual = actual.expect("unable to read struct field");
let actual_field = actual.raw_name().map(|n| n.read()).expect("unable to get symbolref for field name");
let actual_field = actual_field.expect("unable to read symbolref for field name");

let (expected_txt, expected_id) = match expected_field {
SymbolToken::Text(txt) => (txt.clone(), usize::MAX),
SymbolToken::Offset(id) => (String::from(""), *id as usize),
SymbolToken::Absent(symtab, id) => match ctx.get_symbol_from_table(symtab, *id as usize) {
None => (String::from(""), 0_usize),
Some(shared_symbol) => {
let shared_text = shared_symbol.text().unwrap_or("");
(shared_text.to_string(), other.symbol_table().sid_for(&shared_text).unwrap_or(0))
}
}
};
is_equal = is_equal && actual_field.matches_sid_or_text(expected_id, &expected_txt);

let actual_value = actual.value();

is_equal = is_equal && compare_values(ctx, expected_val, &actual_value)?;
}
(None, None) => break,
_ => is_equal = false,
}
let actual_elem = Element::try_from(actual_struct)?;
let actual_struct = actual_elem.as_struct().unwrap();
if actual_struct.len() != expected_fields.len() {
return Ok(false);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🪧 This replaces the custom struct equality test (in-order field iteration/comparison) with Element equality.

@@ -11,34 +11,6 @@ use std::str::FromStr;
mod implementation {
use super::*;

#[test]
fn test_absent_symbol() {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🪧 I don't believe ion-rust can easily or meaningfully implement this test.

Comment on lines -110 to +88
#[test_resources("ion-tests/conformance/null.ion")]
#[test_resources("ion-tests/conformance/core/typed_null.ion")]
#[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/core/*")]
#[test_resources("ion-tests/conformance/data_model/annotations.ion")]
#[test_resources("ion-tests/conformance/data_model/boolean.ion")]
#[test_resources("ion-tests/conformance/data_model/integer.ion")]
#[test_resources("ion-tests/conformance/data_model/null.ion")]
// No support for half-precision floats yet.
// #[test_resources("ion-tests/conformance/data_model/float.ion")]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🪧 With these changes, the runner now passes all of core and most of data_model. However, there are several more things to fix before it will pass the remaining directories.

@zslayton zslayton marked this pull request as ready for review December 13, 2024 17:49
@zslayton zslayton merged commit d55483e into main Dec 13, 2024
34 of 35 checks passed
@zslayton zslayton deleted the update-tests branch December 13, 2024 18:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants