fix(#772) refactored / simplified parameters to KnResultCollector

[maximilien]

Description

Simplify and consolidated parameters to KnResultCollector

Changes

• pass testing.T to KnResultCollector
• pass test.KnTest to KnResultCollector
• change functions calls to only using new KnResultCollector parameters

Reference

Fixes #772

/lint

[knative-prow-robot]

@maximilien: 2 warnings.

Details

In response to this:

Description

Simplify and consolidated parameters to KnResultCollector

Changes

• pass testing.T to KnResultCollector
• pass test.KnTest to KnResultCollector
• change functions calls to only using new KnResultCollector parameters

Reference

Fixes #772

/lint

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[maximilien]

/ok-to-test
/assign @navidshaikh

[maximilien]

Please ignore the golint Godoc issues as they are solved in #771 which I can rebase into this when its merged.

cc @navidshaikh

[maximilien]

/test pull-knative-client-integration-tests-latest-release

[maximilien]

Ok to merge @navidshaikh

[navidshaikh]

NewKnTest() will create a namespace for this test.

1. We don't need namespace creation for version test.
2. We dont have tearDown in this test, which leaving behind the created namespace.
   Ran this locally from PR:

➜  client git:(c8c54c52) go test ./test/e2e/ -count=1 -test.v --tags e2e -run TestVersion                                                                        
=== RUN   TestVersion
=== PAUSE TestVersion
=== CONT  TestVersion
--- PASS: TestVersion (5.54s)
PASS
ok  	knative.dev/client/test/e2e	5.544s

➜  client git:(c8c54c52) kubectl get ns|grep kne2etests
kne2etests0                                             Active   56s

If kn doesn't require a namespace to execute, lets not create one? Call to NewKnTest() will create one.

I'd still suggest the earlier review comment fix for version test, IMO no need to define a new method for only version test case.

func TestVersion(t *testing.T) {
  t.Parallel()
  r := test.NewKnRunResultCollector(t, nil)
  defer r.DumpIfFailed()
  out := test.RunKn("", []string{"version"})
  r.AssertNoError(out)
  assert.Check(t, util.ContainsAll(out.Stdout, "Version"))
}

[navidshaikh]

If we go without this, we'll have a namespace left behind from e2e test run which will break running e2e locally against same cluster in next run.

[maximilien]

I think having a way to run w/o namespace is important since remember that this code is now library that can be used outside. I added code to tear down the namespace when the test is done:

+	defer func() {
+		assert.NilError(t, it.Teardown())
+	}()

[maximilien]

Note also that the new code:

// RunNoNamespace the 'kn' CLI with args but no namespace
func (k Kn) RunNoNamespace(args ...string) KnRunResult {
	return RunKn("", args)
}

Is pretty much just a wrapper to make clear we are running kn on default namespace which is a useful thing in general. Keeps the VersionTest consistent with the other tests. We still have various e2e tests that need to be converged to this model so I think it's best to keep the simple VersionTest in line with the others since people will use that as an exemplar.

[navidshaikh]

Well, as test.RunKn is an exported method, this could actually be an example for how to run kn without creating a namespace in tests. Say a plugin does not require a namespace to perform its task, its e2e can refer version tests.

[maximilien]

/retest

[navidshaikh]

/lgtm

Lets get this in. I've added a comment on the version tests discussion about how plugins not requiring namespace to perform their tasks can use test.RunKn.

[navidshaikh]

Well, as test.RunKn is an exported method, this could actually be an example for how to run kn without creating a namespace in tests. Say a plugin does not require a namespace to perform its task, its e2e can refer version tests.

[knative-prow-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: maximilien, navidshaikh

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [maximilien,navidshaikh]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment