feature(testing): Introduce a mock implementation for KnClient #306

rhuss · 2019-07-26T11:13:35Z

Commands must only use the KnClient API and their unit tests should also
only use a mock implementation for this interface.

This commit introduces such a Mock implementation and works like in
the example below for creating a simple service in a synchronous
way

// New mock client
client := knclient.NewMockKnClient(t)

// Recording:
r := client.Recorder()
// Check for existing service --> no
r.GetService("foo", nil, errors.NewNotFound(v1alpha1.Resource("service"), "foo"))
// Create service (don't validate given service --> "Any()" arg is allowed)
r.CreateService(knclient.Any(), nil)
// Wait for service to become ready
r.WaitForService("foo", knclient.Any(), nil)
// Get for showing the URL
r.GetService("foo", getServiceWithUrl("foo", "http://foo.example.com"), nil)

// Testing:
output, err := executeCommand(client, "create", "foo", "--image", "gcr.io/foo/bar:baz")
assert.NilError(t, err)
assert.Assert(t, util.ContainsAll(output, "created", "foo", "http://foo.example.com", "Waiting"))

// Validate that all recorded API methods have been called
r.Validate()

Such tests have three phases:

A recording phase where the mock client is prepared. In this phase a
recorder is called with the expected arguments and the return values
it can return. The arguments can be also functions with
signature func (t *testing.T, actual interface{}, expected interface{})
and will be called to verify a given argument. Such a function should
t.Fail() on its own if the validation fails.
A convenient Any() method is added to allow no validation on an argument
(see example below).
Method can be called multiple times, but the order needs to reflect
the actual calling order
A playback phase where the test executed which in turn calls out to the
Mocks
A validation phase to check the expected output. The recorder can be
also validated to verify that all recorded mock calls has been
used during the test.

See service_create_mock_test.go for a full example.

rhuss · 2019-07-26T11:16:39Z

@maximilien you might be interested in this PR as it would simplify command testing considerably.

rhuss · 2019-07-26T15:13:14Z

pkg/kn/commands/namespaced.go

