Add e2e test for watch #60331

jennybuckley · 2018-02-23T23:05:04Z

What this PR does / why we need it:
Currently watch is only tested by kubectl tests, but all clients of kubernetes should be able to reliably watch resources for changes. This test should be able to accommodate testing watch for custom resources without many changes.

/sig api-machinery

Added e2e test for watch

jennybuckley · 2018-02-26T04:37:06Z

/test pull-kubernetes-e2e-kops-aws

jennybuckley · 2018-02-26T18:35:52Z

/cc @yliaog
/cc @wenjiaswe

k8s-ci-robot · 2018-02-26T18:35:54Z

@jennybuckley: GitHub didn't allow me to request PR reviews from the following users: wenjiaswe.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @yliaog
/cc @wenjiaswe

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.

yliaog · 2018-02-26T19:24:25Z

test/e2e/apimachinery/watch.go

@@ -43,7 +41,7 @@ const (
 var _ = SIGDescribe("Watchers", func() {
 	f := framework.NewDefaultFramework("watch")

-	It("should concurrently observe add, update, and delete events on pods", func() {
+	It("should observe add, update, and delete events on pods", func() {


what's the reason to make it serial?

I think it used too much code just maintaining the concurrency and the test should be valid either way. Also, it's not serial in the sense that it can't be run at the same time as other tests, so the length of time it takes to run shouldn't matter much.

yliaog · 2018-02-26T19:30:24Z

test/e2e/apimachinery/watch.go

+
+func setPodActiveDeadlineSeconds5(p *v1.Pod) {
+	p.Spec.ActiveDeadlineSeconds = int64ptr(5)
+}


maybe better to merge the above two func into one, and add a new param for deadline seconds?

yliaog · 2018-02-26T19:32:46Z

test/e2e/apimachinery/watch.go

+}
+
+func chanThatClosesIn(seconds time.Duration) chan struct{} {
+	ch := make(chan struct{})


what about using Timer directly?

yliaog · 2018-02-26T19:33:32Z

test/e2e/apimachinery/watch.go

+}
+
+func expectNoEvent(w watch.Interface, eventType watch.EventType, object runtime.Object) {
+	stop := chanThatClosesIn(10)


seems most code logic are the same for the two func, what about merge them into one?

jennybuckley · 2018-02-26T22:17:22Z

#60442 merged
/retest

yliaog · 2018-02-26T23:24:45Z

/lgtm

janetkuo · 2018-02-27T00:21:34Z

test/e2e/apimachinery/watch.go

+		Expect(err).NotTo(HaveOccurred())
+
+		// Wait 10s for the watches to take effect
+		time.Sleep(10 * time.Second)


Are there better way to do this (e.g. polling and check watcher's status)?

After some looking, I think the .Watch() call won't return until the watch has taken effect. The test still passes without this line.

jennybuckley · 2018-02-28T20:57:54Z

/test all
All tests pass, just trying to see how consistent it is

jennybuckley · 2018-02-28T21:49:42Z

/test test pull-kubernetes-e2e-gce

jennybuckley · 2018-03-02T19:29:17Z

/test pull-kubernetes-integration
test is passed but it doesn't show up on the checklist

jennybuckley · 2018-03-02T21:09:02Z

/retest

ayushpateria · 2018-03-12T04:22:35Z

test/e2e/apimachinery/watch.go

+	for {
+		select {
+		case actual := <-w.ResultChan():
+			if expect.Type == actual.Type && (expect.Object == nil || apiequality.Semantic.DeepEqual(expect.Object, actual.Object)) {


When running this e2e test locally it fails with the error: "Timed out waiting for expected event: {DELETED <nil>}". The comparison expect.Object == nil doesn't evaluate to true in the case of delete as we would expect, and this leads to a timeout. This is because expect.Object is an interface whose underlying value is nil but the interface itself is non-nil (https://golang.org/doc/faq#nil_error).

A safer way to do this would be reflect.ValueOf(expect.Object).IsNil()

Thanks for catching this, now I'm just confused how it was passing on my computer and on the pre-submit test...

I can't seem to replicate that behavior, but I still updated the test to be more explicit,
https://play.golang.org/p/_FYAn6o_kTQ
It seems like reflect.ValueOf(expect.Object).IsNil() causes a panic when used on a nil value

Alright! I think it was occurring in mine due to some other reasons. This looks fine, thanks for the example :)

jpbetz · 2018-03-12T21:38:02Z

Thanks for bolstering the test coverage here @jennybuckley ! This is much needed.

/lgtm

janetkuo · 2018-03-12T21:58:07Z

/approve

k8s-ci-robot · 2018-03-12T21:58:15Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: janetkuo, jennybuckley, jpbetz, yliaog

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:

~~test/OWNERS~~ [janetkuo]

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

fejta-bot · 2018-03-17T12:20:50Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

k8s-github-robot · 2018-03-20T06:34:22Z

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

k8s-github-robot · 2018-03-20T06:42:09Z

Automatic merge from submit-queue (batch tested with PRs 60457, 60331, 54970, 58731, 60562). If you want to cherry-pick this change to another branch, please follow the instructions here.

Automatic merge from submit-queue (batch tested with PRs 61404, 61025). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Add e2e test for CRD Watch **What this PR does / why we need it**: This adds an e2e test to watch for custom resources. **Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*: Fixes #55538 **Special notes for your reviewer**: This PR depends on some functions from #60331, and shouldn't be merged before that one gets merged. **Release note**: ```release-note Add e2e test for CRD Watch ```

k8s-ci-robot requested review from krousey and vishh February 23, 2018 23:05

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Feb 23, 2018

jennybuckley force-pushed the watch-e2e-test branch 4 times, most recently from 18f60e2 to 3370313 Compare February 24, 2018 00:25

k8s-ci-robot requested a review from yliaog February 26, 2018 18:35

yliaog reviewed Feb 26, 2018

View reviewed changes

jennybuckley force-pushed the watch-e2e-test branch 3 times, most recently from 8d139fc to 5a4516e Compare February 26, 2018 20:47

k8s-ci-robot assigned yliaog Feb 26, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 26, 2018

janetkuo reviewed Feb 27, 2018

View reviewed changes

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 27, 2018

ayushpateria mentioned this pull request Mar 12, 2018

Add e2e test for CRD Watch #61025

Merged

ayushpateria reviewed Mar 12, 2018

View reviewed changes

Add e2e test for watch

3b2472a

jennybuckley force-pushed the watch-e2e-test branch from 351be30 to 3b2472a Compare March 12, 2018 17:48

k8s-ci-robot assigned jpbetz Mar 12, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 12, 2018

janetkuo approved these changes Mar 12, 2018

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 12, 2018

k8s-github-robot merged commit c0db49c into kubernetes:master Mar 20, 2018

bgrant0607 mentioned this pull request Apr 12, 2018

Promote watch e2e test to conformance #61424

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add e2e test for watch #60331

Add e2e test for watch #60331

jennybuckley commented Feb 23, 2018 •

edited

Loading

jennybuckley commented Feb 26, 2018

jennybuckley commented Feb 26, 2018

k8s-ci-robot commented Feb 26, 2018

yliaog Feb 26, 2018

jennybuckley Feb 26, 2018

yliaog Feb 26, 2018

yliaog Feb 26, 2018

yliaog Feb 26, 2018

jennybuckley commented Feb 26, 2018

yliaog commented Feb 26, 2018

janetkuo Feb 27, 2018

jennybuckley Feb 27, 2018 •

edited

Loading

jennybuckley commented Feb 28, 2018

jennybuckley commented Feb 28, 2018

jennybuckley commented Mar 2, 2018

jennybuckley commented Mar 2, 2018

ayushpateria Mar 12, 2018

ayushpateria Mar 12, 2018

jennybuckley Mar 12, 2018

jennybuckley Mar 12, 2018

ayushpateria Mar 12, 2018

jpbetz commented Mar 12, 2018

janetkuo commented Mar 12, 2018

k8s-ci-robot commented Mar 12, 2018

fejta-bot commented Mar 17, 2018

k8s-github-robot commented Mar 20, 2018

k8s-github-robot commented Mar 20, 2018

Add e2e test for watch #60331

Add e2e test for watch #60331

Conversation

jennybuckley commented Feb 23, 2018 • edited Loading

jennybuckley commented Feb 26, 2018

jennybuckley commented Feb 26, 2018

k8s-ci-robot commented Feb 26, 2018

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

jennybuckley commented Feb 26, 2018

yliaog commented Feb 26, 2018

Choose a reason for hiding this comment

jennybuckley Feb 27, 2018 • edited Loading

Choose a reason for hiding this comment

jennybuckley commented Feb 28, 2018

jennybuckley commented Feb 28, 2018

jennybuckley commented Mar 2, 2018

jennybuckley commented Mar 2, 2018

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

jpbetz commented Mar 12, 2018

janetkuo commented Mar 12, 2018

k8s-ci-robot commented Mar 12, 2018

fejta-bot commented Mar 17, 2018

k8s-github-robot commented Mar 20, 2018

k8s-github-robot commented Mar 20, 2018

jennybuckley commented Feb 23, 2018 •

edited

Loading

jennybuckley Feb 27, 2018 •

edited

Loading