-
Notifications
You must be signed in to change notification settings - Fork 564
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
don't run main function when testing (fixes #277) #282
Conversation
I can confirm this fixes the reproduce case I've provided. |
If it's not too hard, it might be nice to add a regression test for this, since in most cases running Feel free to use or base it on If you feel confident this won't break in the future or this test doesn't belong here, it's fine not to add it. |
I am not familiar with the context of the fix, and I don't completely understand the change, so take that into account. I don't see anything obviously wrong with it, tests pass and the reported issue is resolved, so it LGTM but I cannot offer full confidence in it. |
Test added. Can I tell you more about the change? The problem was that I assumed that you can not depend on a |
That was very helpful indeed, thanks for the explanation. |
|
||
import "testing" | ||
|
||
func TestMain(t *testing.T) { |
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.
@neelance, are you aware that this name TestMain
has special meaning as of Go 1.4?
The
testing
package has a new facility to provide more control over running a set of tests. If the test code contains a functionfunc TestMain(m *testing.M)
that function will be called instead of running the tests directly. The M struct contains methods to access and run the tests.
Source: https://golang.org/doc/go1.4
I kinda suspect it's an unintentional usage of that feature here, since you're testing a feature named main. If so, I think it's better we rename it to avoid doing something unintentionally just due to a coincidence in naming, as it might cause confusion later (if other tests are ever added here, they will not run). Perhaps func TestNotRunMain(m *testing.M)
or so?
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.
@neelance, did you see this?
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 did not know that. Thanks for pointing it out. Fixed via 01e6396.
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.
Great, thanks!
No description provided.