@@ -57,7 +58,10 @@ func (params *KnParams) GetNamespace(cmd *cobra.Command) (string, error) {
 		var err error
 		namespace, err = params.CurrentNamespace()
 		if err != nil {
-			return "", err
+			if !clientcmd.IsEmptyConfig(err) {


This is PR #305 included here.

maximilien

This is all good. My only concern is that it's a custom Mock. Nothing wrong with that for some specific cases.

But in general I try to avoid custom mocks using tools to generate them instead (when the generated mock is good enough).

The reason is that like any custom supporting code it needs to be maintained and adds to the overall effort in ramping up for new devs. Also auto-generating mocks and doubles forces defined interfaces and nicely defined types.

One mock/doubles generating tool I have used with success is counterfeiter. Also used in CF and other large OSS projects.

maximilien · 2019-07-26T20:26:30Z

/approve
/ok-to-test

knative-prow-robot · 2019-07-26T20:26:38Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [maximilien,rhuss]

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

Commands must only use the `KnClient` API and their unit tests should also only use a mock implementation for this interface. This commit introduces such a Mock implementation and works like in the example below for creating a simple service in a synchronous way ``` // New mock client client := knclient.NewMockKnClient(t) // Recording: r := client.Recorder() // Check for existing service --> no r.GetService("foo", nil, errors.NewNotFound(v1alpha1.Resource("service"), "foo")) // Create service (don't validate given service --> "Any()" arg is allowed) r.CreateService(knclient.Any(), nil) // Wait for service to become ready r.WaitForService("foo", knclient.Any(), nil) // Get for showing the URL r.GetService("foo", getServiceWithUrl("foo", "http://foo.example.com"), nil) // Testing: output, err := executeCommand(client, "create", "foo", "--image", "gcr.io/foo/bar:baz") assert.NilError(t, err) assert.Assert(t, util.ContainsAll(output, "created", "foo", "http://foo.example.com", "Waiting")) // Validate that all recorded API methods have been called r.Validate() ``` Such tests have three phases: * A recording phase where the mock client is prepared. In this phase a recorder is called with the expected arguments and the return values it can return. The arguments can be also functions with signature `func (t *testing.T, actual interface{}, expected interface{})` and will be called to verify a given argument. Such a function should `t.Fail()` on its own if the validation fails. A convenient `Any()` method is added to allow no validation on an argument (see example below). Method can be called multiple times, but the order needs to reflect the actual calling order * A playback phase where the test executed which in turn calls out to the Mocks * A validation phase to check the expected output. The recorder can be also validated to verify that all recorded mock calls has been used during the test. See `service_create_mock_test.go` for a full example.

knative-metrics-robot · 2019-07-27T04:53:38Z

The following is the coverage report on pkg/.
Say /test pull-knative-client-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/serving/v1alpha1/client_mock.go	Do not exist	96.0%

rhuss · 2019-07-27T05:00:53Z

The reasons why I decided against a Mock framework (GoMock, counterfeiter, pegomock, ...) are:

I wanted to avoid code generation
The simplest implementation possible which suites our needs

wrt/ to maintenance, yes its a bit more then calling a code generator but not so much as that's all you have to add for a new KnClient method:

// Get a revision by name
func (r *Recorder) GetRevision(name interface{}, revision *v1alpha1.Revision, err error) {
	r.add("GetRevision", apiMethodCall{[]interface{}{name}, []interface{}{revision, err}})
}

func (c *MockKnClient) GetRevision(name string) (*v1alpha1.Revision, error) {
	call := c.getCall("GetRevision")
	c.verifyArgs(call, name)
	return call.result[0].(*v1alpha1.Revision), errorOrNil(call.result[1])
}

As mentioned, simplicity was the main driving factor (which makes up also for a simplified call to the recording method which combines expected arguments and return values in a single argument list.

Could be that's too simplistic. If so, we, of course, switch over to a full Mock framework.

rhuss · 2019-07-30T11:33:50Z

/retest

maximilien · 2019-07-30T20:07:37Z

I am not going to oppose this but my vote would be using a mock framework. Specifically because it's less custom code to maintain.

However, since we have not used any mock framework so far in this project, the decision of using a framework is heavier than a custom mock...hence why I'd compromise with your solution. I want to be easy 🏖 and trust your judgement here.

👍 this message and I'll lgtm. Cheers 🍻

rhuss · 2019-07-30T20:43:33Z

Some other benefits of using that kind of mocks for testing commands:

If we want to offer a KnClient API as stated as a goal, offering a typed test API is a plus for users of this API.
Using that Mock interface exclusively in the Kn commands unit tests, we have a clear separation and also an indication that no command uses the plain Knative Apis directly but only via the KnClient abstraction.

The Mock API is kept deliberately very simplistic to make it easy (even trivial) to extend or to adapt it.
However, I totally agree with @sixolet that an excellent test coverage of the KnClient implementation is essential if we not only want to rely on the integration tests.

maximilien · 2019-07-31T00:28:39Z

/lgtm

…ve#306) * feature(testing): Introduce a Mock implementation for KnClient Commands must only use the `KnClient` API and their unit tests should also only use a mock implementation for this interface. This commit introduces such a Mock implementation and works like in the example below for creating a simple service in a synchronous way ``` // New mock client client := knclient.NewMockKnClient(t) // Recording: r := client.Recorder() // Check for existing service --> no r.GetService("foo", nil, errors.NewNotFound(v1alpha1.Resource("service"), "foo")) // Create service (don't validate given service --> "Any()" arg is allowed) r.CreateService(knclient.Any(), nil) // Wait for service to become ready r.WaitForService("foo", knclient.Any(), nil) // Get for showing the URL r.GetService("foo", getServiceWithUrl("foo", "http://foo.example.com"), nil) // Testing: output, err := executeCommand(client, "create", "foo", "--image", "gcr.io/foo/bar:baz") assert.NilError(t, err) assert.Assert(t, util.ContainsAll(output, "created", "foo", "http://foo.example.com", "Waiting")) // Validate that all recorded API methods have been called r.Validate() ``` Such tests have three phases: * A recording phase where the mock client is prepared. In this phase a recorder is called with the expected arguments and the return values it can return. The arguments can be also functions with signature `func (t *testing.T, actual interface{}, expected interface{})` and will be called to verify a given argument. Such a function should `t.Fail()` on its own if the validation fails. A convenient `Any()` method is added to allow no validation on an argument (see example below). Method can be called multiple times, but the order needs to reflect the actual calling order * A playback phase where the test executed which in turn calls out to the Mocks * A validation phase to check the expected output. The recorder can be also validated to verify that all recorded mock calls has been used during the test. See `service_create_mock_test.go` for a full example. * chore: Test the mock client * chore: Minor fixes * chore: Cosmetic fixes

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Jul 26, 2019

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 26, 2019

knative-prow-robot requested review from maximilien and sixolet July 26, 2019 11:13

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 26, 2019

rhuss changed the title ~~feature(testing): Introduce a Mock implementation for KnClient~~ feature(testing): Introduce a mock implementation for KnClient Jul 26, 2019

rhuss commented Jul 26, 2019

View reviewed changes

maximilien reviewed Jul 26, 2019

View reviewed changes

knative-prow-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Jul 26, 2019

rhuss added 4 commits July 27, 2019 06:50

chore: Test the mock client

787664e

chore: Minor fixes

1da5fc9

chore: Cosmetic fixes

8c6ebfc

rhuss force-pushed the pr/mock-client branch from e704a89 to 8c6ebfc Compare July 27, 2019 04:52

knative-prow-robot assigned maximilien Jul 31, 2019

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 31, 2019

knative-prow-robot merged commit 71c39e8 into knative:master Jul 31, 2019

maximilien mentioned this pull request Aug 7, 2019

Add --dry-run option to service create and update operations #293

Closed

coryrc pushed a commit to coryrc/client that referenced this pull request May 14, 2020

Add knative/observability to tide list (knative#306)

9e018e3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature(testing): Introduce a mock implementation for KnClient #306

feature(testing): Introduce a mock implementation for KnClient #306

rhuss commented Jul 26, 2019 •

edited

Loading

rhuss commented Jul 26, 2019

rhuss Jul 26, 2019

maximilien left a comment •

edited

Loading

maximilien commented Jul 26, 2019

knative-prow-robot commented Jul 26, 2019

knative-metrics-robot commented Jul 27, 2019

rhuss commented Jul 27, 2019 •

edited

Loading

rhuss commented Jul 30, 2019

maximilien commented Jul 30, 2019 •

edited

Loading

rhuss commented Jul 30, 2019 •

edited

Loading

maximilien commented Jul 31, 2019

feature(testing): Introduce a mock implementation for KnClient #306

feature(testing): Introduce a mock implementation for KnClient #306

Conversation

rhuss commented Jul 26, 2019 • edited Loading

rhuss commented Jul 26, 2019

rhuss Jul 26, 2019

Choose a reason for hiding this comment

maximilien left a comment • edited Loading

Choose a reason for hiding this comment

maximilien commented Jul 26, 2019

knative-prow-robot commented Jul 26, 2019

knative-metrics-robot commented Jul 27, 2019

rhuss commented Jul 27, 2019 • edited Loading

rhuss commented Jul 30, 2019

maximilien commented Jul 30, 2019 • edited Loading

rhuss commented Jul 30, 2019 • edited Loading

maximilien commented Jul 31, 2019

rhuss commented Jul 26, 2019 •

edited

Loading

maximilien left a comment •

edited

Loading

rhuss commented Jul 27, 2019 •

edited

Loading

maximilien commented Jul 30, 2019 •

edited

Loading

rhuss commented Jul 30, 2019 •

edited

Loading