-
Notifications
You must be signed in to change notification settings - Fork 112
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 like that this way a lot of boilerplate code is centered in one place (testutil.Init
).
However, I would consider renaming Init
in this case to ParseFlags
or something like that. Its function becomes clearer where it's used like that (testutil.Init
doesn't tell you much what it does)
@holisticode thanks, yes, you are right about the lack of clarity for the Init name. I choose that name because of testing.Init, as it should wrap it. Function testutil.Init does a bit more than flags parse, but ParseFlags would be much more clear even if it sets logging also. I would like to hear what others think. I am ok with both Init and ParseFlags. |
so the stuff that used to be in testutil, should we not create package specific testing subpackages like |
@zelig I suppose that chunk/testing is better thing to do, even if it would be a very small package. I will create it. |
Since Go 1.13 was released a few days ago, could we have a few more reviews of these changes so that we can upgrade? It brings some nice improvements, like more performant defer, mutexes and once.Do, among other things. |
@janos Can you update |
@skylenet yes, I've upgraded the go version in the latest commit, but I have left go 1.11 travis job as ti states "is needed because of the Ubuntu PPA builds". If that is not needed, I will happily remove it. |
* 'master' of github.com:ethersphere/swarm: all: support go 1.13 change to testing/flag packages (ethersphere#1710) swap: refactor lastReceivedCheque, lastSentCheque, balances to peer (ethersphere#1725)
This PR is not urgent. It is a proposal for a breaking change in the soon to be released Go 1.13. To reproduce the problem, install go1.13rc2 and run tests on master branch:
Go 1.13 adds a new function https://tip.golang.org/doc/go1.13#testing
testing.Init
https://tip.golang.org/pkg/testing/#Init that registers testing flags but flag package is complaining ifflag.Parse()
is called before that function, as we do in our init functions to parse testing cli options. We need to parse flags to setup logging options before we call tests in packages. A discussion on the subject is here golang/go#31859.One solution is to call
testing.Init
before we callflag.Parse
and to do that with support for Go version less than 1.13 which do not havetesting.Init
, this PR addstestutil.Init
that handles these cases.The problem is that it changes how we initiate our tests, as testutil now needs to be imported to other packages, and some of swarm packages are already imported in testutil, resulting a cyclic dependency. I think that testutil should be able importable to any swarm package, so I specific helper functions to other packages, leaving testuitl with only stdlib imports.
Another change that this PR introduces is a unification of logging setup in tests, which I believe is needed. I took this opportunity to do this change as the change to all test init functions are already made because of it.
This PR also fixes a problem in
cmd/swarm
package where loglevel option was never parsed as flag.Parse was never called. Function call testutil.Init must be called only on first reexec.Init call.