-
Notifications
You must be signed in to change notification settings - Fork 911
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
POC subtestWrapper #885
Conversation
There was a problem hiding this 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) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
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 theTestDocumentationExamples
test without decoupling the underlying functionality. For example, using the proposed refactor the following command runs only theInsertExamples
subtestThis 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
In VS Code, there is no existing inline subtest selection.