Improve unit tests

[maximilien]

Based on various discussions on different PRs (pasted below). But the gist is to improve unit tests for commands.

From @rhuss comment on PR #66

• Refactor tests to become more unit tests. E.g. commands should be tested only that they propagate the proper info to the library calls (e.g. in serving), the library calls should then use the serving mock library. Only a suggestion, but tests currently are not easy to maintain.

100% agree. I'd use Ginkgo/Gomega and Counterfeiter.... Or similar tooling

[maximilien]

Any opposition to the the tools I mentioned above?

[rhuss]

Any opposition to the the tools I mentioned above?

Nope as I don't know any of them yet so don't have any preferences ;-) Happy to learn though ...

[maximilien]

Sweet. I'll cook up something on a simple command so we can all see the resulting shape and impact on imports etc.

[maximilien]

OK so I submitted PR #185 with an example UT for the kn version command. I know it's a simple one, but I think it illustrates the power (I think) of BDD-style UTs, and of the Ginkgo/Gomega framework. Let me re-iterate a few points and give a few examples of what we gain if we start doing UTs this way. There are more but perhaps best to highlight some of the key ones first.

First things first:

1. Ginkgo/Gomega is a very mature BDD-style test framework. You can write any tests with it. Unit, integration, etc.
2. It's been used for over 5 years in Cloud Foundry (CF) and other places. And CF is highly tests-driven community. So tons of tests and optimization already done with this framework. It's solid
3. It's compatible with go test so this means that we can mix and match go test tests with Ginkgo/Gomega. So with this new test for version, the other tests still run.
4. I did no change to current tests scripts, e.g., ./hack/build.sh and ./test/presubmit-tests.sh or e2e tests. All run fine.
5. We can gradually transition as needed.

Second, what are some quick main advantages of doing UTs with Ginkgo/Gomega:

1. Better test structure. The version command test is super clear, once you understand basic structure and focus on the tests. See PR and this simplified snippet to see what I mean:

package commands_test

import (
   ...
)

var versionOutput = `Version:      fake-version
Build Date:   fake-build-date
Git Revision: fake-git-revision
Dependencies:
- serving:    fake-serving-version
`
var _ = Describe("Version", func() {
	const (
   ...
	)

	var (
		versionCmd *cobra.Command
		knParams   *KnParams
	)

	BeforeEach(func() {
   ...
	})

	Describe("NewVesionCommand", func() {
		It("Creates a VersionCommand", func() {
			Expect(versionCmd).NotTo(BeNil())
			Expect(versionCmd.Use).To(Equal("version"))
			Expect(versionCmd.Short).To(Equal("Prints the client version"))
			Expect(versionCmd.RunE).NotTo(BeNil())
		})

		It("Prints Version, Build date, GitRevision, and ServingVersion", func() {
			err := versionCmd.RunE(nil, []string{})
			Expect(err).NotTo(HaveOccurred())
			Expect(ReadStdout()).To(Equal(versionOutput))
		})
	})
})

2. Running tests show lots more details on what is running. What test. What line. You can focus on one test or group of tests by simply changing It to FIt (focus It) or (Describe to FDescribe)

Running Suite: Commands Suite
=============================
Random Seed: 1560467634
Will run 2 of 2 specs

Version NewVesionCommand
  Creates a VersionCommand
  /Users/maximilien/Desktop/github/knative-client/src/github.com/knative/client/pkg/kn/commands/version_test.go:42
•
------------------------------
Version NewVesionCommand
  Prints Version, Build date, GitRevision, and ServingVersion
  /Users/maximilien/Desktop/github/knative-client/src/github.com/knative/client/pkg/kn/commands/version_test.go:49
•
Ran 2 of 2 Specs in 0.002 seconds
SUCCESS! -- 2 Passed | 0 Failed | 0 Pending | 0 Skipped
PASS

Running with focus on one test:

Running Suite: Commands Suite
=============================
Random Seed: 1560469005
Will run 1 of 2 specs

S
------------------------------
Version NewVesionCommand
  Prints Version, Build date, GitRevision, and ServingVersion
  /Users/maximilien/Desktop/github/knative-client/src/github.com/knative/client/pkg/kn/commands/version_test.go:49
•
Ran 1 of 2 Specs in 0.001 seconds
SUCCESS! -- 1 Passed | 0 Failed | 0 Pending | 1 Skipped
PASS | FOCUSED

3. With Gomega matchers, errors are easier to spot. For instance if I added extra space in the expected versionOutput string for version (at the end), this is the failure printed--note the | pointing to where the difference is:

Running Suite: Commands Suite
=============================
Random Seed: 1560469050
Will run 1 of 2 specs

S
------------------------------
Version NewVesionCommand
  Prints Version, Build date, GitRevision, and ServingVersion
  /Users/maximilien/Desktop/github/knative-client/src/github.com/knative/client/pkg/kn/commands/version_test.go:49

• Failure [0.002 seconds]
Version
/Users/maximilien/Desktop/github/knative-client/src/github.com/knative/client/pkg/kn/commands/version_test.go:18
  NewVesionCommand
  /Users/maximilien/Desktop/github/knative-client/src/github.com/knative/client/pkg/kn/commands/version_test.go:41
    Prints Version, Build date, GitRevision, and ServingVersion [It]
    /Users/maximilien/Desktop/github/knative-client/src/github.com/knative/client/pkg/kn/commands/version_test.go:49

    Expected
        <string>: "...ion
        "
    to equal              |
        <string>: "...ion
         "

    /Users/maximilien/Desktop/github/knative-client/src/github.com/knative/client/pkg/kn/commands/version_test.go:52
------------------------------


Summarizing 1 Failure:

[Fail] Version NewVesionCommand [It] Prints Version, Build date, GitRevision, and ServingVersion
/Users/maximilien/Desktop/github/knative-client/src/github.com/knative/client/pkg/kn/commands/version_test.go:52

Ran 1 of 2 Specs in 0.003 seconds
FAIL! -- 0 Passed | 1 Failed | 0 Pending | 1 Skipped
--- FAIL: TestCommands (0.00s)
FAIL

4. You can group tests nicely with Context which makes for testing code with (unfortunate) complex logic a lot clearer.

[maximilien]

/assign @maximilien

[rhuss]

As I'm not really in go testing, I tried to find out some more information about test frameworks. In summary, it looks like that most of people I talked to consider Ginkgo to be quite heavyweight compared to the benefits it bring to the table.

Iy looks like that these days people are returning back to lightweight testing support and use testing directly.

E.g I talked with @vdemeester who worked on the test suite of Docker and has also written some very interesting blog posts on testing and he has imo some valid points (cited with permissions):

Ginkgo is a weird mix of Go and Gerkhin :angel:
- it adds cyclomatic complexity — and complexityt over all, with new concept fo go testing, etc.
- it hides `testing.T` and thus adds another way to run test from what ppl are used too (`-run …`, …)
(damn I really completely bricked my windows machine lol)
- you have to learn new way of writing test and there is less benefit that there used to be (now with subtest and all, you can have a concept of test suite with built-in testing)
- if you need something more gerkhin like, just use gerkhin and a go "impl" of it (or even another language for the e2e tests, not required to be all in go)

for a reference, for e2e test on the `docker` client we use plain `testing` with an helper library called `gotest.tools` (that allows to easily execute commands and check for results, etc…)
it's less invasive

So I wonder that maybe we should also start small (and e.g. introduce a lightweight assertion library first), and the move to a full blown testing platform if we see the need for it ?

Just some food for discussion ...

[maximilien]

From WG meeting on 7/2/2019 we have agreed to use gotest.tools. So closing this.