enhance unit tests of advance audit feature #50842

CaoShuFeng · 2017-08-17T12:43:39Z

This change addresses comments from @crassirostris
#49115 (comment)

It does three things:

use auditinternal for unit test in filter stage
add a seperate unit test for Audit-ID http header
add unit test for audit log backend

Release note:

NONE

CaoShuFeng · 2017-08-17T12:54:39Z

staging/src/k8s.io/apiserver/plugin/pkg/audit/log/backend_test.go

+			SourceIPs: []string{
+				"127.0.0.1",
+			},
+			// Uncomment the next line this test would fail:


Please take a look at here.
I think this is OK, and it's not an issue for me.

time.Truncate?

Yeap, time.Truncate runs happliy.

liggitt · 2017-08-17T15:50:20Z

/unassign

CaoShuFeng · 2017-08-18T02:00:30Z

/test pull-kubernetes-e2e-gce-etcd3

crassirostris

LGTM, couple of nits

crassirostris · 2017-08-18T14:55:36Z

staging/src/k8s.io/apiserver/pkg/endpoints/filters/audit_test.go

-		if len(line) != len(test.expected) {
-			t.Errorf("[%s] Unexpected amount of lines in audit log: %d", test.desc, len(line))
+		if len(events) != len(test.expected) {
+			t.Errorf("[%s] Unexpected amount of lines in audit log: %d", test.desc, len(events))


If this is Errorf, the execution will continue and line event := events[i] can panic with index out of range

There is a "continue" in the next line.

After using t.Run(), "continue" can't be used.
Changed to t.Fatal().

crassirostris · 2017-08-18T15:01:59Z

staging/src/k8s.io/apiserver/pkg/endpoints/filters/audit_test.go

+		resp := w.Result()
+		if test.expectedHeader {
+			if resp.Header.Get("Audit-ID") == "" {
+				t.Errorf("[%s] expected Audit-ID http header returned, but not returned", test.desc)


Instead of putting [%s] in front of each string, you can use the following approach:

for _, tc := range tcs { t.Run(tc.desc, func() { ... }) }

Then you can use t.Failf to fail the test without continuation

Then you can use t.Failf to fail the test without continuation

Done.

@CaoShuFeng Haven't pushed the changes?

@CaoShuFeng Haven't pushed the changes?

I mean this is done in line 621.
I other places, we can use t.Errorf() to continue the tests to get more error messages. What do you think?

But this particular place has continue afterwards :)

Anyway, if there's no kubernetes-wide policy about tests, do as you wish, that's just my feeling that writing t.Run once is better than writing it in each log line, at least for new tests.

crassirostris · 2017-08-18T15:10:45Z

staging/src/k8s.io/apiserver/plugin/pkg/audit/log/backend_test.go

+			SourceIPs: []string{
+				"127.0.0.1",
+			},
+			// Uncomment the next line this test would fail:


time.Truncate?

CaoShuFeng · 2017-08-20T04:10:30Z

/test pull-kubernetes-kubemark-e2e-gce

CaoShuFeng · 2017-08-20T04:38:15Z

/test pull-kubernetes-kubemark-e2e-gce

CaoShuFeng · 2017-08-20T06:53:02Z

/test pull-kubernetes-kubemark-e2e-gce

CaoShuFeng · 2017-08-20T06:53:11Z

/test pull-kubernetes-kubemark-e2e-gce

CaoShuFeng · 2017-08-21T06:30:45Z

/test pull-kubernetes-kubemark-e2e-gce
/test pull-kubernetes-e2e-gce-etcd3

CaoShuFeng · 2017-08-21T09:15:13Z

staging/src/k8s.io/apiserver/pkg/endpoints/filters/audit_test.go

+				}
+				if (event.ResponseStatus == nil) != (expect.ResponseStatus == nil) {
+					t.Errorf("Unexpected ResponseStatus: %v", event.ResponseStatus)
+					continue


But this particular place has continue afterwards :)

Do you mean the continue in this line?
I think the continue here is OK.
For example, there are 3 events in test.expected totally. And it fails in line 646 for the first event, so we run continue. And the check in line 650 is skipped, and then we will check the next 2 events.

If we use t.Fatalf(), we will not check other events after the first fatal.
That's my opinion.

Ah, sure, agree, it's fine here, sorry :)

crassirostris · 2017-08-21T09:19:51Z

staging/src/k8s.io/apiserver/pkg/endpoints/filters/audit_test.go

+				}
+				if (event.ResponseStatus == nil) != (expect.ResponseStatus == nil) {
+					t.Errorf("Unexpected ResponseStatus: %v", event.ResponseStatus)
+					continue


Ah, sure, agree, it's fine here, sorry :)

crassirostris · 2017-08-21T09:20:54Z

/retest

CaoShuFeng · 2017-08-22T01:25:58Z

/test pull-kubernetes-kubemark-e2e-gce

soltysh · 2017-09-01T10:56:06Z

/test pull-kubernetes-bazel-build
/test pull-kubernetes-bazel-test

CaoShuFeng · 2017-09-04T14:09:40Z

/test pull-kubernetes-e2e-gce-etcd3

soltysh · 2017-09-04T15:01:14Z

@calebmiles why did you remove the milestone from here, this is improving the test so it falls under the category of bugfixes/stabilization imo

CaoShuFeng · 2017-09-05T02:12:35Z

@calebmiles why did you remove the milestone from here, this is improving the test so it falls under the category of bugfixes/stabilization imo

Another people is pinged.
s/calebmiles/calebamiles
@calebamiles

soltysh · 2017-09-06T12:50:01Z

@jdumars I'm adding a 1.8 milestone back here, since this is fixing tests/improving stability of the code. The only reason it wasn't approved and lgtm-ed on Fri is it needed a rebase.

/lgtm

This change does three things: 1. use auditinternal for unit test in filter stage 2. add a seperate unit test for Audit-ID http header 3. add unit test for audit log backend

soltysh · 2017-09-06T15:48:41Z

/lgtm

soltysh · 2017-09-06T15:49:01Z

@sttts for approval

sttts · 2017-09-06T16:19:26Z

/approve no-issue

k8s-github-robot · 2017-09-06T16:19:54Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CaoShuFeng, soltysh, sttts

Associated issue requirement bypassed by: sttts

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

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

~~staging/src/k8s.io/apiserver/OWNERS~~ [sttts]

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

k8s-github-robot · 2017-09-06T20:36:14Z

Automatic merge from submit-queue (batch tested with PRs 49133, 51557, 51749, 50842, 52018)

@crassirostris

…filters Automatic merge from submit-queue (batch tested with PRs 49133, 51557, 51749, 50842, 52018) enhance unit tests of advance audit feature This change addresses comments from @crassirostris #49115 (comment) It does three things: 1. use auditinternal for unit test in filter stage 2. add a seperate unit test for Audit-ID http header 3. add unit test for audit log backend **Release note**: ``` NONE ```

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 17, 2017

k8s-github-robot assigned liggitt and sttts Aug 17, 2017

k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. release-note-none Denotes a PR that doesn't merit a release note. labels Aug 17, 2017

CaoShuFeng mentioned this pull request Aug 17, 2017

Upgrade advanced audit to version v1beta1 #49115

Merged

CaoShuFeng force-pushed the remove_versioned_test_from_filters branch from f78525f to f88cc75 Compare August 17, 2017 12:53

CaoShuFeng commented Aug 17, 2017

View reviewed changes

k8s-ci-robot unassigned liggitt Aug 17, 2017

crassirostris reviewed Aug 18, 2017

View reviewed changes

CaoShuFeng force-pushed the remove_versioned_test_from_filters branch from f88cc75 to 987b28c Compare August 20, 2017 02:48

CaoShuFeng closed this Aug 20, 2017

CaoShuFeng reopened this Aug 20, 2017

CaoShuFeng commented Aug 21, 2017

View reviewed changes

crassirostris approved these changes Aug 21, 2017

View reviewed changes

ericchiang added the area/audit label Aug 31, 2017

soltysh added this to the v1.8 milestone Sep 1, 2017

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 1, 2017

calebamiles modified the milestone: v1.8 Sep 2, 2017

CaoShuFeng force-pushed the remove_versioned_test_from_filters branch from 987b28c to d85132b Compare September 4, 2017 11:54

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 4, 2017

CaoShuFeng force-pushed the remove_versioned_test_from_filters branch 2 times, most recently from cb4da9f to 0f35dcd Compare September 4, 2017 12:50

sttts added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Sep 4, 2017

k8s-ci-robot assigned soltysh Sep 6, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 6, 2017

soltysh added this to the v1.8 milestone Sep 6, 2017

CaoShuFeng force-pushed the remove_versioned_test_from_filters branch from 0f35dcd to 23d37fb Compare September 6, 2017 13:08

k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Sep 6, 2017

enhance unit tests of advance audit feature

c030026

This change does three things: 1. use auditinternal for unit test in filter stage 2. add a seperate unit test for Audit-ID http header 3. add unit test for audit log backend

CaoShuFeng force-pushed the remove_versioned_test_from_filters branch from 23d37fb to c030026 Compare September 6, 2017 13:32

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 6, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 6, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 6, 2017

k8s-github-robot merged commit d369160 into kubernetes:master Sep 6, 2017

CaoShuFeng deleted the remove_versioned_test_from_filters branch November 6, 2017 05:46

enhance unit tests of advance audit feature #50842

enhance unit tests of advance audit feature #50842

Conversation

CaoShuFeng commented Aug 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt commented Aug 17, 2017

CaoShuFeng commented Aug 18, 2017

crassirostris left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CaoShuFeng Aug 20, 2017 • edited

Choose a reason for hiding this comment

CaoShuFeng Aug 20, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CaoShuFeng Aug 20, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CaoShuFeng commented Aug 20, 2017

CaoShuFeng commented Aug 20, 2017

CaoShuFeng commented Aug 20, 2017

CaoShuFeng commented Aug 20, 2017

CaoShuFeng commented Aug 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crassirostris commented Aug 21, 2017

CaoShuFeng commented Aug 22, 2017

soltysh commented Sep 1, 2017

CaoShuFeng commented Sep 4, 2017

soltysh commented Sep 4, 2017

CaoShuFeng commented Sep 5, 2017

soltysh commented Sep 6, 2017

soltysh commented Sep 6, 2017

soltysh commented Sep 6, 2017

sttts commented Sep 6, 2017

k8s-github-robot commented Sep 6, 2017

k8s-github-robot commented Sep 6, 2017

crassirostris left a comment •

edited

CaoShuFeng Aug 20, 2017 •

edited

CaoShuFeng Aug 20, 2017 •

edited

CaoShuFeng Aug 20, 2017 •

edited