RUST-1387 Execute CSFLE unified spec tests #770
Conversation
abr-egn
requested review from
a team and
kmahar
and removed request for
a team
|
LGTM mod the outstanding namespace thread so tagging everyone else in! |
| @@ -271,9 +277,30 @@ pub struct DataKeyOptions { | |||
| pub key_material: Option<Vec<u8>>, | |||
| } | |||
|
|
|||
| impl<'de> Deserialize<'de> for DataKeyOptions { | |||
There was a problem hiding this comment.
Is this implementation only used for the spec tests? If so, I think we should move it into the test code per this discussion.
There was a problem hiding this comment.
Done. Also moved the csfle operation definitions to their own submodule to reduce the need for #[cfg(feature = "csfle")] spam.
| @@ -202,6 +204,11 @@ impl ClientEntity { | |||
| pub(crate) async fn sync_workers(&self) { | |||
| self.client.sync_workers().await; | |||
| } | |||
|
|
|||
| #[allow(dead_code)] | |||
There was a problem hiding this comment.
nit: does this have the same effect as #[cfg(feature = "csfle")]? if so I think it's a bit more clear to gate this function behind the feature flag than to allow dead code, that way if it's no longer needed for the csfle tests we know to remove it
| @@ -133,6 +134,11 @@ impl RunOnRequirement { | |||
| return false; | |||
| } | |||
| } | |||
| if let Some(csfle) = &self.csfle { | |||
| if *csfle && std::env::var("KMS_PROVIDERS").is_err() { | |||
There was a problem hiding this comment.
if csfle is set in the test file, does that mean that KMS_PROVIDERS should be set? or is it valid for one of these values to be set but not the other? if it's the former, perhaps we should panic or log some kind of message here to avoid skipping CSFLE tests silently
There was a problem hiding this comment.
This returning false translates into the normal "client topology not compatible with test" message, so it's not quite silent, but that's an increasingly unhelpful message so I augmented this to return details.
abr-egn
force-pushed
the
RUST-1387/fle-spec
branch
from
762360e to
c82ae15
Compare
RUST-1387
This updates the unified runner to support csfle tests (mostly just new operations). This PR also pulls in the legacy test files - nothing uses those yet but there didn't seem a reason not to.