e2e: improve Lease coverage #9017

hexfusion · 2017-12-15T03:26:28Z

Added tests to improve Lease coverage in e2e testing.

authLeaseTestKeepAlive
authLeaseTestTimeToLiveExpired
leaseTestTimeToLiveExpired

gyuho · 2017-12-15T05:52:02Z

Also test lease timetolive command output on the expired lease (e.g. ...granted with TTL(0s), remaining(-...)?

hexfusion · 2017-12-15T11:44:58Z

e2e/ctl_v3_auth_test.go

+	}
+	if !strings.Contains(line, "TTL(3s), remaining(0s)") {
+		cx.t.Fatalf("expected 'TTL(3s), remaining(0s)', got %q", line)
+	}


@gyuho why does this test pass? Are one of the CI checks running e2e against master?

Seems like it just got lucky, it ran timetolive command when TTL is <1-sec.

gyuho · 2017-12-15T17:22:36Z

e2e/ctl_v3_auth_test.go

+	if err := ctlV3Put(cx, "key", "val", leaseID); err != nil {
+		cx.t.Fatalf("authLeaseTestTimeToLiveExpired: ctlV3Put error (%v)", err)
+	}
+	// eliminate false posative


gyuho · 2017-12-15T17:26:40Z

e2e/ctl_v3_auth_test.go

+	if err != nil {
+		cx.t.Fatal(err)
+	}
+	line, err := proc.Expect(" granted with TTL(")


This should be matched by TTL(0s), remaining(-1s) since we are testing the expired lease output.

gyuho · 2017-12-15T17:43:37Z

e2e/ctl_v3_lease_test.go

@@ -73,6 +75,33 @@ func leaseTestGrantLeasesList(cx ctlCtx) {
 	}
 }

