Repository: knative/client
Pull request #1357 (2021-06-28 20:50:50)

Improve test coverage of main package
## Description

Refactor `main` function to improve test coverage and testability of it as well.

## Changes

<!-- Please add list of more detailed changes. These changes should be reflected also in the commit messages -->

* Refactor to minimal  `main()`
* Wrap `run` in `runWithExit()` to produce exit codes.

## Reference

Related to #1322 

knative-prow-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/client/pull/1357#" title="Author self-approved">dsimansk</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative%2Fclient).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/knative/client/blob/main/OWNERS)~~ [dsimansk]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
dsimansk: I'd like to check how much that bumps code coverage, there are a few branches in `run()` function to add test for.
codecov[bot]: # [Codecov](https://codecov.io/gh/knative/client/pull/1357?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative) Report
> Merging [#1357](https://codecov.io/gh/knative/client/pull/1357?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative) (1b2deb9) into [main](https://codecov.io/gh/knative/client/commit/1859299c4cef8c85abee6abc11e921a0b852c571?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative) (1859299) will **increase** coverage by `0.08%`.
> The diff coverage is `80.00%`.

[![Impacted file tree graph](https://codecov.io/gh/knative/client/pull/1357/graphs/tree.svg?width=650&height=150&src=pr&token=bkTp6BzJiV&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative)](https://codecov.io/gh/knative/client/pull/1357?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative)

```diff
@@            Coverage Diff             @@
##             main    #1357      +/-   ##
==========================================
+ Coverage   73.71%   73.80%   +0.08%     
==========================================
  Files         160      160              
  Lines        8145     8146       +1     
==========================================
+ Hits         6004     6012       +8     
+ Misses       1435     1430       -5     
+ Partials      706      704       -2     
```


| [Impacted Files](https://codecov.io/gh/knative/client/pull/1357?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative) | Coverage Δ | |
|---|---|---|
| [cmd/kn/main.go](https://codecov.io/gh/knative/client/pull/1357/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative#diff-Y21kL2tuL21haW4uZ28=) | `71.05% <80.00%> (+9.71%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/knative/client/pull/1357?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/knative/client/pull/1357?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative). Last update [1859299...1b2deb9](https://codecov.io/gh/knative/client/pull/1357?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative).

knative-metrics-robot: The following is the coverage report on the affected files.
Say `/test pull-knative-client-go-coverage` to re-run this coverage report

File | Old Coverage | New Coverage | Delta
---- |:------------:|:------------:|:-----:
[cmd/kn/main.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_client/1357/pull-knative-client-go-coverage/1409615305457537024/artifacts/line-cov.html#file0) | 74.6% | 81.9% | 7.3

knative-metrics-robot: The following is the coverage report on the affected files.
Say `/test pull-knative-client-go-coverage` to re-run this coverage report

File | Old Coverage | New Coverage | Delta
---- |:------------:|:------------:|:-----:
[cmd/kn/main.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_client/1357/pull-knative-client-go-coverage/1409639976550797312/artifacts/line-cov.html#file0) | 74.6% | 91.7% | 17.0

knative-metrics-robot: The following is the coverage report on the affected files.
Say `/test pull-knative-client-go-coverage` to re-run this coverage report

File | Old Coverage | New Coverage | Delta
---- |:------------:|:------------:|:-----:
[cmd/kn/main.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_client/1357/pull-knative-client-go-coverage/1409640596657672192/artifacts/line-cov.html#file0) | 74.6% | 91.7% | 17.0

knative-prow-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/client/pull/1357#" title="Author self-approved">dsimansk</a>*, *<a href="https://github.com/knative/client/pull/1357#pullrequestreview-694664211" title="Approved">rhuss</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative%2Fclient).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/knative/client/blob/main/OWNERS)~~ [dsimansk,rhuss]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->

Patch:
diff --git a/cmd/kn/main.go b/cmd/kn/main.go
index 49d23915..064570b2 100644
--- a/cmd/kn/main.go
+++ b/cmd/kn/main.go
@@ -44,12 +44,7 @@ func main() {
 		}),
 	)
 
-	err := run(os.Args[1:])
-	if err != nil && len(os.Args) > 1 {
-		printError(err)
-		// This is the only point from where to exit when an error occurs
-		os.Exit(1)
-	}
+	os.Exit(runWithExit(os.Args[1:]))
 }
 
 // runError is used when during the execution of a command/plugin an error occurs and
@@ -63,6 +58,14 @@ func (e *runError) Error() string {
 	return e.err.Error()
 }
 
+func runWithExit(args []string) int {
+	if err := run(args); err != nil {
+		printError(err)
+		return 1
+	}
+	return 0
+}
+
 // Run the main program. Args are the args as given on the command line (excluding the program name itself)
 func run(args []string) error {
 	// Parse config & plugin flags early to read in configuration file


Test patch:
diff --git a/cmd/kn/main_test.go b/cmd/kn/main_test.go
index 245cb664..d7c8cd35 100644
--- a/cmd/kn/main_test.go
+++ b/cmd/kn/main_test.go
@@ -21,6 +21,8 @@ import (
 	"strings"
 	"testing"
 
+	"knative.dev/client/pkg/kn/plugin"
+
 	"github.com/spf13/cobra"
 	"gotest.tools/v3/assert"
 
@@ -238,7 +240,7 @@ func TestStripFlags(t *testing.T) {
 	assert.ErrorContains(t, err, "needs an argument")
 }
 
-func TestRunWithError(t *testing.T) {
+func TestPrintError(t *testing.T) {
 	data := []struct {
 		given    string
 		expected string
@@ -286,18 +288,151 @@ func TestRunWithPluginError(t *testing.T) {
 	}
 }
 
-// Smoke test
+func TestRunWithExit(t *testing.T) {
+	oldArgs := os.Args
+	defer (func() {
+		os.Args = oldArgs
+	})()
+	testCases := []struct {
+		args           []string
+		expectedOut    []string
+		expectedErrOut []string
+		exitCode       int
+	}{
+		{
+			[]string{"kn", "version"},
+			[]string{"version", "build", "git"},
+			[]string{""},
+			0,
+		},
+		{
+			[]string{"kn", "non-existing"},
+			[]string{""},
+			[]string{"unknown", "command"},
+			1,
+		},
+		{
+			[]string{"kn", "service", "foo"},
+			[]string{""},
+			[]string{"unknown", "sub-command"},
+			1,
+		},
+		{
+			[]string{"kn", "service", "create", "foo", "--foo"},
+			[]string{""},
+			[]string{"unknown", "flag"},
+			1,
+		},
+	}
+	for _, tc := range testCases {
+		capture := test.CaptureOutput(t)
+		os.Args = tc.args
+		exitCode := runWithExit(tc.args[1:])
+		out, errOut := capture.Close()
+		assert.Equal(t, exitCode, tc.exitCode)
+		assert.Assert(t, util.ContainsAllIgnoreCase(out, tc.expectedOut...))
+		assert.Assert(t, util.ContainsAllIgnoreCase(errOut, tc.expectedErrOut...))
+	}
+}
+
+type internalPlugin struct {
+	executeError func() error
+	commandParts []string
+}
+
+func (p internalPlugin) CommandParts() []string       { return p.commandParts }
+func (p internalPlugin) Name() string                 { return "" }
+func (p internalPlugin) Execute(args []string) error  { return p.executeError() }
+func (p internalPlugin) Description() (string, error) { return "", nil }
+func (p internalPlugin) Path() string                 { return "" }
+
 func TestRun(t *testing.T) {
 	oldArgs := os.Args
-	os.Args = []string{"kn", "--config", "/no/config/please.yaml", "version"}
 	defer (func() {
 		os.Args = oldArgs
 	})()
 
-	capture := test.CaptureOutput(t)
-	err := run(os.Args[1:])
-	out, _ := capture.Close()
+	testCases := []struct {
+		args           []string
+		expectedOut    []string
+		expectedErrOut []string
+		plugin         plugin.Plugin
+	}{
+		{
+			[]string{"kn", "version"},
+			[]string{"version", "build", "git"},
+			[]string{},
+			nil,
+		},
+		{
+			[]string{"kn", "non-existing"},
+			[]string{},
+			[]string{"unknown", "command"},
+			nil,
+		},
+		{
+			[]string{"kn", "service", "foo"},
+			[]string{},
+			[]string{"unknown", "sub-command"},
+			nil,
+		},
+		{
+			[]string{"kn", "service", "create", "foo", "--foo"},
+			[]string{},
+			[]string{"unknown", "flag"},
+			nil,
+		},
+		// Internal plugins
+		{
+			[]string{"kn", "foo"},
+			[]string{"OK", "plugin", "out"},
+			[]string{},
 
-	assert.NilError(t, err)
-	assert.Assert(t, util.ContainsAllIgnoreCase(out, "version", "build", "git"))
+			&internalPlugin{
+				executeError: func() error {
+					fmt.Println("OK plugin out")
+					return nil
+				},
+				commandParts: []string{"foo"},
+			},
+		},
+		{
+			[]string{"kn", "service", "create"},
+			[]string{},
+			[]string{"plugin", "overriding", "'service create'"},
+			&internalPlugin{
+				executeError: nil,
+				commandParts: []string{"service", "create"},
+			},
+		},
+		{
+			[]string{"kn", "foo", "bar"},
+			[]string{},
+			[]string{"internal", "plugin", "error"},
+			&internalPlugin{
+				executeError: func() error {
+					return errors.New("internal plugin error")
+				},
+				commandParts: []string{"foo", "bar"},
+			},
+		},
+	}
+	for _, tc := range testCases {
+		os.Args = tc.args
+		if tc.plugin != nil {
+			plugin.InternalPlugins = plugin.PluginList{}
+			plugin.InternalPlugins = append(plugin.InternalPlugins, tc.plugin)
+		}
+		capture := test.CaptureOutput(t)
+		err := run(tc.args[1:])
+		out, _ := capture.Close()
+		if len(tc.expectedErrOut) > 0 {
+			assert.Assert(t, err != nil)
+			assert.Assert(t, util.ContainsAllIgnoreCase(err.Error(), tc.expectedErrOut...))
+		} else {
+			assert.NilError(t, err)
+			assert.Assert(t, util.ContainsAllIgnoreCase(out, tc.expectedOut...))
+		}
+
+	}
 }