Skip to content

Conversation

Abirdcfly
Copy link
Contributor

Signed-off-by: Abirdcfly fp544037857@gmail.com

- What I did
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

An example:
https://go.dev/play/p/5K3KZ5RzHhR

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

*/

- How I did it

- How to verify it

- Description for the changelog

fix minor code unreachability error caused by wrong use of t.Fatalf

- A picture of a cute animal (not mandatory but encouraged)

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

Thanks for contributing! I left some suggestions; could you update the PR (amending the commit)? Let me know if you need help doing so!

Comment on lines 226 to 227
t.Fatalf("Got duplicate IP %s", ip.String())
t.Errorf("Got duplicate IP %s", ip.String())
break
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for contributing (sorry for the delay, I saw your PR, but wanted to have a slightly closer look).

So, there's indeed unreachable code in this function (the break would never be reached), but I'm not sure if changing t.Fatal to t.Error is the correct fix here. i'm not very familiar with this test, but looking at the code;

  • allocate() is used by various tests
  • all of them look to be using it to run these steps in parallel

Changing to t.Error() effectively would mean;

  • All parallel runs continue running
  • This for loop also continues running
  • (probably) the code below the loop would still Fatal the test

So the "old" code (before this PR) would fail early; based on all of the above, I think that's intentional, because if one of the (parallel) runs fails, the test should fail, so there's not really a reason to continue running the other (parallel) tests as they're not really testing anything else.

I would suggest to instead keep the t.Error(), and remove the break;

if there, ok := tctx.ipMap[ip.String()]; ok && there {
	t.Fatalf("Got duplicate IP %s", ip.String())
}

While updating, perhaps also change the code outside of the loop, because the assert.Check is redundant, and;

assert.Check(t, is.Len(tctx.ipList, tctx.maxIP))
if len(tctx.ipList) != tctx.maxIP {
	t.Fatal("mismatch number allocation")
}

Can be changed to an assert.Assert() which would check the length and call t.FailNow() if it fails;

assert.Assert(t, is.Len(tctx.ipList, tctx.maxIP))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you very much for your patient review and suggestions, the code has been updated.

Signed-off-by: Abirdcfly <fp544037857@gmail.com>
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

thanks for updating!

LGTM (assuming CI passes 😅)

@Abirdcfly
Copy link
Contributor Author

Kindly ping @thaJeztah for merge, thanks.

@tianon tianon merged commit 8d9d5a3 into moby:master Aug 13, 2022
@thaJeztah thaJeztah added this to the v-next milestone Aug 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants