Istioctl Go tests for get/create/update/delete #5734

esnible · 2018-05-21T14:14:38Z

This creates tests on the output of istioctl get x (also delete and a bit of create/update.)

The implementation uses memory.Make() to create a model.ConfigStore and uses it as a mock.

This doesn't fix any issues but I wanted to get it in place so that #5668 , #5538 , and #5502 will have test cases and code coverage.

codecov · 2018-05-21T14:45:29Z

Codecov Report

Merging #5734 into release-0.8 will decrease coverage by 2%.
The diff coverage is 65%.

@@              Coverage Diff              @@
##           release-0.8   #5734     +/-   ##
=============================================
- Coverage           73%     72%     -1%     
=============================================
  Files              322     330      +8     
  Lines            27576   28713   +1137     
=============================================
+ Hits             20027   20442    +415     
- Misses            6754    7441    +687     
- Partials           795     830     +35

Impacted Files	Coverage Δ
pilot/pkg/config/kube/crd/client.go	`1% <ø> (ø)`	⬆️
istioctl/cmd/istioctl/main.go	`36% <65%> (ø)`
...ilot/pkg/networking/plugin/authn/authentication.go	`68% <0%> (-15%)`	⬇️
mixer/adapter/stackdriver/stackdriver.go	`34% <0%> (-14%)`	⬇️
galley/pkg/resource/accessor.go	`92% <0%> (-2%)`	⬇️
mixer/adapter/kubernetesenv/kubernetesenv.go	`68% <0%> (-2%)`	⬇️
mixer/pkg/runtime/validator/validator.go	`74% <0%> (-1%)`	⬇️
mixer/adapter/statsd/statsd.go	`96% <0%> (-1%)`	⬇️
mixer/adapter/prometheus/server.go	`95% <0%> (-1%)`	⬇️
pilot/pkg/config/monitor/copilot_snapshot.go	`98% <0%> (ø)`	⬇️
... and 42 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d91d44e...73838f6. Read the comment docs.

ayj · 2018-05-21T16:45:20Z

istioctl/cmd/istioctl/istioctl_test.go

+				}
+				return nil
+			})
+


If possible, it would be good to invoke the subcommand via Execute to verify we've setup PersistentPreRunE correct. e.g.

rootCmd.SetArgs(c.args) rootCmd.Execute()

ayj · 2018-05-21T16:47:06Z

istioctl/cmd/istioctl/istioctl_test.go

+}
+
+// captureOutput invokes f capturing the output sent to stderr and stdout
+func captureOutput(f func() error) (string, string, error) {


Could we capture cobra output more directly using the SetOutput helper function?

var out bytes.Buffer rootCmd.SetOutput(out)

esnible · 2018-05-21T19:08:18Z

@ayj I am now using Execute() to run and SetOutput() to obtain stderr messages. I am still using os.Stdout redirection to capture normal output because istioctl writes directly to os.Stdout and I could not find a way to use Cobra to capture that output.

esnible · 2018-05-22T01:01:17Z

/retest

ayj · 2018-05-22T15:51:25Z

istioctl/cmd/istioctl/istioctl_test.go

+	capturedOutput := <-outChannel
+
+	return capturedOutput, errF
+}


