Skip to content
This repository has been archived by the owner on Aug 2, 2021. It is now read-only.

Incentives rpc test #1733

Merged
merged 8 commits into from
Sep 10, 2019
Merged

Incentives rpc test #1733

merged 8 commits into from
Sep 10, 2019

Conversation

holisticode
Copy link
Contributor

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.

@holisticode holisticode self-assigned this Sep 6, 2019
Copy link
Member

@janos janos left a 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.


// 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_"
Copy link
Member

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.

func TestSwapRPC(t *testing.T) {

var (
p2pPort = 30100
Copy link
Member

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.

Copy link
Contributor Author

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


// start a service stack
stack := createAndStartSvcNode(swap, ipcPath, p2pPort, t)
defer stack.Stop()
Copy link
Member

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()
	}()

Copy link
Contributor Author

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?

Copy link
Member

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.

Copy link
Member

@janos janos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@mortelli mortelli left a 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

@holisticode holisticode force-pushed the incentives-rpc-test branch 2 times, most recently from c0c52da to 2770a46 Compare September 9, 2019 21:17
@holisticode holisticode merged commit b9ce9f7 into master Sep 10, 2019
@skylenet skylenet added this to the 0.5.0 milestone Sep 17, 2019
chadsr added a commit to chadsr/swarm that referenced this pull request Sep 23, 2019
* '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)
@mortelli mortelli deleted the incentives-rpc-test branch November 14, 2019 18:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants