Skip to content
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

Add RewrapManyDataKeyProseTest #1083

Merged
merged 2 commits into from
Feb 13, 2023
Merged

Conversation

katcharov
Copy link
Contributor

Copy link
Member

@rozza rozza left a comment

Choose a reason for hiding this comment

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

Couple of minor naming changes, to follow current conventions.

Other than that LGTM

Note: I've added JAVA-4879 to track improving the discoverability of prose tests and to define a clearer naming convention / grouping. As I'm not sure all prose tests have been implemented.

import static com.mongodb.client.Fixture.getMongoClientSettingsBuilder;
import static org.junit.jupiter.api.Assertions.assertEquals;

public abstract class AbstractRewrapManyDataKeyProseTest {
Copy link
Member

Choose a reason for hiding this comment

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

Rename to AbstractClientEncryptionRewrapManyDataKeyProseTest to aid future discovery

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Many tests are named AbstractClientSideEncryption... - is this just a minor inconsistency, or is there some difference?

Copy link
Member

Choose a reason for hiding this comment

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

This is the weirdness that I hope JAVA-4879 will clean up.

Probably a mix of inconsistent naming and grouping for the CSFLE Prose tests - its not clear which ones map to which prose tests.

import static com.mongodb.client.Fixture.getMongoClient;
import static com.mongodb.client.Fixture.getMongoClientSettingsBuilder;
import static org.junit.jupiter.api.Assertions.assertEquals;

Copy link
Member

Choose a reason for hiding this comment

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

Lets add a comment to the prose test in the spec

import com.mongodb.client.vault.ClientEncryptions;
import com.mongodb.reactivestreams.client.syncadapter.SyncMongoClient;

public class RewrapManyDataKeyProseTest extends AbstractRewrapManyDataKeyProseTest {
Copy link
Member

Choose a reason for hiding this comment

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

Rename to ClientEncryptionRewrapManyDataKeyProseTest to aid future discovery

@katcharov katcharov marked this pull request as ready for review February 13, 2023 15:17
@katcharov katcharov requested a review from rozza February 13, 2023 15:18
Copy link
Member

@rozza rozza left a comment

Choose a reason for hiding this comment

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

LGTM

@katcharov katcharov merged commit d8b17c6 into mongodb:master Feb 13, 2023
@katcharov katcharov deleted the JAVA-4698 branch February 13, 2023 15:58
ispringer pushed a commit to evergage/mongo-java-driver that referenced this pull request May 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants