RUST-656 Run tests against serverless in evergreen #504
Conversation
.evergreen/run-serverless-tests.sh
Outdated
| cargo test ${DEFAULT_FEATURES} --features $FEATURE_FLAGS $1 $OPTIONS | ||
| } | ||
|
|
||
| cargo_test test::spec::crud |
There was a problem hiding this comment.
This is the subset of tests listed by the spec; running the full suite had a lot of noisy errors (unsupported operations, failures due to restrictions, db cleanup issues) that didn't seem worth individually chasing down.
| .get_or_insert_with(Default::default) | ||
| .push(BulkWriteError { index, ..err }); | ||
| Err(e) => { | ||
| let labels = e.labels().clone(); |
There was a problem hiding this comment.
Github diff is misleading here - the only change is cloning the labels before the match to accommodate the labels field no longer being visible.
| @@ -1190,6 +1190,9 @@ impl ClientOptions { | |||
| /// Applies the options in other to these options if a value is not already present | |||
| #[cfg(test)] | |||
| pub(crate) fn merge(&mut self, other: ClientOptions) { | |||
| if self.hosts.is_empty() { | |||
There was a problem hiding this comment.
I'm not super thrilled about the behavior here but I can't think of a clean solution.
TestClientandEventClienthave awith_additional_optionsconstructor fn that, semantically, takes a passed-inClientOptionsand merges in the set of "test-default" options there for any unset fields, with preference given to the passed-in fields where both were set.- Prior to this change,
hostswas not included in the merge, so the passed-inClientOptionswould always determinehosts. This caused thelabel_not_added_first_read_error/label_not_added_second_read_errortests to break on serverless because those tests were not settinghostsand the default value was not valid in those cases. - Ideally, the semantics for
hostswould be the same as for the rest of the fields - if unset, it would use the "test default". However, because it's populated in the builder by default, there's no way to actually determine "if unset"; testing against the default value causes other tests to fail because those are specifically constructing a hosts vec that, coincidentally, is equal to the default value, and should not be overridden by the test default. - Again ideally, this could be fixed by changing
hoststo anOption<Vec<_>>or not populating it by default sovec[]would be a reasonable signal for "unset", but either of those would be a breaking change. - Calling
with_additional_optionswith aClientOptionsconstructed via builder with the default hosts is a nasty pitfall that's easy to hit - from the call site it looks like you'll be constructing a client with only the options specified, and the defaulthostsvalue will coincidentally work in most but not all testing environments. - Existing tests with the exception of the two that broke avoid this by either explicitly populating
hostsor copying theClientOptionsfrom theCLIENT_OPTIONSvalue.
src/error.rs
Outdated
|
|
||
| /// Labels categorizing the error. | ||
| #[serde(rename = "errorLabels", default)] | ||
| pub labels: Vec<String>, |
There was a problem hiding this comment.
Error labels are only ever actually returned at the top level of an error response, and it's merely a quirk / bug in the spec tests that they appear in the write concern error. @isabelatkinson actually just put up a PR to fix that: mongodb/specifications#1086.
For that reason, I don't think we should include public API support for this case, and if need be we can just skip the affected tests until the PR is merged. I think I remember you saying we may have to wait for a serverless change on this too to support the labels being specified in the correct place.
There was a problem hiding this comment.
The serverless proxy apparently enforces that they appear here; I've removed this field from the public API but I'd prefer to leave it and the related logic in place so the tests can continue to pass.
There was a problem hiding this comment.
have we confirmed that this behavior is correct/desired on the serverless end? it seems like a weird inconsistency. I think it's fine to have it within the driver right now so that we can run the tests but in the long run it would be nice not to have to maintain a special case like this
There was a problem hiding this comment.
I'm in the process of following up about this on the serverless side - it looks like this isn't expected behavior. I'll make sure to send a follow-up PR for the driver (even if it's just a TODO with a bug reference) once that gets sorted out.
There was a problem hiding this comment.
SGTM to leave this in temporarily until serverless fixes the issue on their end.
There was a problem hiding this comment.
I've added todos here referencing CLOUDP-105256.
abr-egn
force-pushed
the
RUST-656/serverless-test
branch
from
eb834f7
to
0715c6b
Compare
src/client/options/mod.rs
Outdated
| @@ -1232,6 +1236,10 @@ impl ClientOptions { | |||
| original_uri | |||
| ] | |||
| ); | |||
| #[cfg(test)] | |||
There was a problem hiding this comment.
Do we need #[cfg(test)] here? The method already has it
| @@ -179,6 +179,8 @@ async fn run_spec_tests() { | |||
| .await | |||
| .unwrap(); | |||
| } | |||
|
|
|||
| let _ = client.database(&db_name).drop(None).await; | |||
There was a problem hiding this comment.
I think we should unwrap here to make sure this doesn't silently fail
abr-egn
force-pushed
the
RUST-656/serverless-test
branch
from
62d8f8f
to
932207f
Compare
abr-egn
force-pushed
the
RUST-656/serverless-test
branch
from
932207f
to
2682541
Compare
RUST-656
This adds evergreen configuration to run a subset of tests on serverless, and fixes various test runner bugs that were preventing those tests from passing.
This also closes RUST-983.