RUST-978 Unified test format additions in support of load balanced tests #465
Conversation
| @@ -343,39 +309,65 @@ impl TestOperation for DeleteOne { | |||
| } | |||
|
|
|||
| #[derive(Debug, Default, Deserialize)] | |||
| #[serde(rename_all = "camelCase", deny_unknown_fields)] | |||
There was a problem hiding this comment.
Removing deny_unknown_fields is unfortunate but this was already in an unsupported situation by using it with a flattened field, and outright broke when it was itself used as such.
There was a problem hiding this comment.
technically it's a requirement of the unified test format that we error on unrecognized fields, though it would be quite a bit of boilerplate to adhere to that here, so not sure if it's worth it or not.
There was a problem hiding this comment.
I'd personally go with "not" - I think not having to deal with keeping multiple duplicate data structures in sync is the lesser evil here - but I don't know what precedent is like.
There was a problem hiding this comment.
We do currently do this for the Find operation in the command monitoring tests. One way we could ensure this stays in sync is to avoid using the builder pattern when constructing the FindOptions and instead use the struct literal without using ..Default::default(). I believe we can do this even though the struct is #[non_exhaustive] because it's a type we defined. This way, if we add a new option to FindOptions, the tests won't compile until we account for it in the conversion. Would still be a bit of boilerplate, but it's worth it IMO.
I do agree that if we had to manually keep the two in sync, it would probably be better to just use FindOptions directly here though. However, given that we can do some tricks to guarantee they're in sync at compile time, I lean towards duplication personally.
| @@ -343,39 +309,65 @@ impl TestOperation for DeleteOne { | |||
| } | |||
|
|
|||
| #[derive(Debug, Default, Deserialize)] | |||
| #[serde(rename_all = "camelCase", deny_unknown_fields)] | |||
There was a problem hiding this comment.
technically it's a requirement of the unified test format that we error on unrecognized fields, though it would be quite a bit of boilerplate to adhere to that here, so not sure if it's worth it or not.
abr-egn
force-pushed
the
RUST-978/lb-unified-test-changees
branch
from
80e3ca2 to
a4f16ba
Compare
RUST-978
This adds several new constructs to the unified test format (and tests for those) that will be used for the load balancer unified tests. This also covers RUST-984.