Skip to content

Conversation

joyjwang
Copy link
Contributor

GODRIVER-3241

Summary

Background & Motivation

@mongodb-drivers-pr-bot mongodb-drivers-pr-bot bot added the review-priority-low Low Priority PR for Review: within 3 business days label Sep 27, 2024
Copy link
Contributor

mongodb-drivers-pr-bot bot commented Sep 27, 2024

API Change Report

./v2/x/mongo/driver/drivertest

compatible changes

MockDeployment: added
MockDescription: added
NewMockDeployment: added

@joyjwang joyjwang marked this pull request as ready for review September 27, 2024 17:54
Copy link
Member

@prestonvasquez prestonvasquez left a comment

Choose a reason for hiding this comment

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

The goal of this ticket is to move the deployment to the experimental package, and use it where needed internally. Allowing external users to mock a server, if they want. See here for a rough POC of the proposal: prestonvasquez@dd57d5b

)

func TestOPMSGMockDeployment(t *testing.T) {
var md *MockDeployment
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Kept the unit test as a mock without actually connecting because the process of connecting with a mock deployment confused me, but if it would be better coverage to actually connect would definitely appreciate guidance

Copy link
Member

@prestonvasquez prestonvasquez Oct 4, 2024

Choose a reason for hiding this comment

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

We should test this using a client. For example:

md := NewMockDeployment()

opts := options.Client()
opts.Opts = append(opts.Opts, func(co *options.ClientOptions) error {
	co.Deployment = md

	return nil
})

client, err := mongo.Connect(opts)
require.NoError(t, err)

md.AddResponses(bson.D{{"ok", 1}})

err = client.Ping(context.Background(), nil)
require.NoError(t, err)

)

func TestOPMSGMockDeployment(t *testing.T) {
var md *MockDeployment
Copy link
Member

@prestonvasquez prestonvasquez Oct 4, 2024

Choose a reason for hiding this comment

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

We should test this using a client. For example:

md := NewMockDeployment()

opts := options.Client()
opts.Opts = append(opts.Opts, func(co *options.ClientOptions) error {
	co.Deployment = md

	return nil
})

client, err := mongo.Connect(opts)
require.NoError(t, err)

md.AddResponses(bson.D{{"ok", 1}})

err = client.Ping(context.Background(), nil)
require.NoError(t, err)

func newMockDeployment(responses ...bson.D) *mockDeployment {
return &mockDeployment{
// NewMockDeployment returns a mock driver.Deployment that responds with OP_MSG wire messages.
func NewMockDeployment(responses ...bson.D) *MockDeployment {
Copy link
Member

Choose a reason for hiding this comment

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

@joyjwang We should note in the comment that for most use cases it's better to test with an actual database.

CC: @matthewdale

Copy link
Member

@prestonvasquez prestonvasquez left a comment

Choose a reason for hiding this comment

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

Appologies for the inconvenience, but could we move opmsg_deploymeng.go and the test to x/mongo/driver/drivertest/? That's where the mock connection API lives, and seems more suitable than x/mongo/driver/integration.

@joyjwang joyjwang merged commit a3eb9a6 into mongodb:master Oct 10, 2024
31 of 33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

review-priority-low Low Priority PR for Review: within 3 business days

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants