cmd/peer: Refactor and test processing of response

[simar7]

This PR refactors a small piece of processing code and adds tests.

Signed-off-by: Simarpreet Singh simar@linux.com

[simar7]

Some insight: Initially I wanted to treat the runWatch function as a black box and write a test for it to simulate state changes. While it was exciting at first, the test setup for this proved to be quite involved since runWatch runs a busy loop and is only controllable through the error return value from b.Recv().

In favour of simplicity and readability, I instead took this approach. This refactors and tests the functional aspects of processing an event, rather than its setup.

[rolinh]

Initially I wanted to treat the runWatch function as a black box and write a test for it to simulate state changes. While it was exciting at first, the test setup for this proved to be quite involved since runWatch runs a busy loop and is only controllable through the error return value from b.Recv().

While not trivial, it's probably simpler than you think. We actually have a fake peer client in the Cilium tree. It is actually used for Hubble Relay's pool manager unit tests.
As part of the test, you can simply instantiate a fake peer client add pass it to runWatch(). You don't have to go this route though :) But it should actually be fairly simple to implement as all that needs to be done is to return something when the fake peer client's Notify method is called.

[simar7]

Initially I wanted to treat the runWatch function as a black box and write a test for it to simulate state changes. While it was exciting at first, the test setup for this proved to be quite involved since runWatch runs a busy loop and is only controllable through the error return value from b.Recv().

While not trivial, it's probably simpler than you think. We actually have a fake peer client in the Cilium tree. It is actually used for Hubble Relay's pool manager unit tests.
As part of the test, you can simply instantiate a fake peer client add pass it to runWatch(). You don't have to go this route though :) But it should actually be fairly simple to implement as all that needs to be done is to return something when the fake peer client's Notify method is called.

Thanks for that insight @rolinh – I unknowingly did venture into that route on my own before charting on the current way but I will re-visit it once again. The pool manager unit tests look very interesting in their approach. Definitely something to learn from.

I'll time box and take a stab at it. There's definitely value in testing runWatch() as a whole.

[simar7]

So I tried to use the testutils package as is it has the test doubles needed in this case but I ran into a small hiccup. I believe the package isn't importable outside the repository as it doesn't have a go.mod within the package. https://github.com/cilium/cilium/tree/master/pkg/hubble/testutils

I tried to validate the above by doing the following, which seems to work:

1. Locally cloning a copy of cilium repo
2. Adding the appropriate go.mod

$ testutils git:(master) ✗ cat go.mod 
module github.com/pkg/hubble/testutils

go 1.16

3. Doing a replace directive to point at the locally cloned cilium repo.

replace (
	github.com/cilium/cilium/pkg/hubble/testutils => /Users/simar/repos/cilium/pkg/hubble/testutils
)

I believe testutils may or may not have been intended to use outside of the cilium repo and as a result it isn't importable as a go module. It should be a simple addition to make testutils importable and I can contribute a PR for it if that's OK.

In the interest of slicing thin, do you think we should keep adding tests for runWatch() in a separate PR and merge this if it looks good to you as is?

[rolinh]

I believe the package isn't importable outside the repository as it doesn't have a go.mod within the package.

Actually, this is not the issue at play here. We suffer from a problem with the grpc dependency which broke version compatibility and this affects a cilium dependency (etcd). We managed to use a recent grpc version in this repository because so far, nothing we pulled from cilium pulled the etcd dependency. It seems that pulling in the testutils package does :/ :

$ go mod tidy && go mod vendor && go mod verify
go: finding module for package google.golang.org/grpc/naming
github.com/cilium/hubble/cmd/peer tested by
        github.com/cilium/hubble/cmd/peer.test imports
        github.com/cilium/cilium/pkg/hubble/testutils imports
        github.com/cilium/cilium/pkg/ipcache imports
        github.com/cilium/cilium/pkg/kvstore imports
        go.etcd.io/etcd/clientv3 tested by
        go.etcd.io/etcd/clientv3.test imports
        go.etcd.io/etcd/integration imports
        go.etcd.io/etcd/proxy/grpcproxy imports
        google.golang.org/grpc/naming: module google.golang.org/grpc@latest found (v1.36.0), but does not contain package google.golang.org/grpc/naming

More information about this problem here. We're currently pending on a new release of the etcd dependency which should fix this problem.

In the interest of slicing thin, do you think we should keep adding tests for runWatch() in a separate PR and merge this if it looks good to you as is?

As I can't tell when the above issue will be resolved, I think your proposal is reasonable.

[simar7]

I believe the package isn't importable outside the repository as it doesn't have a go.mod within the package.

Actually, this is not the issue at play here. We suffer from a problem with the grpc dependency which broke version compatibility and this affects a cilium dependency (etcd). We managed to use a recent grpc version in this repository because so far, nothing we pulled from cilium pulled the etcd dependency. It seems that pulling in the testutils package does :/ :

$ go mod tidy && go mod vendor && go mod verify
go: finding module for package google.golang.org/grpc/naming
github.com/cilium/hubble/cmd/peer tested by
        github.com/cilium/hubble/cmd/peer.test imports
        github.com/cilium/cilium/pkg/hubble/testutils imports
        github.com/cilium/cilium/pkg/ipcache imports
        github.com/cilium/cilium/pkg/kvstore imports
        go.etcd.io/etcd/clientv3 tested by
        go.etcd.io/etcd/clientv3.test imports
        go.etcd.io/etcd/integration imports
        go.etcd.io/etcd/proxy/grpcproxy imports
        google.golang.org/grpc/naming: module google.golang.org/grpc@latest found (v1.36.0), but does not contain package google.golang.org/grpc/naming

More information about this problem here. We're currently pending on a new release of the etcd dependency which should fix this problem.

In the interest of slicing thin, do you think we should keep adding tests for runWatch() in a separate PR and merge this if it looks good to you as is?

As I can't tell when the above issue will be resolved, I think your proposal is reasonable.

Oh I see. I'll subscribe to that PR in that case to keep track.