+func leaseTestTimeToLiveExpired(cx ctlCtx) {


Seems redundant with authLeaseTestTimeToLiveExpired. Could this be simplified by sharing code with authLeaseTestTimeToLiveExpired?

hexfusion · 2017-12-15T18:23:41Z

@gyuho thanks for review, will resolve.

hexfusion · 2017-12-15T22:36:35Z

@gyuho PTAL, thanks.

gyuho · 2017-12-15T23:00:49Z

e2e/ctl_v3_lease_test.go

+		return fmt.Errorf(fmt.Sprintf("expected '%s', got %q", e, line))
+	}
+	if err := ctlV3Get(cx, []string{"key"}, kv{"key", ""}); err != nil {
+		fmt.Errorf("leaseTestTimeToLiveExpired: ctlV3Get error (%v)", err)


Forgot to return this error?

hexfusion · 2017-12-15T23:32:38Z

@gyuho fixed PTAL

hexfusion · 2017-12-16T19:05:46Z

Excuse the dust folks shaking off the rust a bit. \o/

xiang90 · 2017-12-16T19:46:59Z

e2e/ctl_v3_lease_test.go

+	if err != nil {
+		return err
+	}
+	line, err := proc.Expect(" granted with TTL(")


@hexfusion @gyuho

eventually we should return things like lease already expired to explicitly return the error.

xiang90 · 2017-12-16T19:47:13Z

@hexfusion Thanks!

LGTM. Defer to @gyuho

codecov-io · 2017-12-16T20:44:16Z

Codecov Report

Merging #9017 into master will increase coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #9017      +/-   ##
==========================================
+ Coverage   76.12%   76.13%   +<.01%     
==========================================
  Files         359      359              
  Lines       29838    29838              
==========================================
+ Hits        22715    22717       +2     
- Misses       5548     5551       +3     
+ Partials     1575     1570       -5

Impacted Files	Coverage Δ
mvcc/watchable_store.go	`85.97% <0%> (-4.43%)`	⬇️
clientv3/leasing/txn.go	`88.09% <0%> (-3.18%)`	⬇️
proxy/grpcproxy/watch.go	`91.92% <0%> (-3.11%)`	⬇️
rafthttp/peer.go	`90.22% <0%> (-1.51%)`	⬇️
clientv3/op.go	`72.56% <0%> (-1.33%)`	⬇️
pkg/netutil/netutil.go	`64.36% <0%> (-1.15%)`	⬇️
clientv3/leasing/kv.go	`90.93% <0%> (-0.68%)`	⬇️
clientv3/leasing/cache.go	`91.11% <0%> (-0.56%)`	⬇️
etcdserver/api/v3rpc/watch.go	`92.69% <0%> (-0.46%)`	⬇️
etcdserver/server.go	`79.12% <0%> (+0.22%)`	⬆️
... and 12 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 33a1d30...ed36728. Read the comment docs.

gyuho

minor nits, and lgtm. Thanks!

gyuho · 2017-12-18T02:52:37Z

e2e/ctl_v3_lease_test.go

+	if err != nil {
+		return err
+	}
+	line, err := proc.Expect(" granted with TTL(")


_, err := proc.Expect("TTL(0s), remaining(-1s)") // expect expired lease

and remove line 105-108?

Yeah good point, thanks.

gyuho · 2017-12-18T15:21:27Z

Could we fix https://travis-ci.org/coreos/etcd/jobs/318061698#L859?

hexfusion mentioned this pull request Dec 15, 2017

On master, key doesn't be deleted when bound lease expires #9006

Closed

hexfusion changed the title ~~e2e: improve lease coverage~~ e2e: improve Lease coverage Dec 15, 2017

hexfusion commented Dec 15, 2017

View reviewed changes

gyuho reviewed Dec 15, 2017

View reviewed changes

hexfusion force-pushed the test_lease_auth branch from ee329c0 to 8d0e777 Compare December 15, 2017 23:31

hexfusion force-pushed the test_lease_auth branch 3 times, most recently from 2bc80d6 to db908e2 Compare December 16, 2017 19:03

xiang90 reviewed Dec 16, 2017

View reviewed changes

gyuho suggested changes Dec 18, 2017

View reviewed changes

hexfusion force-pushed the test_lease_auth branch from db908e2 to 4b5b281 Compare December 18, 2017 12:49

e2e: improve lease coverage

ed36728

hexfusion force-pushed the test_lease_auth branch from 4b5b281 to ed36728 Compare December 18, 2017 15:48

gyuho approved these changes Dec 18, 2017

View reviewed changes

gyuho merged commit 9b98cbb into etcd-io:master Dec 18, 2017

hexfusion mentioned this pull request Dec 18, 2017

e2e: add leaseTestTimeToLiveExpired #9010

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

e2e: improve Lease coverage #9017

e2e: improve Lease coverage #9017

hexfusion commented Dec 15, 2017 •

edited

Loading

gyuho commented Dec 15, 2017

hexfusion Dec 15, 2017 •

edited

Loading

gyuho Dec 15, 2017

gyuho Dec 15, 2017

gyuho Dec 15, 2017

gyuho Dec 15, 2017

hexfusion commented Dec 15, 2017 •

edited

Loading

hexfusion commented Dec 15, 2017

gyuho Dec 15, 2017

hexfusion commented Dec 15, 2017

hexfusion commented Dec 16, 2017

xiang90 Dec 16, 2017

xiang90 commented Dec 16, 2017

codecov-io commented Dec 16, 2017 •

edited

Loading

gyuho left a comment

gyuho Dec 18, 2017

hexfusion Dec 18, 2017

gyuho commented Dec 18, 2017

e2e: improve Lease coverage #9017

e2e: improve Lease coverage #9017

Conversation

hexfusion commented Dec 15, 2017 • edited Loading

gyuho commented Dec 15, 2017

hexfusion Dec 15, 2017 • edited Loading

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

hexfusion commented Dec 15, 2017 • edited Loading

hexfusion commented Dec 15, 2017

Choose a reason for hiding this comment

hexfusion commented Dec 15, 2017

hexfusion commented Dec 16, 2017

Choose a reason for hiding this comment

xiang90 commented Dec 16, 2017

codecov-io commented Dec 16, 2017 • edited Loading

Codecov Report

gyuho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gyuho commented Dec 18, 2017

hexfusion commented Dec 15, 2017 •

edited

Loading

hexfusion Dec 15, 2017 •

edited

Loading

hexfusion commented Dec 15, 2017 •

edited

Loading

codecov-io commented Dec 16, 2017 •

edited

Loading