Client-Side Library Implementation

[strib]

Finished my first pass. Exciting!

[jxguan]

Update on the progress: I've written the code to use NewConnectionWithTransport, but now I'm having weird flaky tests. Among the five tests, only the first three passes. But if I disable the first three, the last two would also pass sometimes (but not always). I'm still investigating this - I believe it might be some connection settings issue.

[jxguan]

https://github.com/keybase/search/tree/bugfix

I'm now officially stuck. I now only enable part of the TestDeleteFile test, and everything works fine. But once I uncomment lines 180-192 in client_test.go, the test becomes flaky, as sometimes the CreateClient would return an error

--- FAIL: TestDeleteFile (0.03s)
    client_test.go:81: error when creating the client: dial tcp 127.0.0.1:8022: connect: cannot assign requested address

Originally I thought I got this error because of too many connections, but in this case I'm only opening one single connection. Also, that part is not even in the commented-out code... I'm utterly confused. I think it's highly possible that I'm missing something obvious here. I should probably save time by dealing with this bug later until you have time to take a quick look.

[strib]

@jxguan: I thought you weren't going to use any TCP connections during the client-side tests? What happened to implementing a simple test server that implements the search server interface?

Anyway, you're probably hitting something like this, where the previous TCP connection to the server is still in TIME_WAIT: https://stackoverflow.com/questions/26019164/too-many-time-wait-connections-getting-cannot-assign-requested-address, probably because the client is shutting down the TCP connection. It's probably not easy to have the server shut down the connection, so I'd say you should get rid of the need for a TCP connection altogether.

[jxguan]

I see. Yeah I've written the fake test server and use that in all the tests. But that seems unable to test the CreateClient function. So I was using the old tests to check that my implementation using NewConnectionWithTransport would work. So is there any proper way to test the updated CreateClient part?

[strib]

I see, I was looking in the bugfix branch. I would refactor CreateClient into a helper function that takes a SearchServerInterface, and then you can have a much more limited constructor that creates a connection and constructs a client, and then passes it to the helper function. Then your tests can use the same helper, but just pass in a fake server instead. That'll get most of the test coverage, without involving TCP.

[jxguan]

K I think I've fixed the issues. Would be great to have a second pass before merging.

[strib]

Looks good to me mod comments, thanks!