Skip to content

Conversation

@prestonvasquez
Copy link
Member

@prestonvasquez prestonvasquez commented Apr 2, 2025

GODRIVER-3445

This PR suggested using git submodules to avoid manually syncing unified spec tests with the MongoDB specifications.

When using for the first time, users must initialize the submodule:

git submodule update --init

And then switching to branches that do not have the spec submodule:

git submodule deinit -f -- testdata/specifications
rm -rf testdata/specifications
git rm --cached testdata/specifications

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

API Change Report

No changes found!

@prestonvasquez prestonvasquez marked this pull request as ready for review April 2, 2025 02:27
@prestonvasquez prestonvasquez changed the title Godriver 3445 GODRIVER-3445 Use Git submodules for spec tests Apr 2, 2025

// run a CRUD operation and verify errors and outcomes.
// the test description is needed to see determine if the test is an aggregate with $out
func runCrudOperation(mt *mtest.T, testDescription string, operation crudOperation, outcome crudOutcome) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

All additional code in retryable_writes_prose_test.go was copied from crud_spec_test.go, which was removed because it's redundant with the new CRUD unified spec tests.

Comment on lines 26 to 27
// TODO: create new directory for mongocrypt
const resourcesDir = "../../../../testdata/mongocrypt"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this TODO still relevant?

case "maxconnecting":
require.Equal(t, value, float64(cs.MaxConnecting))
default:
fmt.Println(key)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove println.

Suggested change
fmt.Println(key)

// Test case for all connection string spec tests.
func TestAuthSpec(t *testing.T) {
for _, file := range spectest.FindJSONFilesInDir(t, authTestsDir) {
fmt.Println("file", file)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove println.

Suggested change
fmt.Println("file", file)

}

func runConnectionStringTest(t *testing.T, test connectionStringTest) {
if test.SkipReason != "" {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we also remove the SkipReason field from the connectionStringTest struct? As far as I can tell, the connection-string test spec don't include that field.

params:
working_dir: "src/go.mongodb.org/mongo-driver"
script: |
git submodule update --init
Copy link
Collaborator

Choose a reason for hiding this comment

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

Optional: Consider using the init-submodule task instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

This solution doesn't work for windows for some reason 🤷

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's weird. Oh well, 🤷

Copy link
Collaborator

@matthewdale matthewdale 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! 👍

@prestonvasquez prestonvasquez merged commit 31ef273 into mongodb:master Apr 3, 2025
28 of 34 checks passed
@prestonvasquez prestonvasquez deleted the GODRIVER-3445 branch April 3, 2025 16:14
@prestonvasquez prestonvasquez restored the GODRIVER-3445 branch April 10, 2025 20:24
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