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.

Sign up for GitHub

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

Fix coverage failures #13693

Merged
merged 1 commit into from
Feb 14, 2022
Merged

Fix coverage failures #13693

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion scripts/build.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ GO_BUILD_ENV=("CGO_ENABLED=0" "GO_BUILD_FLAGS=${GO_BUILD_FLAGS}" "GOOS=${GOOS}"
toggle_failpoints() {
mode="$1"
if command -v gofail >/dev/null 2>&1; then
run gofail "$mode" server/etcdserver/ server/mvcc/backend/
run gofail "$mode" server/etcdserver/ server/storage/backend/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change looks good.

But the directory change was made more than half year ago in this commit, why we do not see any issue in such a long time? cc @serathius @ptabor @spzala

elif [[ "$mode" != "disable" ]]; then
log_error "FAILPOINTS set but gofail not found"
exit 1
Expand Down
2 changes: 1 addition & 1 deletion scripts/test.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,7 +242,7 @@ function split_dir {
local num="${2}"
local i=0
for f in "${d}/"*; do
local g=$(( "${i}" % "${num}" ))
local g=$(( i % num ))
mkdir -p "${d}_${g}"
mv "${f}" "${d}_${g}/"
(( i++ ))
Expand Down
54 changes: 33 additions & 21 deletions tests/e2e/ctl_v3_kv_no_quorum_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,29 +31,41 @@ import (
)

func TestSerializableReadWithoutQuorum(t *testing.T) {
// Initialize a cluster with 3 members
epc, err := e2e.InitEtcdProcessCluster(t, e2e.NewConfigAutoTLS())
if err != nil {
t.Fatalf("Failed to initilize the etcd cluster: %v", err)
tcs := []struct {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change on the e2e test case looks good, but is it related to the the issue of coverage failure?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I won't ask change on this, but it would be better if we could only fix one thing in one PR next time.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test linearizableReadTest indeed causes the coverage test failure. Thanks for the fix.
I will have a deep dive into the coverage test sometime later.

name string
testFunc func(cx ctlCtx)
}{
{
name: "serializableReadTest",
testFunc: serializableReadTest,
},
{
name: "linearizableReadTest",
testFunc: linearizableReadTest,
},
}
for _, tc := range tcs {
t.Run(tc.name, func(t *testing.T) {
// Initialize a cluster with 3 members
epc, err := e2e.InitEtcdProcessCluster(t, e2e.NewConfigAutoTLS())
if err != nil {
t.Fatalf("Failed to initilize the etcd cluster: %v", err)
}

// Remove two members, so that only one etcd will get started
epc.Procs = epc.Procs[:1]
// Remove two members, so that only one etcd will get started
epc.Procs = epc.Procs[:1]

// Start the etcd cluster with only one member
if err := epc.Start(); err != nil {
t.Fatalf("Failed to start the etcd cluster: %v", err)
}

// construct the ctl context
cx := getDefaultCtlCtx(t)
cx.epc = epc
// Start the etcd cluster with only one member
if err := epc.Start(); err != nil {
t.Fatalf("Failed to start the etcd cluster: %v", err)
}

// run serializable test and wait for result
runCtlTest(t, serializableReadTest, nil, cx)

// run linearizable test and wait for result
runCtlTest(t, linearizableReadTest, nil, cx)
// construct the ctl context
cx := getDefaultCtlCtx(t)
cx.epc = epc
runCtlTest(t, tc.testFunc, nil, cx)
})
}
}

func serializableReadTest(cx ctlCtx) {
Expand All @@ -65,7 +77,7 @@ func serializableReadTest(cx ctlCtx) {

func linearizableReadTest(cx ctlCtx) {
cx.quorum = true
if err := ctlV3Get(cx, []string{"key1"}, []kv{}...); err == nil {
cx.t.Error("linearizableReadTest is expected to fail, but it succeeded")
if err := ctlV3GetWithErr(cx, []string{"key"}, []string{"retrying of unary invoker failed"}); err != nil { // expect errors
cx.t.Fatalf("ctlV3GetWithErr error (%v)", err)
}
}