Review time.sleep in tests

[ayuryshev]

Task(s)

1. Examine tests using time.Sleep. Examine stability of test before start of work:

go clean -testcache || go test ./pkg/[package]/...   # repeat 10 or more times

or

go clean -testcache || go test -timeout 1m github.com/skycoin/skywire/pkg/[package] -run [name of test]

note random FAILs if they will occure.

2. Try to eliminate time.Sleep from tests without changing their behaviour
3. In case if time.Sleep is justified - write a comment describing "why time.Sleep is justified in this test"
4. Check stability of test after changes in the same way

Tests with time.Sleep

• [ ] internal/noise/net_test.go:2
• [ ] internal/therealproxy/server_test.go:2
• [ ] internal/therealssh/channel_pty_test.go:4
• [ ] internal/therealssh/channel_test.go:3
• [x] internal/therealssh/client_test.go:1
• [x] pkg/app/app_test.go:1
• [x] pkg/messaging/client_test.go:6
• [ ] pkg/messaging/pool_test.go:2
• [ ] pkg/node/node_test.go:3
• [ ] pkg/node/rpc_test.go:2
• [ ] pkg/router/router_test.go:15
• [ ] pkg/transport/manager_test.go:5

[jdknives]

@Darkren on Discord:

I didn’t get the intent of this. Based on the issue description I suppose that tests with time.Sleep behave unstable, but I’m not sure about that. I’d say that unstable code will be unstable with or without time.Sleep. The only problem I see with time.Sleep is that a lot of such calls may exceed test timeout, thus leading to failure

[ayuryshev]

Those tests has at least one defect - their duration is always time.Sleep duration + time of test itself. I propose to cleanup them in a way described in: https://speakerdeck.com/mitchellh/advanced-testing-with-go?slide=62

...
select {
   case <- thingHappened:
   case <- time.After(delay):
     t.Fatal("timeout")
}

I counted 50 cases of using time.Sleep in *test.go