Skip to content

RUST-523 Support change streams in unified spec tests #564

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 10 commits into from
Jan 27, 2022

Conversation

abr-egn
Copy link
Contributor

@abr-egn abr-egn commented Jan 25, 2022

RUST-523

This pulls in the unified spec test for change streams and adds support to the Rust unified runner for creating and iterating change streams. I also added more detailed mismatch reporting to the runner as part of debugging initial failures.

Copy link
Contributor

@patrickfreed patrickfreed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good! I like the added detail to the matching. Only have a few minor questions/suggestions

@abr-egn abr-egn requested a review from patrickfreed January 27, 2022 14:22
Copy link
Contributor

@isabelatkinson isabelatkinson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good! nice to see some upgrades to the matcher

@@ -44,14 +45,21 @@ pub struct ClientEntity {
observe_sensitive_commands: bool,
}

impl ClientEntity {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this method is necessary -- ClientEntity implements Deref with a target type of Client so you should be able to call watch directly on a ClientEntity (where this method is used).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I missed that, thank you! Removed.

Copy link
Contributor

@patrickfreed patrickfreed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM mod Isabel's comment

Copy link
Contributor

@isabelatkinson isabelatkinson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm! looks like there's something going on with evergreen but i don't think we need another full CI run here

@abr-egn abr-egn merged commit 724a227 into mongodb:master Jan 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants