RUST-967 Fold modifiers subfield into parent struct (#459)

Author: abr-egn

src/cmap/test/mod.rs

@@ -20,8 +20,10 @@ use crate::{
     sdam::{ServerUpdate, ServerUpdateSender},
     test::{
         assert_matches,
+        eq_matches,
         run_spec_test,
         EventClient,
+        MatchErrExt,
         Matchable,
         CLIENT_OPTIONS,
         LOCK,
@@ -321,42 +323,51 @@ impl Operation {
 }
 
 impl Matchable for TlsOptions {
-    fn content_matches(&self, expected: &TlsOptions) -> bool {
+    fn content_matches(&self, expected: &TlsOptions) -> std::result::Result<(), String> {
         self.allow_invalid_certificates
             .matches(&expected.allow_invalid_certificates)
-            && self
-                .ca_file_path
-                .as_ref()
-                .map(|pb| pb.display().to_string())
-                .matches(
-                    &expected
-                        .ca_file_path
-                        .as_ref()
-                        .map(|pb| pb.display().to_string()),
-                )
-            && self
-                .cert_key_file_path
-                .as_ref()
-                .map(|pb| pb.display().to_string())
-                .matches(
-                    &expected
-                        .cert_key_file_path
-                        .as_ref()
-                        .map(|pb| pb.display().to_string()),
-                )
+            .prefix("allow_invalid_certificates")?;
+        self.ca_file_path
+            .as_ref()
+            .map(|pb| pb.display().to_string())
+            .matches(
+                &expected
+                    .ca_file_path
+                    .as_ref()
+                    .map(|pb| pb.display().to_string()),
+            )
+            .prefix("ca_file_path")?;
+        self.cert_key_file_path
+            .as_ref()
+            .map(|pb| pb.display().to_string())
+            .matches(
+                &expected
+                    .cert_key_file_path
+                    .as_ref()
+                    .map(|pb| pb.display().to_string()),
+            )
+            .prefix("cert_key_file_path")?;
+        Ok(())
     }
 }
 
 impl Matchable for EventOptions {
-    fn content_matches(&self, expected: &EventOptions) -> bool {
-        self.max_idle_time.matches(&expected.max_idle_time)
-            && self.max_pool_size.matches(&expected.max_pool_size)
-            && self.min_pool_size.matches(&expected.min_pool_size)
+    fn content_matches(&self, expected: &EventOptions) -> std::result::Result<(), String> {
+        self.max_idle_time
+            .matches(&expected.max_idle_time)
+            .prefix("max_idle_time")?;
+        self.max_pool_size
+            .matches(&expected.max_pool_size)
+            .prefix("max_pool_size")?;
+        self.min_pool_size
+            .matches(&expected.min_pool_size)
+            .prefix("min_pool_size")?;
+        Ok(())
     }
 }
 
 impl Matchable for Event {
-    fn content_matches(&self, expected: &Event) -> bool {
+    fn content_matches(&self, expected: &Event) -> std::result::Result<(), String> {
         match (self, expected) {
             (Event::PoolCreated(actual), Event::PoolCreated(ref expected)) => {
                 actual.options.matches(&expected.options)
@@ -368,8 +379,12 @@ impl Matchable for Event {
                 actual.connection_id.matches(&expected.connection_id)
             }
             (Event::ConnectionClosed(actual), Event::ConnectionClosed(ref expected)) => {
-                actual.reason == expected.reason
-                    && actual.connection_id.matches(&expected.connection_id)
+                eq_matches("reason", &actual.reason, &expected.reason)?;
+                actual
+                    .connection_id
+                    .matches(&expected.connection_id)
+                    .prefix("connection_id")?;
+                Ok(())
             }
             (Event::ConnectionCheckedOut(actual), Event::ConnectionCheckedOut(ref expected)) => {
                 actual.connection_id.matches(&expected.connection_id)
@@ -380,12 +395,21 @@ impl Matchable for Event {
             (
                 Event::ConnectionCheckOutFailed(actual),
                 Event::ConnectionCheckOutFailed(ref expected),
-            ) => actual.reason == expected.reason,
-            (Event::ConnectionCheckOutStarted(_), Event::ConnectionCheckOutStarted(_)) => true,
-            (Event::PoolCleared(_), Event::PoolCleared(_)) => true,
-            (Event::PoolReady(_), Event::PoolReady(_)) => true,
-            (Event::PoolClosed(_), Event::PoolClosed(_)) => true,
-            _ => false,
+            ) => {
+                if actual.reason == expected.reason {
+                    Ok(())
+                } else {
+                    Err(format!(
+                        "expected reason {:?}, got {:?}",
+                        expected.reason, actual.reason
+                    ))
+                }
+            }
+            (Event::ConnectionCheckOutStarted(_), Event::ConnectionCheckOutStarted(_)) => Ok(()),
+            (Event::PoolCleared(_), Event::PoolCleared(_)) => Ok(()),
+            (Event::PoolReady(_), Event::PoolReady(_)) => Ok(()),
+            (Event::PoolClosed(_), Event::PoolClosed(_)) => Ok(()),
+            (actual, expected) => Err(format!("expected event {:?}, got {:?}", actual, expected)),
         }
     }
 }

src/test/mod.rs

@@ -15,6 +15,7 @@ pub(crate) use self::{
     spec::{run_single_test, run_spec_test, run_spec_test_with_path, RunOn, Serverless, Topology},
     util::{
         assert_matches,
+        eq_matches,
         CmapEvent,
         CommandEvent,
         Event,
@@ -23,6 +24,7 @@ pub(crate) use self::{
         FailCommandOptions,
         FailPoint,
         FailPointMode,
+        MatchErrExt,
         Matchable,
         SdamEvent,
         TestClient,

src/test/spec/command_monitoring/event.rs

@@ -2,7 +2,7 @@ use serde::Deserialize;
 
 use crate::{
     bson::Document,
-    test::{CommandEvent, Matchable},
+    test::{eq_matches, CommandEvent, MatchErrExt, Matchable},
 };
 
 #[derive(Debug, Deserialize, PartialEq)]
@@ -23,7 +23,7 @@ pub enum TestEvent {
 }
 
 impl Matchable for TestEvent {
-    fn content_matches(&self, actual: &TestEvent) -> bool {
+    fn content_matches(&self, actual: &TestEvent) -> Result<(), String> {
         match (self, actual) {
             (
                 TestEvent::Started {
@@ -37,9 +37,14 @@ impl Matchable for TestEvent {
                     command: expected_command,
                 },
             ) => {
-                actual_command_name == expected_command_name
-                    && actual_database_name == expected_database_name
-                    && actual_command.matches(expected_command)
+                eq_matches("command_name", actual_command_name, expected_command_name)?;
+                eq_matches(
+                    "database_name",
+                    actual_database_name,
+                    expected_database_name,
+                )?;
+                actual_command.matches(expected_command).prefix("command")?;
+                Ok(())
             }
             (
                 TestEvent::Succeeded {
@@ -51,7 +56,9 @@ impl Matchable for TestEvent {
                     reply: expected_reply,
                 },
             ) => {
-                actual_command_name == expected_command_name && actual_reply.matches(expected_reply)
+                eq_matches("command_name", actual_command_name, expected_command_name)?;
+                actual_reply.matches(expected_reply).prefix("reply")?;
+                Ok(())
             }
             (
                 TestEvent::Failed {
@@ -60,8 +67,8 @@ impl Matchable for TestEvent {
                 TestEvent::Failed {
                     command_name: expected_command_name,
                 },
-            ) => actual_command_name == expected_command_name,
-            _ => false,
+            ) => eq_matches("command_name", actual_command_name, expected_command_name),
+            _ => Err(format!("expected event {:?}, got {:?}", self, actual)),
         }
     }
 }

src/test/spec/command_monitoring/operation.rs

@@ -110,43 +110,8 @@ impl TestOperation for DeleteOne {
     }
 }
 
-// This struct is necessary because the command monitoring tests specify the options in a very old
-// way (SPEC-1519).
-#[derive(Debug, Deserialize, Default)]
-struct FindModifiers {
-    #[serde(rename = "$comment", default)]
-    comment: Option<String>,
-    #[serde(rename = "$hint", default)]
-    hint: Option<Hint>,
-    #[serde(
-        rename = "$maxTimeMS",
-        deserialize_with = "bson_util::deserialize_duration_option_from_u64_millis",
-        default
-    )]
-    max_time: Option<Duration>,
-    #[serde(rename = "$min", default)]
-    min: Option<Document>,
-    #[serde(rename = "$max", default)]
-    max: Option<Document>,
-    #[serde(rename = "$returnKey", default)]
-    return_key: Option<bool>,
-    #[serde(rename = "$showDiskLoc", default)]
-    show_disk_loc: Option<bool>,
-}
-
-impl FindModifiers {
-    fn update_options(&self, options: &mut FindOptions) {
-        options.comment = self.comment.clone();
-        options.hint = self.hint.clone();
-        options.max_time = self.max_time;
-        options.min = self.min.clone();
-        options.max = self.max.clone();
-        options.return_key = self.return_key;
-        options.show_record_id = self.show_disk_loc;
-    }
-}
-
 #[derive(Debug, Default, Deserialize)]
+#[serde(deny_unknown_fields)]
 pub(super) struct Find {
     filter: Option<Document>,
     #[serde(default)]
@@ -158,7 +123,23 @@ pub(super) struct Find {
     #[serde(default)]
     limit: Option<i64>,
     #[serde(default)]
-    modifiers: Option<FindModifiers>,
+    comment: Option<String>,
+    #[serde(default)]
+    hint: Option<Hint>,
+    #[serde(
+        rename = "maxTimeMS",
+        deserialize_with = "bson_util::deserialize_duration_option_from_u64_millis",
+        default
+    )]
+    max_time: Option<Duration>,
+    #[serde(default)]
+    min: Option<Document>,
+    #[serde(default)]
+    max: Option<Document>,
+    #[serde(rename = "returnKey", default)]
+    return_key: Option<bool>,
+    #[serde(rename = "showRecordId", default)]
+    show_record_id: Option<bool>,
 }
 
 impl TestOperation for Find {
@@ -168,18 +149,21 @@ impl TestOperation for Find {
 
     fn execute(&self, collection: Collection<Document>) -> BoxFuture<Result<()>> {
         async move {
-            let mut options = FindOptions {
+            let options = FindOptions {
                 sort: self.sort.clone(),
                 skip: self.skip,
                 batch_size: self.batch_size.map(|i| i as u32),
                 limit: self.limit,
+                comment: self.comment.clone(),
+                hint: self.hint.clone(),
+                max_time: self.max_time,
+                min: self.min.clone(),
+                max: self.max.clone(),
+                return_key: self.return_key,
+                show_record_id: self.show_record_id,
                 ..Default::default()
             };
 
-            if let Some(ref modifiers) = self.modifiers {
-                modifiers.update_options(&mut options);
-            }
-
             let mut cursor = collection.find(self.filter.clone(), options).await?;
 
             while let Some(result) = cursor.next().await {

src/test/spec/json/command-monitoring/legacy/find.json

@@ -89,21 +89,19 @@
           "skip": {
             "$numberLong": "2"
           },
-          "modifiers": {
-            "$comment": "test",
-            "$hint": {
-              "_id": 1
-            },
-            "$max": {
-              "_id": 6
-            },
-            "$maxTimeMS": 6000,
-            "$min": {
-              "_id": 0
-            },
-            "$returnKey": false,
-            "$showDiskLoc": false
-          }
+          "comment": "test",
+          "hint": {
+            "_id": 1
+          },
+          "max": {
+            "_id": 6
+          },
+          "maxTimeMS": 6000,
+          "min": {
+            "_id": 0
+          },
+          "returnKey": false,
+          "showRecordId": false
         }
       },
       "expectations": [

src/test/spec/v2_runner/test_event.rs

@@ -36,7 +36,7 @@ impl CommandStartedEvent {
             }
         }
 
-        self.command.content_matches(&expected)
+        self.command.content_matches(&expected).is_ok()
     }
 }