A quick google search for this pattern indicates you may want to start the goroutine before invoking the test function (see here). The golang testing example code might be a good reference (see https://golang.org/src/testing/example.go#L60).

Also, I'm not sure how capturing stdio will work if tests are later run in parallel. It might be good to convert istioctl functions over to use explicit printf and fatalf interface functions like Mixer and Galley code. We could then redirect that to cobra.Print or provide our own fake to explicit capture all stdout and stderr.

ayj · 2018-05-22T15:51:46Z

istioctl/cmd/istioctl/istioctl_test.go

+
+		stdOutput, fErr := captureStdout(
+			func() error {
+				rootCmd.SetArgs(c.args)


Why is rootCmd.SetArgs(c.args) called twice?

ayj · 2018-05-22T17:51:01Z

/lgtm

esnible · 2018-05-22T19:12:42Z

/test e2e-pilot

…tl_tests

istio-testing · 2018-05-31T15:27:59Z

New changes are detected. LGTM label has been removed.

istio-testing · 2018-05-31T15:28:03Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ayj, esnible
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: douglas-reid

Assign the PR to them by writing /assign @douglas-reid in a comment when ready.

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

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

istio-testing · 2018-05-31T15:28:05Z

@esnible: PR needs rebase.

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.

googlebot · 2018-05-31T15:28:22Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

istio-testing · 2018-05-31T15:28:59Z

@esnible: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
prow/istio-unit-tests.sh	`73838f6`	link	`/test istio-unit-tests`
prow/istio-presubmit.sh	`73838f6`	link	`/test istio-presubmit`

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. I understand the commands that are listed here.

esnible · 2018-05-31T15:56:52Z

@ayj Now that Master is open I retargeted this for 1.0. cla/google is failing and the presubmit can't auto-merge. Is it reasonable to just abandon this and cut and paste the changes into a fresh PR?

Go tests for 'istioctl get'

0b89715

googlebot added the cla: yes label May 21, 2018

esnible requested a review from ayj May 21, 2018 14:14

Uncomment Uri matches

c0239db

esnible changed the title ~~Istioctl Go tests tests~~ WIP Istioctl Go tests tests May 21, 2018

Test cases for create/replace/delete

e07b588

esnible changed the title ~~WIP Istioctl Go tests tests~~ Istioctl Go tests for get/create/update/delete May 21, 2018

ayj reviewed May 21, 2018

View reviewed changes

Capture stderr with SetOutput

f113ca9

ayj reviewed May 22, 2018

View reviewed changes

Send all output through buffers

e2f6a14

istio-testing assigned ayj May 22, 2018

istio-testing added lgtm labels May 22, 2018

ayj approved these changes May 22, 2018

View reviewed changes

esnible added 6 commits May 31, 2018 11:27

Go tests for 'istioctl get'

50971ca

Uncomment Uri matches

1cf9a19

Test cases for create/replace/delete

18ae496

Capture stderr with SetOutput

128954c

Send all output through buffers

78582f2

Merge branch 'istioctl_tests' of github.com:esnible/istio into istioc…

73838f6

…tl_tests

istio-testing removed the lgtm label May 31, 2018

istio-testing added needs-rebase Indicates a PR needs to be rebased before being merged and removed approved labels May 31, 2018

googlebot added cla: no Set by the Google CLA bot to indicate the author of a PR has not signed the Google CLA. and removed cla: yes labels May 31, 2018

esnible changed the base branch from release-0.8 to master May 31, 2018 15:33

esnible mentioned this pull request May 31, 2018

Tests for istioctl get/delete/etc #5949

Merged

esnible closed this May 31, 2018

esnible deleted the istioctl_tests branch May 31, 2018 19:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Istioctl Go tests for get/create/update/delete #5734

Istioctl Go tests for get/create/update/delete #5734

Uh oh!

esnible commented May 21, 2018 •

edited

Loading

Uh oh!

codecov bot commented May 21, 2018 •

edited

Loading

Uh oh!

ayj May 21, 2018

Uh oh!

ayj May 21, 2018

Uh oh!

esnible commented May 21, 2018

Uh oh!

esnible commented May 22, 2018

Uh oh!

ayj May 22, 2018

Uh oh!

ayj May 22, 2018

Uh oh!

ayj commented May 22, 2018

Uh oh!

esnible commented May 22, 2018

Uh oh!

istio-testing commented May 31, 2018

Uh oh!

istio-testing commented May 31, 2018

Uh oh!

istio-testing commented May 31, 2018

Uh oh!

googlebot commented May 31, 2018

Uh oh!

istio-testing commented May 31, 2018

Uh oh!

esnible commented May 31, 2018

Uh oh!

Uh oh!

Istioctl Go tests for get/create/update/delete #5734

Istioctl Go tests for get/create/update/delete #5734

Uh oh!

Conversation

esnible commented May 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented May 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ayj May 21, 2018

Choose a reason for hiding this comment

Uh oh!

ayj May 21, 2018

Choose a reason for hiding this comment

Uh oh!

esnible commented May 21, 2018

Uh oh!

esnible commented May 22, 2018

Uh oh!

ayj May 22, 2018

Choose a reason for hiding this comment

Uh oh!

ayj May 22, 2018

Choose a reason for hiding this comment

Uh oh!

ayj commented May 22, 2018

Uh oh!

esnible commented May 22, 2018

Uh oh!

istio-testing commented May 31, 2018

Uh oh!

istio-testing commented May 31, 2018

Uh oh!

istio-testing commented May 31, 2018

Uh oh!

googlebot commented May 31, 2018

Uh oh!

istio-testing commented May 31, 2018

Uh oh!

esnible commented May 31, 2018

Uh oh!

Uh oh!

esnible commented May 21, 2018 •

edited

Loading

codecov bot commented May 21, 2018 •

edited

Loading