Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tests: Refactor spawn json command #14029

Merged
merged 1 commit into from
May 13, 2022
Merged

Conversation

serathius
Copy link
Member

@chaochn47
Copy link
Member

chaochn47 commented May 11, 2022

LGTM.

The e2e test failures in the CI are limited to lease.

         lease_test.go:312: 
            	Error Trace:	lease_test.go:312
            	            				execute.go:29
            	            				asm_386.s:1319
            	Error:      	Received unexpected error:
            	            	match not found. Set EXPECT_DEBUG for more info Err: read /dev/ptmx: input/output error, last lines:
            	            	{"level":"warn","ts":"2022-05-10T22:55:17.039Z","caller":"flags/flag.go:93","msg":"unrecognized environment variable","environment-variable":"ETCDCTL_API=3"}
            	            	{"cluster_id":238453183653593855,"member_id":14578408409545168728,"revision":1,"raft_term":2,"ID":6293921973442619141,"TTL":20,"Error":""}
            	Test:       	TestLeaseGrantRevoke/ClientTLS

The missing "header" line where spawn json cmd expects is due to

etcd/client/v3/lease.go

Lines 71 to 75 in b7be91f

// LeaseLeasesResponse wraps the protobuf message LeaseLeasesResponse.
type LeaseLeasesResponse struct {
*pb.ResponseHeader
Leases []LeaseStatus `json:"leases"`
}

the same as I called out in #14020 (comment)

So changing

line, err := cmd.Expect("header")
to

line, err := cmd.Expect("cluster_id")
# any other fields in the header should work 
# line, err := cmd.Expect("raft_term")

Should fix the test failures. Or we leave the test lease interface implementation alone.

tests/framework/e2e/etcdctl.go Outdated Show resolved Hide resolved
tests/framework/e2e/etcdctl.go Show resolved Hide resolved
tests/framework/e2e/etcdctl.go Show resolved Hide resolved
tests/framework/e2e/etcdctl.go Show resolved Hide resolved
tests/framework/e2e/etcdctl.go Show resolved Hide resolved
tests/framework/e2e/etcdctl.go Outdated Show resolved Hide resolved
tests/framework/e2e/etcdctl.go Outdated Show resolved Hide resolved
tests/framework/e2e/etcdctl.go Outdated Show resolved Hide resolved
tests/framework/e2e/etcdctl.go Show resolved Hide resolved
tests/framework/e2e/etcdctl.go Show resolved Hide resolved
@ahrtr
Copy link
Member

ahrtr commented May 11, 2022

I reran the failed pipelines 3 times, but always failing. Please make sure the failures are not caused by this PR or some recent PRs.

If the lease issue (the fields are not under the "header") is the reason for the failure, then why some other PRs (i.e. 14022 ) are all green?

@chaochn47
Copy link
Member

If the lease issue (the fields are not under the "header") is the reason for the failure, then why some other PRs (i.e. #14022 ) are all green?

Because other PRs checks id existence in the header.

line, err := cmd.Expect("id")

@ahrtr
Copy link
Member

ahrtr commented May 11, 2022

If the lease issue (the fields are not under the "header") is the reason for the failure, then why some other PRs (i.e. #14022 ) are all green?

Because other PRs checks id existence in the header.

line, err := cmd.Expect("id")

Thanks for the clarification. Then the failure is indeed caused by this PR. We can leave the LeaseList as it's (in the main branch) for now, and add a comment there. Once we refactor the lease in future, we can revisit the the (ctl *EtcdctlV3) LeaseList() again.

@codecov-commenter
Copy link

codecov-commenter commented May 11, 2022

Codecov Report

Merging #14029 (83a4309) into main (066e510) will decrease coverage by 0.19%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main   #14029      +/-   ##
==========================================
- Coverage   74.99%   74.80%   -0.20%     
==========================================
  Files         450      450              
  Lines       37173    37170       -3     
==========================================
- Hits        27879    27806      -73     
- Misses       7520     7582      +62     
- Partials     1774     1782       +8     
Flag Coverage Δ
all 74.80% <ø> (-0.20%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
server/proxy/grpcproxy/register.go 69.76% <0.00%> (-9.31%) ⬇️
server/etcdserver/api/v3lock/lock.go 61.53% <0.00%> (-8.47%) ⬇️
client/pkg/v3/fileutil/purge.go 66.03% <0.00%> (-7.55%) ⬇️
client/v3/namespace/watch.go 87.87% <0.00%> (-6.07%) ⬇️
server/storage/mvcc/watchable_store.go 85.14% <0.00%> (-5.80%) ⬇️
server/auth/simple_token.go 83.07% <0.00%> (-5.39%) ⬇️
server/proxy/grpcproxy/watch.go 92.48% <0.00%> (-4.05%) ⬇️
server/etcdserver/api/v3rpc/watch.go 84.89% <0.00%> (-3.70%) ⬇️
server/etcdserver/api/v3rpc/member.go 93.54% <0.00%> (-3.23%) ⬇️
client/v3/concurrency/election.go 79.68% <0.00%> (-2.35%) ⬇️
... and 19 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 066e510...83a4309. Read the comment docs.

@ahrtr ahrtr merged commit 29f090b into etcd-io:main May 13, 2022
@serathius serathius deleted the spawn-json-command branch June 15, 2023 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

5 participants