fix(service update): Print success message after successful update #169

rhuss · 2019-06-07T13:42:23Z

No description provided.

rhuss · 2019-06-07T13:56:02Z

Hmm, I really don't understand how test coverage could decrease with this PR:

I added 1 line and removed 2 in code
The new line is always teste also by existing tests
I added a new assertion (but that doesn't count to coverage, I know)

But no idea how @knative-metrics-robots measure coverage. Nevertheless, I question the result.

navidshaikh

LGTM

rhuss · 2019-06-07T15:15:33Z

But no idea how @knative-metrics-robots measure coverage. Nevertheless, I question the result.

Ok, I see. Because I removed also two lines which were already covered, therefore the coverage dropped

c        c-2
--  >    ----
t        t-2

is true as along t > c.

cppforlife · 2019-06-10T18:06:44Z

pkg/kn/commands/service/service_update.go

@@ -55,19 +56,19 @@ func NewServiceUpdateCommand(p *commands.KnParams) *cobra.Command {
 			}
 			service = service.DeepCopy()

-			err = editFlags.Apply(service, cmd)
-			if err != nil {
+			if err = editFlags.Apply(service, cmd); err != nil {


dont think we are switching to the collapsed style judging by the rest of the code.

fair point, shouldn't touch existing code for good (aka better) reasons.

cppforlife · 2019-06-10T18:07:04Z

pkg/kn/commands/service/service_update_test.go

@@ -108,6 +109,10 @@ func TestServiceUpdateImage(t *testing.T) {
 	} else if template.Spec.DeprecatedContainer.Image != "gcr.io/foo/quux:xyzzy" {
 		t.Fatalf("wrong image set: %v", template.Spec.DeprecatedContainer.Image)
 	}
+
+	if !strings.Contains(output, "update") {


might as well test fuller message.

will anchor on some more keywords here.

@rhuss why no just expect what is being printed "Service '...' updated in namespace ..."?

Because I want to focus on the important context provided by an error message, not on the exact wording. That's why I don't do == assert on error/success messages. That also helps me in focussing on what should be contained in good messages:

All context available at that point in code (i.e. the input parameters of the method)

A suggestion how to get over an error (if its an error message)

Keyword of what has happened, reflecting the action performed ("update").

Side effect: Your tests become more robust as they don't depend on the exact wording and whether there is a comma or not (except you insist on that). In TDD world you would first write the test with the things you would expect to be included in the message, and then write the message. Not write the message first and just blindly check for it (that test doesn't hasn't much expressiveness anyway)

@rhuss one of the important aspects of a test is readability. personally i dont find following code readable because its intent is not clear. it also ends up more fragile as it may produce false positives.

if !strings.Contains(strings.ToLower(output), "update") || !strings.Contains(output, "foo") || !strings.Contains(strings.ToLower(output), "service") || !strings.Contains(strings.ToLower(output), "namespace") || !strings.Contains(output, "bar") { t.Fatalf("wrong or no success message: %s", output) }

however preferred style of these types of assertions is quite subjective so i wont dwell on it too much.

knative-metrics-robot · 2019-06-10T20:53:47Z

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/kn/commands/service/service_update.go	75.0%	76.0%	1.0

rhuss · 2019-06-14T07:37:29Z

@cppforlife Anything else missing for this PR to get merged ?

cppforlife · 2019-06-17T19:07:16Z

/lgtm
/approve

knative-prow-robot · 2019-06-17T19:07:24Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cppforlife, navidshaikh, 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~~ [cppforlife]

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

…native#169) * fix(service update): Print success message after successful update * chore(service update): Improve test, revert unneeded changes

Implements Kn plugins re-using some code from kubectl plugins. This is WIP to allow concrete discussions and testing and hopefully also allow sub tasks to be created and worked on in parallel. This version contains the following: 1. wraps the main root Kn command to support plugin 2. plugins are any executable in PATH with name kn-* 3. 'kn plugins list' to list found kn plugins 4. skips any kn plugins found with name that match core commands, e.g., kn-service would be ignored 5. execute any valid kn plugins found And is missing: 1. unit and integration tests 2. plugin install command 3. plugin repository command 4. plugin / Knative server version negotiation 5. anything else we agree on in plugin req doc Adds a Golang version of e2e Basic workflow that can execute locally (knative#121) 1. creates a namespace 'kne2etests' (default name, change with env KN_E2E_NAMESPACE) 2. executes kn commands as per Basic workflow doc in said namespace 3. verifies each command's output 4. deletes the 'kne2etests' namespace Fix typo in README.md (knative#181) Renames service 'get' to 'list' (knative#179) * Renames service 'get' to 'list' As per title. * Renames get references to list in e2e tests * Replaces get to list in docs Renames revision 'get' to 'list' (knative#180) * Renames revision 'get' to 'list' As per title. * Renames revision get references to revision list in docs feat(build.sh) - Add mandatory license check (knative#187) We could decide to make that optional, too, if this is coming into the way. Chore: Fix typos in docs (knative#178) fix(service update): Print success message after successful update (knative#169) * fix(service update): Print success message after successful update * chore(service update): Improve test, revert unneeded changes [WIP] Kn plugins implementation Implements Kn plugins re-using some code from kubectl plugins. This is WIP to allow concrete discussions and testing and hopefully also allow sub tasks to be created and worked on in parallel. This version contains the following: 1. wraps the main root Kn command to support plugin 2. plugins are any executable in PATH with name kn-* 3. 'kn plugins list' to list found kn plugins 4. skips any kn plugins found with name that match core commands, e.g., kn-service would be ignored 5. execute any valid kn plugins found 6. started adding UTs for plugin commands and files 7. rebased with latest HEAD And is missing: 1. integration tests 2. plugin install command 3. plugin repository command 4. plugin / Knative server version negotiation 5. anything else we agree on in plugin req doc

)

fix(service update): Print success message after successful update

9bce15b

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

knative-prow-robot requested review from evankanderson and sixolet June 7, 2019 13:42

knative-prow-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jun 7, 2019

navidshaikh approved these changes Jun 7, 2019

View reviewed changes

cppforlife reviewed Jun 10, 2019

View reviewed changes

chore(service update): Improve test, revert unneeded changes

d1baaa0

rhuss mentioned this pull request Jun 14, 2019

Adds a Ginkgo-style unit test to kn version command #185

Closed

knative-prow-robot assigned cppforlife Jun 17, 2019

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

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

knative-prow-robot merged commit 8a1d412 into knative:master Jun 17, 2019

rhuss deleted the pr/update-success-message branch March 9, 2021 10:32

dsimansk pushed a commit to dsimansk/client that referenced this pull request Oct 12, 2023

clustertask patch for devconsole create serverless function (knative#169

810b65b

)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(service update): Print success message after successful update #169

fix(service update): Print success message after successful update #169

rhuss commented Jun 7, 2019

rhuss commented Jun 7, 2019

navidshaikh left a comment

rhuss commented Jun 7, 2019 •

edited

Loading

cppforlife Jun 10, 2019

rhuss Jun 10, 2019

cppforlife Jun 10, 2019

rhuss Jun 10, 2019

cppforlife Jun 11, 2019

rhuss Jun 12, 2019 •

edited

Loading

cppforlife Jun 17, 2019

knative-metrics-robot commented Jun 10, 2019

rhuss commented Jun 14, 2019 •

edited

Loading

cppforlife commented Jun 17, 2019

knative-prow-robot commented Jun 17, 2019

fix(service update): Print success message after successful update #169

fix(service update): Print success message after successful update #169

Conversation

rhuss commented Jun 7, 2019

rhuss commented Jun 7, 2019

navidshaikh left a comment

Choose a reason for hiding this comment

rhuss commented Jun 7, 2019 • edited Loading

cppforlife Jun 10, 2019

Choose a reason for hiding this comment

rhuss Jun 10, 2019

Choose a reason for hiding this comment

cppforlife Jun 10, 2019

Choose a reason for hiding this comment

rhuss Jun 10, 2019

Choose a reason for hiding this comment

cppforlife Jun 11, 2019

Choose a reason for hiding this comment

rhuss Jun 12, 2019 • edited Loading

Choose a reason for hiding this comment

cppforlife Jun 17, 2019

Choose a reason for hiding this comment

knative-metrics-robot commented Jun 10, 2019

rhuss commented Jun 14, 2019 • edited Loading

cppforlife commented Jun 17, 2019

knative-prow-robot commented Jun 17, 2019

rhuss commented Jun 7, 2019 •

edited

Loading

rhuss Jun 12, 2019 •

edited

Loading

rhuss commented Jun 14, 2019 •

edited

Loading