Add TestNewSwarm and simplify NewSwarm constructor #557
Conversation
| common.FromHex(config.BzzKey), | ||
| kp, | ||
| ) | ||
|
|
||
| config.HiveParams.Discovery = true |
There was a problem hiding this comment.
Why are we overriding the configuration param here?
| @@ -228,7 +225,6 @@ func NewSwarm(ctx *node.ServiceContext, backend chequebook.Backend, config *api. | |||
| self.bzz = network.NewBzz(bzzconfig, to, stateStore, stream.Spec, self.streamer.Run) | |||
|
|
|||
| // Pss = postal service over swarm (devp2p over bzz) | |||
| config.Pss = config.Pss.WithPrivateKey(self.privateKey) | |||
There was a problem hiding this comment.
I think that config should bot be mutated in constructor, so I moved this line to api.Config.Init function. @nolash is that ok?
| return nil, fmt.Errorf("error connecting to SWAP API %s: %s", config.SwapApi, err) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
was there a reason why this was initialized outside before?
There was a problem hiding this comment.
To have ipcEndpoint available in all test cases.
There was a problem hiding this comment.
and I guess now we don't need that anymore?
| t.Error("private key is not set") | ||
| } | ||
| if !s.config.HiveParams.Discovery { | ||
| t.Error("config.HiveParams.Discovery is false, must be true regardless the configuration") |
There was a problem hiding this comment.
Why do we have configuration parameters if it always has to be true?
There was a problem hiding this comment.
Exactly my question in this PR #557 (diff). That is the case that we have now.
| if s.dns == nil { | ||
| t.Error("dns is not initialized") | ||
| } | ||
| }, |
There was a problem hiding this comment.
we should check mutable resources here aswell, their validation depends on ens.
There was a problem hiding this comment.
ResourceHandler has ensClient and ethClient as unexported fields. How would you suggest to do the check?
There was a problem hiding this comment.
I guess it's not so easy, also because ResourceHandler isn't even a member of the Swarm object. But it got me thinking that there should probably be some way of querying certain parts of the state of it. I'll keep it in mind for later.
| }, | ||
| check: func(t *testing.T, s *Swarm, _ *api.Config) { | ||
| if s.backend != nil { | ||
| t.Error("backend is not nil") |
There was a problem hiding this comment.
What does this configuration mean in practice? Should it actually do something?
There was a problem hiding this comment.
backend is related to swap, it is just unfortunately called.
There was a problem hiding this comment.
I don't understand the answer, but it doesn't seem important in any case.
Objects states are mostly private and not accessible from the |
| dir, err := ioutil.TempDir("", "swarm") | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } |
There was a problem hiding this comment.
Ups, thanks Lewis. It is fixed now.
This PR: