Network Server uplink fixes and improvements #912
Conversation
rvolosatovs
force-pushed
the
feature/ns-uplink
branch
from
d6f77b1 to
c829d6c
Compare
|
I originally preserved the merge commits on purpose, as mentioned in description. |
| defer test.SetDefaultEventsPubSub(&test.MockEventPubSub{ | ||
| PublishFunc: func(ev events.Event) { | ||
| switch name := ev.Name(); name { | ||
| case "gs.up.receive", "gs.down.tx.success", "gs.down.tx.fail", "gs.status.receive": |
There was a problem hiding this comment.
I tried to refactor this into using the assertions in test package, but there are too many unhandled events published in the test script, so I adapted it in such a way.
There was a problem hiding this comment.
What would be the preferred way instead?
There was a problem hiding this comment.
Catch and assert all events published. (e.g. gs.gateway.{dis,}connect, gs.up.drop, etc.)
| @@ -664,9 +657,30 @@ func TestGatewayServer(t *testing.T) { | |||
| t.Run(tc.Name, func(t *testing.T) { | |||
| a := assertions.New(t) | |||
|
|
|||
| testCtx := test.ContextWithT(test.Context(), t) | |||
| testCtx, cancel := context.WithTimeout(testCtx, 5*timeout) | |||
There was a problem hiding this comment.
What is the benefit of this?
Now if operation 1 takes longer than timeout, it passes on to operation 2 which may not have enough time to finish, causing the wrong part of the test to fatal.
There was a problem hiding this comment.
The time.After(timeout) was used in the select statement in order to prevent deadlocks, right? AFAIK there's no predefined time interval, within which an operation has to finish.
Given this, this approach achieves exactly the same goal as the old time.After(timeout), however not it's done on per-testcase basis, not per-operation.
- The
ctx.Done()can be used not only by the test cases, but also by the functions, takingctx(not relevant here, but is often the case in other tests) - If an operation 1 took a bit longer than
timeout, the test case should not fail IMO, it should continue execution, since thetimeoutvalue is just an arbitrary value we chose to avoid deadlocks.
Please note that the context timeout is 5*timeout, which seemed to approximate the worst-case timeout ceiling used in previous approach.
There was a problem hiding this comment.
To address case you describe exactly: if operation 1 took longer than timeout, but succeeded, then the timeout value is too low
There was a problem hiding this comment.
The
time.After(timeout)was used in theselectstatement in order to prevent deadlocks, right?
No, it is used for testing timeouts of steps.
AFAIK there's no predefined time interval, within which an operation has to finish.
It's not about the time interval, it is about giving each step a certain time to finish.
Again, what is the benefit of a timeout context? I get the context cancellation part for the test, but simply not the timeout.
To address case you describe exactly: if operation 1 took longer than
timeout, but succeeded, then thetimeoutvalue is too low
The problem is that if you have a timeout of 100 and you have 5 operations, that there's a timeout after 500, right? So if operation 1 takes 490, operation 2 is very unlikely to finish in the remaining 10, so it gives a "operation 2 timeout" error, while that is not the correct fatal.
| defer test.SetDefaultEventsPubSub(&test.MockEventPubSub{ | ||
| PublishFunc: func(ev events.Event) { | ||
| switch name := ev.Name(); name { | ||
| case "gs.up.receive", "gs.down.tx.success", "gs.down.tx.fail", "gs.status.receive": |
There was a problem hiding this comment.
What would be the preferred way instead?
rvolosatovs
force-pushed
the
feature/ns-uplink
branch
from
3a8ae54 to
f5b65d1
Compare
|
@johanstokking please see updated changes(f5b65d1) |
Summary
Combination of
#872
#863
#858
#870
#860
Changes
See the PRs referenced above.
Notes for Reviewers
Preserved merges, rebased on top of
masterand added 3 minor commits to this:64c6bb8
46e98fc
d6f77b1
c829d6c @htdvisser
3a8ae54 @johanstokking