RUST-670 Expect unified test format operations to succeed #388
Conversation
| #[serde(deserialize_with = "crate::bson_util::deserialize_u64_from_bson_number")] | ||
| #[serde( | ||
| deserialize_with = "crate::bson_util::deserialize_u64_from_bson_number", | ||
| serialize_with = "crate::bson::serde_helpers::serialize_u64_as_i64" |
There was a problem hiding this comment.
This addition was necessary because the results from test operations are all serialized into Bson.
| "Running tests from {}", | ||
| test_file_full_path.display().to_string() | ||
| ); | ||
| pub(crate) async fn run_single_test<T, F, G>(path: PathBuf, run_test_file: &F) |
There was a problem hiding this comment.
The valid_fail tests need to be able to call this method directly to ensure that each test file in the directory fails; just calling run_spec_test will panic on the first test file and return.
| ) | ||
| }) | ||
| .unwrap_or_else(|| { | ||
| if let Some(expect_error) = operation.expect_error { |
There was a problem hiding this comment.
The only change here is refactoring the conditionals to unwrap the result if expect_error is not present rather than just when expect_result is present.
| let path = path.join(&test_file_path); | ||
| let path_display = path.display().to_string(); | ||
|
|
||
| std::panic::AssertUnwindSafe(run_single_test(path, &run_unified_format_test)) |
There was a problem hiding this comment.
My original plan for this work was to return errors directly from the unified test runner code, but given the amount of panics and unwraps in our tests this led to excessive verbosity. Instead I opted for catch_unwind, which catches panics and converts them into a Result that can be examined. AssertUnwindSafe is essentially a wrapper that suppresses a compiler error that would otherwise appear indicating that the future returned from run_single_test cannot be unwound safely. I'd be hesitant to use this in driver code given that it bypasses some compiler safety, but because this is just test code this feels like the best solution that doesn't over-complicate the test runner itself.
| @@ -246,7 +246,7 @@ impl Deref for Operation { | |||
| pub(super) struct DeleteMany { | |||
| filter: Document, | |||
| #[serde(flatten)] | |||
| options: Option<DeleteOptions>, | |||
| options: DeleteOptions, | |||
There was a problem hiding this comment.
The combination of #[serde(flatten)] and wrapping these fields in an Option was silencing certain deserialization errors.
| @@ -509,7 +509,7 @@ pub struct CountOptions { | |||
| /// | |||
| /// This options maps to the `maxTimeMS` MongoDB query option, so the duration will be sent | |||
| /// across the wire as an integer number of milliseconds. | |||
| #[serde(deserialize_with = "deserialize_duration_from_u64_millis")] | |||
| #[serde(default, deserialize_with = "deserialize_duration_from_u64_millis")] | |||
There was a problem hiding this comment.
Is there a reason these are the only fields on which you've applied the default attribute?
There was a problem hiding this comment.
default is needed when deserialize_with is being used to default the field to None if it isn't specified. I added default to these fields specifically because they weren't properly deserializing from the test files.
There was a problem hiding this comment.
Makes sense. Does it make more sense to include default as a container attribute instead now that the options structs aren't Options? I guess every field is optional so it doesn't matter much either way.
There was a problem hiding this comment.
Yeah it shouldn't matter, default is only really necessary when we're overriding the default deserialization behavior with deserialize_with because the method provided there doesn't handle Options.
Finished up this GBF ticket as I had a small amount of work left and will be out next Friday.
This PR adds validation that the
valid_failtests in the Unified Test Format specification fail upon error.