HTTP timeouts and proxy

[pgillich]

It's an extended implementation for #183 .

[crobby]

Is it possible to have some unit tests added to cover this functionality?

[pgillich]

Is it possible to have some unit tests added to cover this functionality?

Yes. Done.

[alegrey91]

Hello @pgillich, the PR looks good but I suspect it is a bit over complicated for what we actually need.
Can you please explain what's the need of creating the HTTPClienter interface and mock the New() method?
I might be missing something, so better to ask.

[alegrey91]

Can we move it undert the cliclient_test.go file?

[pgillich]

Yes, we can. But the compile will be failed, because this function is called from another package, too (cmd/*_test.go). Go compiler enables importing from *_test.go files only from same package.

It's possible to create a new package, for example testutil, which can be imported from everywhere. But it may introduce a new claim: a new package. But anyway, if you mind, I can do it.

[alegrey91]

I think that creating a new package would be a better approach. What do you think @crobby ?

[alegrey91]

Can we move it undert the cliclient_test.go file?

[pgillich]

Same situation to TestingReplaceDefaultHTTPClient.

[alegrey91]

Personally, I would avoid adding conditions for tests into main code.

[pgillich]

Me too, but the config.ServerConfig does not support the insecure flag.

If you mind, I can extend the config.ServerConfig with the insecure flag, annotated with json:"-", to make sure, it won't be loaded from config file.

[alegrey91]

Can you explain why we need to set the Insecure for tests?

[pgillich]

A httptest.Server was started, which supports TLS (client TLS config can be get), but it was unable to use, because github.com/rancher/norman/clientbase.NewAPIClient replaces net/http.Client.Transport (including proxy and TLS config),
so the client TLS config of net/http/httptest.Server will be lost. I cannot understand why NewAPIClient was designed this way.

So insecure must be used with httptest.Server.

[pgillich]

Hello @pgillich, the PR looks good but I suspect it is a bit over complicated for what we actually need.
Can you please explain what's the need of creating the HTTPClienter interface and mock the New() method?
I might be missing something, so better to ask.

Without mocking the network communication, below new/changed functions cannot be tested:

• cmd/login.go:getCertFromServer()
• cmd/ssh.go:getSSHKey()
• cliclient/cliclient.go:newManagementClient()

The HTTPClienter interface is needed for mocking, because in Go, only interfaces can be mocked.

Originally I didn't want to write unit tests, because I could see, the coverage is low and the source code structure is not ready for writing good unit test coverage (for example: missing Go interfaces). But @crobby asked me to write unit test. So I did it.

[alegrey91]

Hello @pgillich, the PR looks good but I suspect it is a bit over complicated for what we actually need.
Can you please explain what's the need of creating the HTTPClienter interface and mock the New() method?
I might be missing something, so better to ask.

Without mocking the network communication, below new/changed functions cannot be tested:

• cmd/login.go:getCertFromServer()
• cmd/ssh.go:getSSHKey()
• cliclient/cliclient.go:newManagementClient()

The HTTPClienter interface is needed for mocking, because in Go, only interfaces can be mocked.

Originally I didn't want to write unit tests, because I could see, the coverage is low and the source code structure is not ready for writing good unit test coverage (for example: missing Go interfaces). But @crobby asked me to write unit test. So I did it.

I got the point and thanks for writing tests. My concern is that we are introducing several things that can be considered superfluous. What I mean is that we can probably avoid of testing the NewManagementClient() since we are just checking that the master client is not nil. wdyt @crobby?
If possible I would avoid mocking things when possible.

[pmatseykanets]

IMHO we shouldn't bring another mocking tool. If we need mocks we probably have to use what rancher/rancher uses go.uber.org/mock.

[pgillich]

I got the point and thanks for writing tests. My concern is that we are introducing several things that can be considered superfluous. What I mean is that we can probably avoid of testing the NewManagementClient() since we are just checking that the master client is not nil. wdyt @crobby? If possible I would avoid mocking things when possible.

I understand you would like to keep the source code as simple as possible. But automatic tests make the code more complex, in order to introduce clean coding boundaries. This extra cost cannot be avoided and it's higher in Go, comparing to VM-based languages, for example Python and Java.

Automatic tests are important for open source projects, because there aren't dedicated manual testers, who make manual tests for new functions and regression tests. The tradeoff between untested features (risk) and use case coverage (cost) is decision of the project (repo) maintainers.

When I watched the source code at the first time, I realized, the risk <--> cost decision was low code (use case) coverage, because the boundaries (Go interfaces) were missing for high code coverage. So I skipped the automatic test.

But when @crobby asked me to write automatic tests, I did it. There were some new/changed codes, which can be tested by unit tests with low effort. But there were some new/changed codes which need too much effort to write unit tests, because the code should be restructured (boundaries with Go interfaces), before writing unit tests.

Instead of the costly restructuring, I introduced function tests, which emulate a Rancher server responses with httptest.Server. The TestMasterClientNewManagementClient is also a function test, not a simple unit test, which checks only if master client is not nil. It's a function test, which covers more codes, including communication with Rancher server. During running the test, the usage of cliclient.DefaultHTTPClient is also tested by mock (the test is failed, if it's not used).

The introduced function tests look superfluous at the first time, but the this investment will return. This kind of tests can be improved to component tests, which start a cli.Command and cover most of the code.

The decision is in the hands of the maintainers: turning to the function and component tests, or staying at only unit test. Please notice me the unified decision of the maintainers and I will change the mocks to go.uber.org/mock or I will remove all the function tests and its related code changes.

[crobby]

Closing in favor of #406.
@pgillich Thank you so much for bringing attention to this and your contribution.