-
Notifications
You must be signed in to change notification settings - Fork 955
testing: implement Cleanup(), with tests #2505
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
testing: implement Cleanup(), with tests #2505
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.
Almost there. Looking good!
569b539
to
5700168
Compare
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.
LGTM
Please resolve merge conflict. Thanks. |
No change in behavior, just preparing for next commit, and gently nudging code closer to upstream.
Also reorder and regroup common's fields slightly to match upstream. TODO: pull in more upstream tests once this package is goroutine-safe
5700168
to
5d5ad88
Compare
Thank you @dkegel-fastly now merging. |
|
||
// matchStringOnly is part of upstream, and is used below to provide a dummy deps to pass to MainStart | ||
// so it can be run with go (tested with go 1.16) to provide a baseline for the regression test. | ||
// See c56cc9b3b57276. Unfortunately, testdeps is internal, so we can't just use &testdeps.TestDeps{}. |
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.
Unfortunately, testdeps is internal, so we can't just use &testdeps.TestDeps{}.
That's not the reason: the testing package can most certainly import internal packages because they're both part of GOROOT. The reason that testdeps exists is so that the testing package can import useful packages while those packages can still import the testing package (for their own tests) without creating a circular dependency.
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.
Oops, nevermind. The comment is correct. I didn't realize the code was in testdata/testing.go (instead of src/testing.go).
t.Cleanup() will be needed when we implement t.TempDir().
Land it now with tests to reduce the size of the future pull request for t.TempDir().
Also update testdata/testing.go to run properly with go 1.16 (it crashed without this).
This was useful while I was debugging the tRunner() change.