Skip to content

POC subtestWrapper #885

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

Closed
wants to merge 1 commit into from
Closed

POC subtestWrapper #885

wants to merge 1 commit into from

Conversation

prestonvasquez
Copy link
Member

@prestonvasquez prestonvasquez commented Mar 22, 2022

GODRIVER-2310

Create a subtest wrapper for the documentation example tests, specifically for use in the TestDocumentationExamples test function. This will help in testing individual functions within the TestDocumentationExamples test without decoupling the underlying functionality. For example, using the proposed refactor the following command runs only the InsertExamples subtest

go test -run TestDocumentationExamples/InsertExamples

This will also help with debugging in text editors, such as VS Code, where you can use configurations that launches only on specific documentation example subtests

// example vs code test configuration
{
    "name": "Launch Example Subtest",
    "type": "go",
    "request": "launch",
    "mode": "test",
    "program": "${fileDirname}",
    "args": ["-test.run", "TestDocumentationExamples/InsertExamples"]
}

In VS Code, there is no existing inline subtest selection.

@prestonvasquez prestonvasquez changed the title subtestWrapper draft POC subtestWrapper Mar 22, 2022
Copy link
Contributor

@benjirewis benjirewis left a comment

Choose a reason for hiding this comment

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

I see you've included GODRIVER-2310 in the PR description, but just want to note that this PR does not include the snapshot examples. I like the refactor to make sure each example is an explicit subtest, but I wonder about the necessity of the subtestWrapper struct and associated methods.

documentation_examples.RunCommandExamples(t, db)
documentation_examples.IndexExamples(t, db)
documentation_examples.StableAPIExamples()
stw := newSubtestWrapper(t, db)
Copy link
Contributor

Choose a reason for hiding this comment

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

While I do enjoy the abstraction here, is the usage of subtestWrapper strictly necessary? What would you think about just making each of these an explicit subtest? As in:

t.Run("InsertExamples", func(t *testing.T) {
   documentation_examples.InsertExamples(t, db)
}
t.Run("QueryToplevelFieldsExamples", func(t *testing.T) {
   documentation_examples.QueryToplevelFieldsExamples(t, db)
}
...

That might be more readable?

Copy link
Member Author

Choose a reason for hiding this comment

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

The only real value of the wrapper is to avoid multi-line subtests but that's just cosmetic. I'm more than happy to do it this way.

stw.run("DeleteExamples", documentation_examples.DeleteExamples)
stw.run("RunCommandExamples", documentation_examples.RunCommandExamples)
stw.run("IndexExamples", documentation_examples.IndexExamples)
stw.runEmpty("StableAPExamples", documentation_examples.StableAPIExamples)

// Because it uses RunCommand with an apiVersion, the strict count example can only be
// run on 5.0+ without auth. It also cannot be run on 6.0+ since the count command was
Copy link
Contributor

Choose a reason for hiding this comment

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

You could also use an mtest.Testing instance for the StableAPIStrictCountExample like mtest.Options().MinServerVersion("5.0").MaxServerVersion("6.0").Auth(true) instead of the odd testutil and os.Getenv logic I wrote below.

@prestonvasquez prestonvasquez deleted the GODRIVER-2310.addExampleTestWrapper branch March 23, 2022 22:31
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.

2 participants