-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add websocket transport to transit relay #40
Add websocket transport to transit relay #40
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 guess some rework is necessary for the tests to make it compile with the API changes in our current code.
wormhole/wormhole_test.go
Outdated
relayServer := newTestTCPRelayServer() | ||
defer relayServer.close() | ||
testDisableLocalListener = true | ||
defer func() { testDisableLocalListener = false }() |
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 might not work anymore on our master as we removed this global variable in 66d2eb9.
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.
Should this code be updated to use t.disableListener
, reverted to the old revision, removed entirely... I see lines 250-254 preserves the old code so I assume we can just undo this change?
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.
Oh never mind, functions such as Receive() set that already. So I guess that leaves the question, do we want the old relayServer assignment and deferment?
wormhole/wormhole_test.go
Outdated
if err != nil { | ||
t.Fatal(err) | ||
} | ||
receiver, err := c1.Receive(ctx, code) |
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.
Receive()
now takes 3 parameters. So this would not compile in the current form.
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.
Do we want disableListener
parameter true or false here?
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.
You want disableListener
as false for those tests that just use relay and not transfer from client to client.
wormhole/wormhole_test.go
Outdated
}, | ||
} | ||
testDisableLocalListener = true | ||
defer func() { testDisableLocalListener = false }() |
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.
Same comment as above. This global has been removed.
wormhole/wormhole_test.go
Outdated
t.Fatal(err) | ||
} | ||
// skip th wrapper so we can provide our own offer | ||
code, _, err := c0.sendFileDirectory(ctx, offer, r) |
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.
sendFileDirectory()
also takes an extra param now.
wormhole/wormhole_test.go
Outdated
if err != nil { | ||
t.Fatal(err) | ||
} | ||
receiver, err := c1.Receive(ctx, code) |
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.
same comment here as above on Receive()
Suggest doing a build and run the test and try to fix those errors to make it build/test. |
Client.SendFile, Client.Receive, and other functions now require a disableListener parameter. This replaces the global testDisableLocalListener.
Client.SendFile, Client.Receive, and other functions now require a disableListener parameter. This replaces the global testDisableLocalListener.
TestPendingSendCancelable and TestPendingRecvCancelable tests do not complete before 10 minute timeout, so disable/remove these tests for now.
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.
Looks good to me. 👍 Please leave out those four snippets of code and merge. Thanks!
…eastAuthority#46)"" This reverts commit d9b3558.
Canceling the context will now cancel an in-flight file transfer. This should cover most cases. There might still be some edge cases where cancellation doesn't work. Closes: #40 [via git-merge-pr]
Cherry-picked commits from psanford#63
Code Review Checklist (to be filled out by reviewer)