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

Bug Report: some minor code unreachability error mainly in testing #10772

Closed
Abirdcfly opened this issue Jul 21, 2022 · 0 comments · Fixed by #10771
Closed

Bug Report: some minor code unreachability error mainly in testing #10772

Abirdcfly opened this issue Jul 21, 2022 · 0 comments · Fixed by #10771
Labels
Component: General Changes throughout the code base Type: Bug Type: Testing

Comments

@Abirdcfly
Copy link
Contributor

Abirdcfly commented Jul 21, 2022

Overview of the Issue

t.Error* will report test failures but continue executing the test.
t.Fatal* will report test failures and stop the test immediately.

So some of the code is actually unreachable

let me give a test example:

package main

import (
	"testing"
)

// LastIndex returns the index of the last instance of x in list, or
// -1 if x is not present.
func LastIndex(list []int, x int) int {
	for i := len(list) - 1; i >= 0; i-- {
		if list[i] == x {
			return i
		}
	}
	return -1
}

func TestLastIndex(t *testing.T) {
	tests := []struct {
		list []int
		x    int
		want int
	}{
		{list: []int{1}, x: 1, want: 0},
		{list: []int{1, 1}, x: 1, want: 3}, // change want:1 to want:3, should fail
		{list: []int{2, 1}, x: 2, want: 0},
		{list: []int{1, 2, 1, 1}, x: 2, want: 1},
		{list: []int{1, 1, 1, 2, 2, 1}, x: 3, want: -1},
		{list: []int{3, 1, 2, 2, 1, 1}, x: 3, want: 111}, // change want:0 to want:111, should fail
	}
	for i, tt := range tests {
		t.Logf("index:%d", i)
		if got := LastIndex(tt.list, tt.x); got != tt.want {
			t.Errorf("LastIndex(%v, %v) = %v, want %v", tt.list, tt.x, got, tt.want)
		}
	}
}

func TestLastIndex2(t *testing.T) {
	tests := []struct {
		list []int
		x    int
		want int
	}{
		{list: []int{1}, x: 1, want: 0},
		{list: []int{1, 1}, x: 1, want: 3}, // change want:1 to want:3, should fail
		{list: []int{2, 1}, x: 2, want: 0},
		{list: []int{1, 2, 1, 1}, x: 2, want: 1},
		{list: []int{1, 1, 1, 2, 2, 1}, x: 3, want: -1},
		{list: []int{3, 1, 2, 2, 1, 1}, x: 3, want: 111}, // change want:0 to want:111, should fail
	}
	for i, tt := range tests {
		t.Logf("index:%d", i)
		if got := LastIndex(tt.list, tt.x); got != tt.want {
			t.Fatalf("LastIndex(%v, %v) = %v, want %v", tt.list, tt.x, got, tt.want) // Fatalf is equivalent to Logf followed by FailNow, so the following code will be unreachable
			continue  // unreachable
		}
	}
}

/* output:

=== RUN   TestLastIndex
    prog.go:32: index:0
    prog.go:32: index:1
    prog.go:34: LastIndex([1 1], 1) = 1, want 3
    prog.go:32: index:2
    prog.go:32: index:3
    prog.go:32: index:4
    prog.go:32: index:5
    prog.go:34: LastIndex([3 1 2 2 1 1], 3) = 0, want 111
--- FAIL: TestLastIndex (0.00s)
=== RUN   TestLastIndex2
    prog.go:53: index:0
    prog.go:53: index:1
    prog.go:55: LastIndex([1 1], 1) = 1, want 3
--- FAIL: TestLastIndex2 (0.00s)
FAIL

*/

As for log vitess use

vitess/go/vt/log/log.go

Lines 70 to 71 in bf13249

// Fatalf formats arguments like fmt.Printf
Fatalf = glog.Fatalf

What is actually called is

https://github.com/golang/glog/blob/9ef845f417d839250ceabbc25c1b26101e772dd7/glog.go#L1144-L1149

So same with testing.

Reproduction Steps

No

Binary Version

No

Operating System and Environment details

No

Log Fragments

No response

@Abirdcfly Abirdcfly added Needs Triage This issue needs to be correctly labelled and triaged Type: Bug labels Jul 21, 2022
@frouioui frouioui added Type: Testing Component: General Changes throughout the code base and removed Needs Triage This issue needs to be correctly labelled and triaged labels Jul 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: General Changes throughout the code base Type: Bug Type: Testing
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants