Add e2e tests to verify mixer report and check for ingress #3513

bianpengyuan · 2018-02-15T07:06:34Z

This test is duplicated from globalcheckandreport with ingress as destination in request count query.

istio-testing · 2018-02-15T07:06:42Z

Hi @bianpengyuan. Thanks for your PR.

I'm waiting for a istio member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

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.

bianpengyuan · 2018-02-15T20:40:24Z

/assign @douglas-reid

douglas-reid · 2018-02-16T04:49:43Z

/ok-to-test

douglas-reid · 2018-02-16T04:51:35Z

tests/e2e/tests/mixer/mixer_test.go

+	}
+	allowPrometheusSync()
+
+	log.Info("Successfully sent request(s) to /productpage; checking metrics...")


instead of weaving together t.Log and log.Info statements, should we have consistency here of always using t.Log?

ldemailly · 2018-02-18T20:05:40Z

/test e2e-suite-rbac-auth

ldemailly · 2018-02-19T01:06:50Z

filled the unrelated failure issue #3600

mandarjog · 2018-02-19T18:55:05Z

tests/e2e/tests/mixer/mixer_test.go

+	t.Logf("Baseline established: prior200s = %f", prior200s)
+	t.Log("Visiting product page...")
+
+	// visit production page as well as ingress.


nit: Visit product page through ingress.

mandarjog · 2018-02-19T18:57:47Z

@bianpengyuan Thanks for the PR.

This just adds report verification, we also need check.
Can you refactor code so that there is common code between globalcheckandreport and IngressCheckAndReport instead of copying?

bianpengyuan · 2018-02-22T18:10:55Z

@mandarjog Thanks for reviewing and sorry for the delay.

An ingress denial rule check test has been added for ingress. Let me know if this is enough or I need to add more for testing check.
For metric report, I extracted out the same code into a method. I am having a hard time to come up an elegant way to construct these tests. Let me know if you have a better idea. Also I have changed GlobalCheckAndReport to just GlobalReport -- there is no check rules at all in it.

mandarjog · 2018-02-22T18:13:34Z

tests/e2e/tests/mixer/mixer_test.go

@@ -414,19 +425,25 @@ func TestNewMetrics(t *testing.T) {
 }

 func TestDenials(t *testing.T) {
+	testDenials(t, denialRule)


Please add 2 top level tests func TestIngressDenial and func TestDenial.
This way there is better visibility about what exactly failed.

mandarjog · 2018-02-22T18:14:26Z

tests/e2e/tests/mixer/mixer_test.go

@@ -275,20 +285,21 @@ func TestGlobalCheckAndReport(t *testing.T) {
 	t.Logf("Baseline established: prior200s = %f", prior200s)
 	t.Log("Visiting product page...")

+	// visit production page.


nit: product page.
I think you have used production page here and elsewhere.

mandarjog · 2018-02-22T18:15:28Z

tests/e2e/tests/mixer/mixer_test.go

-	// establish baseline
-	t.Log("Establishing metrics baseline for test...")
-	query := fmt.Sprintf("istio_request_count{%s=\"%s\"}", destLabel, fqdn("productpage"))
+	checkMetricReport(t, promAPI, "productpage")


Similar to check Please add 2 top level tests for TestMetric and TestIngressMetric

mandarjog · 2018-02-22T18:16:31Z

Hey @bianpengyuan apart from my comments everything else looks good.
Using top level test names gives better visibility, thanks.

mandarjog

Looks good

mandarjog · 2018-02-22T18:56:29Z

/lgtm

istio-merge-robot · 2018-02-22T18:56:53Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mandarjog

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

Needs approval from an approver in each of these OWNERS Files:

~~OWNERS~~ [mandarjog]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

mandarjog · 2018-02-22T18:57:29Z

One last thing before merge, please update the PR title and description to reflect what it does now.

istio-merge-robot · 2018-02-22T19:13:54Z

/test all [submit-queue is verifying that this PR is safe to merge]

istio-testing · 2018-02-22T19:16:32Z

@bianpengyuan: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
prow/e2e-suite-rbac-auth.sh	`71ec871`	link	`/test e2e-suite-rbac-auth`

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.

istio-merge-robot · 2018-02-22T19:37:04Z

Automatic merge from submit-queue.

bianpengyuan added 2 commits February 14, 2018 22:54

Add a mixer e2e test for ingress metric report.

8eff289

Add comment.

14074f0

bianpengyuan requested a review from a team February 15, 2018 07:06

googlebot added the cla: yes label Feb 15, 2018

istio-testing added the needs-ok-to-test label Feb 15, 2018

Fix lint.

71ec871

istio-testing assigned douglas-reid Feb 15, 2018

douglas-reid assigned mandarjog Feb 16, 2018

istio-testing removed the needs-ok-to-test label Feb 16, 2018

douglas-reid reviewed Feb 16, 2018

View reviewed changes

Update log method.

43542fe

mandarjog reviewed Feb 19, 2018

View reviewed changes

Update tests, add a denial test for ingress.

dcae3b1

bianpengyuan requested a review from a team February 22, 2018 18:02

mandarjog reviewed Feb 22, 2018

View reviewed changes

bianpengyuan added 2 commits February 22, 2018 10:34

Use top level tests, fix nit.

0f976ae

Fix lint.

5971356

mandarjog approved these changes Feb 22, 2018

View reviewed changes

istio-testing added the lgtm label Feb 22, 2018

istio-merge-robot added the approved label Feb 22, 2018

bianpengyuan changed the title ~~Add a mixer e2e test for ingress metric report.~~ Add e2e tests to verify mixer report and check for ingress Feb 22, 2018

istio-merge-robot merged commit 4f98ea3 into istio:master Feb 22, 2018

Add e2e tests to verify mixer report and check for ingress #3513

Add e2e tests to verify mixer report and check for ingress #3513

Uh oh!

Conversation

bianpengyuan commented Feb 15, 2018

Uh oh!

istio-testing commented Feb 15, 2018

Uh oh!

bianpengyuan commented Feb 15, 2018

Uh oh!

douglas-reid commented Feb 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ldemailly commented Feb 18, 2018

Uh oh!

ldemailly commented Feb 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mandarjog commented Feb 19, 2018

Uh oh!

bianpengyuan commented Feb 22, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mandarjog commented Feb 22, 2018

Uh oh!

mandarjog left a comment

Choose a reason for hiding this comment

Uh oh!

mandarjog commented Feb 22, 2018

Uh oh!

istio-merge-robot commented Feb 22, 2018

Uh oh!

mandarjog commented Feb 22, 2018

Uh oh!

istio-merge-robot commented Feb 22, 2018

Uh oh!

istio-testing commented Feb 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

istio-merge-robot commented Feb 22, 2018

Uh oh!

Uh oh!

istio-testing commented Feb 22, 2018 •

edited

Loading