-
Notifications
You must be signed in to change notification settings - Fork 110
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 think that this test does not hurt to have and at least gives basic rpc over ipc functionality validation.
swap/protocol_test.go
Outdated
|
||
// newServiceNode creates a p2p.Service node stub | ||
func newServiceNode(port int, ipcPath string, httpport int, wsport int, modules ...string) (*node.Node, error) { | ||
var datadirPrefix = ".data_" |
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.
This directory is created inside the swap package. I think that a temporary directory would be cleaner. It is nicely removed in TestSwapRPC, but maybe it would be good to avoid creating temporary test directories in the project code.
swap/protocol_test.go
Outdated
func TestSwapRPC(t *testing.T) { | ||
|
||
var ( | ||
p2pPort = 30100 |
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.
It would be good to discover a free port to listen on. I think that there is a function in the codebase to do so in almost reliable way, but good enough, maybe assignTCPPort in cmd/swarm/config_test.go. But I am not sure if it is needed as rpc is connected over ipc, it works only with 0 value.
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.
Indeed, I removed the port altogether, works fine as it is only ipc
swap/protocol_test.go
Outdated
|
||
// start a service stack | ||
stack := createAndStartSvcNode(swap, ipcPath, p2pPort, t) | ||
defer stack.Stop() |
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.
This test spends most fo the time in stopping the node here, and since it does not validate rpc node lifecycle, but only swap balance calls, I think that it can be made "faster" with putting that in the background:
defer func() {
go stack.Stop()
}()
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.
booah waaaaaayyy fasterrrrrrr!!
;)
Is it ensured though that Stop
will be executed, and that the test will not terminate before that, creating a memory leak?
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.
Well, test will terminate before Stop is done. There are two cases after the test is done. One is that there are no more tests to be executed (depending on go test command options or order of tests in the package) and the goroutine that calls Stop will be terminated before the Stop is done. This is ok, as the main goroutine is terminated also. Another case is that there are other tests, and that Stop goroutine will be scheduled somewhere in the future, but we do not know when. This should be also ok, as the stack is not a global variable, the test result is not dependent on Stop being called and we know that the stop will be done in some reasonable time, not creating a gorotuine leak. I think that it is ok to do this trick in this test.
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
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, only needs rebasing with master
c0c52da
to
2770a46
Compare
2770a46
to
5e68322
Compare
* 'master' of github.com:ethersphere/swarm: network: terminate Hive connect goroutine on Stop (ethersphere#1740) Incentives rpc test (ethersphere#1733) swarm, swap: pass chequebook address at start-up (ethersphere#1718)
This PR adds a test for an API call of swap's balance interface via RPC.
It basically starts a node stack with a Swap Service and then just queries for balances. The test is mainly meant to check that the API works, not for testing the balance maths.