Skip to content

Convert all tests in integration to use the testify suite package#973

Open
autarch wants to merge 1 commit intomasterfrom
03-31-convert_all_tests_in_integration_to_use_the_testify_suite_package
Open

Convert all tests in integration to use the testify suite package#973
autarch wants to merge 1 commit intomasterfrom
03-31-convert_all_tests_in_integration_to_use_the_testify_suite_package

Conversation

@autarch
Copy link
Copy Markdown
Collaborator

@autarch autarch commented Mar 31, 2026

No description provided.

@autarch Graphite App
Copy link
Copy Markdown
Collaborator Author

autarch commented Mar 31, 2026 •
edited
Loading

This stack of pull requests is managed by Graphite. Learn more about stacking.

@autarch autarch mentioned this pull request Mar 31, 2026
Merged
@autarch autarch changed the base branch from 03-30-move_all_existing_round_trip_go_tests_to_integration_ to graphite-base/973 March 31, 2026 16:37
@autarch autarch force-pushed the graphite-base/973 branch from 6c44f53 to efb6f41 Compare April 1, 2026 16:32
@autarch autarch force-pushed the 03-31-convert_all_tests_in_integration_to_use_the_testify_suite_package branch from 86aad88 to 721a4f6 Compare April 1, 2026 16:32
@autarch autarch changed the base branch from graphite-base/973 to 03-30-move_all_existing_round_trip_go_tests_to_integration_ April 1, 2026 16:32
This was referenced Apr 1, 2026
Merged
Merged
Merged
Merged
Draft
Open
@autarch autarch force-pushed the 03-31-convert_all_tests_in_integration_to_use_the_testify_suite_package branch from 721a4f6 to 5f9bf57 Compare April 1, 2026 18:33
@autarch autarch changed the base branch from 03-30-move_all_existing_round_trip_go_tests_to_integration_ to graphite-base/973 April 1, 2026 19:30
@autarch autarch force-pushed the 03-31-convert_all_tests_in_integration_to_use_the_testify_suite_package branch from 5f9bf57 to 13ba381 Compare April 1, 2026 19:31
@autarch autarch force-pushed the graphite-base/973 branch from efb6f41 to 53ae805 Compare April 1, 2026 19:31
@graphite-app graphite-app bot changed the base branch from graphite-base/973 to master April 1, 2026 19:32
@autarch autarch force-pushed the 03-31-convert_all_tests_in_integration_to_use_the_testify_suite_package branch 5 times, most recently from cc948a7 to 323143c Compare April 1, 2026 20:34
@autarch autarch mentioned this pull request Apr 1, 2026
Draft
@autarch autarch requested a review from mmcclimon April 1, 2026 21:08
@autarch autarch marked this pull request as ready for review April 1, 2026 21:09
@autarch autarch requested a review from a team as a code owner April 1, 2026 21:09
mmcclimon
mmcclimon requested changes Apr 2, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@mmcclimon mmcclimon left a comment

Choose a reason for hiding this comment

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

I didn't read everything closely here because I noticed enough on the skim that I think we should take another run at this one. Probably you could catch nearly everything by searching for s.T() and removing nearly all of them. (It's probably totally reasonable to make s.Context() which is just a wrapper for s.T().Context(), for example.)

integration/dumprestore/roundtrip_test.go Outdated
func(t *testing.T) {
testDumpAndRestoreConfigDBIncludesAllCollections(t)
func() {
testDumpAndRestoreConfigDBIncludesAllCollections(s.T())
Copy link
Copy Markdown
Contributor

@mmcclimon mmcclimon Apr 2, 2026

Choose a reason for hiding this comment

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

Anywhere in here we're passing around s.T() these should probably just be methods on the suite instead.

Copy link
Copy Markdown
Collaborator Author

@autarch autarch Apr 3, 2026

Choose a reason for hiding this comment

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

This should be fixed now.

integration/dumprestore/roundtrip_test.go Outdated
Comment on lines +504 to +505
assert.Equal(
t,
s.T(),
Copy link
Copy Markdown
Contributor

@mmcclimon mmcclimon Apr 2, 2026

Choose a reason for hiding this comment

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

This should be s.Assert().Equal(...) (I suspect there are more of these hiding around.)

Copy link
Copy Markdown
Collaborator Author

@autarch autarch Apr 3, 2026

Choose a reason for hiding this comment

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

Fixed.

integration/dumprestore/roundtrip_test.go Outdated
err = testDB.Drop(ctx)
if err != nil {
t.Fatalf("Failed to drop test database: %v", err)
s.T().Fatalf("Failed to drop test database: %v", err)
Copy link
Copy Markdown
Contributor

@mmcclimon mmcclimon Apr 2, 2026

Choose a reason for hiding this comment

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

This can just be s.Require.NoError(err).

Copy link
Copy Markdown
Collaborator Author

@autarch autarch Apr 3, 2026

Choose a reason for hiding this comment

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

Fixed.

integration/importexport/csv_test.go Outdated
assert.EqualValues(t, 0, n, "c=3 should not have been exported (not in fieldFile)")
n, err := dest.CountDocuments(s.T().Context(), bson.D{{"a", 1}})
s.Require().NoError(err)
s.EqualValues(3, n, "3 documents should have a=1")
Copy link
Copy Markdown
Contributor

@mmcclimon mmcclimon Apr 2, 2026

Choose a reason for hiding this comment

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

I don't know if the tools officially have the same style guide mongosync does, but

  • these should be s.Assert().EqualValues(...), and
  • we should add the testifylint linter in this repo to catch this stuff.

Copy link
Copy Markdown
Collaborator Author

@autarch autarch Apr 3, 2026

Choose a reason for hiding this comment

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

There's really no style guide for the tools. We should probably copy the one from mongosync. I will make a PR.

Copy link
Copy Markdown
Collaborator Author

@autarch autarch Apr 3, 2026

Choose a reason for hiding this comment

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

Actually, I take it back. In the Mongosync coding standards docs, we already say this applied to the DB Tools.

@autarch autarch force-pushed the 03-31-convert_all_tests_in_integration_to_use_the_testify_suite_package branch from 323143c to e8844d7 Compare April 3, 2026 22:18
@autarch Graphite App
Copy link
Copy Markdown
Collaborator Author

autarch commented Apr 3, 2026

I added an s.Contextmethod. At this point, there are only a few s.T()calls left in the integration tests, and they all seem reasonable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mmcclimon mmcclimon mmcclimon requested changes

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@autarch @mmcclimon