Repository: decred/dcrd
Pull request #3056 (2023-02-15 00:23:47)

chaincfg: Rework init time deployment val logic.
This reworks the deployment validation logic that runs at initialization time to make it easier to modify and bring it more in line with the more modern coding practices used in the codebase.  Similarly, it also reworks the associated deployment validation tests to make them more consistent with modern practices in the code as well as to simplify them and make them more flexible by moving the primary logic for all of the expected results into the test definitions themselves.

The following is a high-level overview of the changes:

- Use `math/bits` from the `stdlib` for population and trailing zero counts
- Rework error handling to use wrapped errors for better error messages while still allowing testability
- Rename several internal errors to better describe what they represent
- Add additional checks for missing abstain and no choices
- Modify `validateDeployments` to accept all deployments for all versions
- Return errors from `validateDeployments`
- Only `panic` directly in the `init` func
- Add missing comments and make the existing ones match reality
- Rework associated deployment validation tests
- Adds test for the missing abstain and missing no choice cases



Patch:
diff --git a/chaincfg/init.go b/chaincfg/init.go
index dd36081e..b7362cd8 100644
--- a/chaincfg/init.go
+++ b/chaincfg/init.go
@@ -1,4 +1,4 @@
-// Copyright (c) 2017-2019 The Decred developers
+// Copyright (c) 2017-2023 The Decred developers
 // Use of this source code is governed by an ISC
 // license that can be found in the LICENSE file.
 
@@ -7,36 +7,26 @@ package chaincfg
 import (
 	"errors"
 	"fmt"
+	"math/bits"
 	"strings"
 )
 
 var (
-	errDuplicateVoteId  = errors.New("duplicate vote id")
-	errInvalidMask      = errors.New("invalid mask")
-	errNotConsecutive   = errors.New("choices not consecutive")
-	errTooManyChoices   = errors.New("too many choices")
-	errInvalidAbstain   = errors.New("invalid abstain bits")
-	errInvalidBits      = errors.New("invalid vote bits")
-	errInvalidIsAbstain = errors.New("one and only one IsAbstain rule " +
-		"violation")
-	errInvalidIsNo      = errors.New("one and only one IsNo rule violation")
-	errInvalidBothFlags = errors.New("IsNo and IsAbstain may not be both " +
-		"set to true")
+	errDuplicateVoteId   = errors.New("duplicate vote id")
+	errInvalidMask       = errors.New("invalid mask")
+	errNotConsecutive    = errors.New("choices not consecutive")
+	errTooManyChoices    = errors.New("too many choices")
+	errInvalidAbstain    = errors.New("invalid abstain bits")
+	errInvalidBits       = errors.New("invalid vote bits")
+	errMissingAbstain    = errors.New("missing abstain choice")
+	errTooManyAbstain    = errors.New("only one choice may have abstain flag")
+	errMissingNo         = errors.New("missing no choice")
+	errTooManyNo         = errors.New("only one choice may have no flag")
+	errBothFlags         = errors.New("abstain and no flags are mutually exclusive")
 	errDuplicateChoiceId = errors.New("duplicate choice ID")
 )
 
-// bitsSet counts number of bits set.
-// Proudly stolen from:
-// https://graphics.stanford.edu/~seander/bithacks.html#CountBitsSetKernighan
-func bitsSet(bits uint16) uint {
-	c := uint(0)
-	for v := bits; v != 0; c++ {
-		v &= v - 1
-	}
-	return c
-}
-
-// consecOnes count consecutive 1 bits set.
+// consecOnes counts the number of consecutive 1 bits set.
 func consecOnes(bits uint16) uint {
 	c := uint(0)
 	for v := bits; v != 0; c++ {
@@ -45,56 +35,43 @@ func consecOnes(bits uint16) uint {
 	return c
 }
 
-// shift calculates the number of bits that need shifting to get to an index.
-func shift(mask uint16) uint {
-	shift := uint(0)
-	for {
-		if mask&0x0001 == 0x0001 {
-			break
-		}
-		shift++
-		mask >>= 1
-	}
-	return shift
-}
-
+// validateChoices ensures the provided choices conform to the required voting
+// choice semantics.
 func validateChoices(mask uint16, choices []Choice) error {
-	var (
-		numAbstain, numNo int
-	)
-
-	// Check that mask is consecutive.
-	if consecOnes(mask) != bitsSet(mask) {
+	// Ensure the mask only consists of consecutive bits.
+	maskPopulationCount := uint(bits.OnesCount16(mask))
+	if consecOnes(mask) != maskPopulationCount {
 		return errInvalidMask
 	}
 
-	// Check bits and choice bounds.
-	if len(choices) > 1<<bitsSet(mask) {
+	// Ensure there are not more choices than the mask bits can represent.
+	if len(choices) > 1<<maskPopulationCount {
 		return errTooManyChoices
 	}
 
+	var numAbstain, numNo int
 	dups := make(map[string]struct{})
-	s := shift(mask)
+	s := uint(bits.TrailingZeros16(mask))
 	for index, choice := range choices {
-		// Check that choice 0 is the abstain vote.
+		// Ensure that choice 0 is the abstain vote.
 		if mask&choice.Bits == 0 && !choice.IsAbstain {
 			return errInvalidAbstain
 		}
 
-		// Check mask bits.
+		// Ensure the bits for the choice are covered by the mask.
 		if mask&choice.Bits != choice.Bits {
 			return errInvalidBits
 		}
 
-		// Check that index is consecutive.  This test is below the
-		// Check mask bits one for testing reasons.  Leave it here.
+		// Ensure the index is consecutive.  This test is below the mask check
+		// for testing reasons.  Leave it here.
 		if uint16(index) != choice.Bits>>s {
 			return errNotConsecutive
 		}
 
-		// Check that both flags aren't set to true.
+		// Ensure only one of the choice type identification flags are set.
 		if choice.IsAbstain && choice.IsNo {
-			return errInvalidBothFlags
+			return errBothFlags
 		}
 
 		// Count flags.
@@ -105,73 +82,69 @@ func validateChoices(mask uint16, choices []Choice) error {
 			numNo++
 		}
 
-		// Check for duplicates.
+		// Ensure there are not any duplicates.
 		id := strings.ToLower(choice.Id)
-		_, found := dups[id]
-		if found {
+		if _, found := dups[id]; found {
 			return errDuplicateChoiceId
 		}
 		dups[id] = struct{}{}
 	}
 
-	// Check that there is only one IsNo and IsAbstain flag set to true.
-	if numAbstain != 1 {
-		return errInvalidIsAbstain
-	}
-	if numNo != 1 {
-		return errInvalidIsNo
+	// Ensure there is one and only one of each choice type identification flag
+	// set.
+	switch {
+	case numAbstain == 0:
+		return errMissingAbstain
+	case numAbstain > 1:
+		return errTooManyAbstain
+	case numNo == 0:
+		return errMissingNo
+	case numNo > 1:
+		return errTooManyNo
 	}
 
 	return nil
 }
 
-func validateAgenda(vote Vote) error {
-	return validateChoices(vote.Mask, vote.Choices)
-}
+// validateDeployments ensures all of deployments in the provided map adhere to
+// the required semantics for deployment definitions.  For example, it ensures
+// there are no duplicate vote IDs and that all choices conform to the required
+// voting choice semantics.
+func validateDeployments(allDeployments map[uint32][]ConsensusDeployment) error {
+	for version, deployments := range allDeployments {
+		// Ensure there are no duplicate vote IDs across the deployment version.
+		dups := make(map[string]struct{})
+		for index, deployment := range deployments {
+			voteID := strings.ToLower(deployment.Vote.Id)
+			if _, found := dups[voteID]; found {
+				return fmt.Errorf("version %d deployment index %d id %q: %w",
+					version, index, deployment.Vote.Id, errDuplicateVoteId)
+			}
+			dups[voteID] = struct{}{}
+		}
+	}
 
-func validateDeployments(deployments []ConsensusDeployment) (int, error) {
-	dups := make(map[string]struct{})
-	for index, deployment := range deployments {
-		// Check for duplicates.
-		id := strings.ToLower(deployment.Vote.Id)
-		_, found := dups[id]
-		if found {
-			return index, errDuplicateVoteId
+	for version, deployments := range allDeployments {
+		for index, deployment := range deployments {
+			// Ensure the vote choices conform to all required semantics.
+			vote := &deployment.Vote
+			err := validateChoices(vote.Mask, vote.Choices)
+			if err != nil {
+				return fmt.Errorf("version %d deployment index %d id %q: %w",
+					version, index, vote.Id, err)
+			}
 		}
-		dups[id] = struct{}{}
 	}
 
-	return -1, nil
+	return nil
 }
 
-func validateAgendas() {
+func init() {
 	allParams := []*Params{MainNetParams(), TestNet3Params(), SimNetParams(),
 		RegNetParams()}
 	for _, params := range allParams {
-		for version, deployments := range params.Deployments {
-			index, err := validateDeployments(deployments)
-			if err != nil {
-				e := fmt.Sprintf("invalid agenda on %v "+
-					"version %v id %v: %v", params.Name,
-					version, deployments[index].Vote.Id,
-					err)
-				panic(e)
-			}
-
-			for _, deployment := range deployments {
-				err := validateAgenda(deployment.Vote)
-				if err != nil {
-					e := fmt.Sprintf("invalid agenda "+
-						"on %v version %v id %v: %v",
-						params.Name, version,
-						deployment.Vote.Id, err)
-					panic(e)
-				}
-			}
+		if err := validateDeployments(params.Deployments); err != nil {
+			panic(fmt.Sprintf("invalid agenda on %s: %v", params.Name, err))
 		}
 	}
 }
-
-func init() {
-	validateAgendas()
-}


Test patch:
diff --git a/chaincfg/init_test.go b/chaincfg/init_test.go
index 89f451b1..880d12e1 100644
--- a/chaincfg/init_test.go
+++ b/chaincfg/init_test.go
@@ -1,4 +1,4 @@
-// Copyright (c) 2017-2020 The Decred developers
+// Copyright (c) 2017-2023 The Decred developers
 // Use of this source code is governed by an ISC
 // license that can be found in the LICENSE file.
 
@@ -11,373 +11,175 @@ import (
 	"github.com/decred/dcrd/wire"
 )
 
-var (
-	consecMask = Vote{
-		Id:          "voteforpedro",
-		Description: "You should always vote for Pedro",
-		Mask:        0xa, // 0b1010 XXX not consecutive mask
-		Choices: []Choice{
-			{
-				Id:          "Abstain",
-				Description: "Abstain voting for Pedro",
-				Bits:        0x0, // 0b0000
-				IsAbstain:   true,
-				IsNo:        false,
-			},
-			{
-				Id:          "Yes",
-				Description: "Vote for Pedro",
-				Bits:        0x2, // 0b0010
-				IsAbstain:   false,
-				IsNo:        false,
-			},
-			{
-				Id:          "No",
-				Description: "Dont vote for Pedro",
-				Bits:        0x4, // 0b0100
-				IsAbstain:   false,
-				IsNo:        true,
-			},
-		},
-	}
-
-	tooManyChoices = Vote{
-		Id:          "voteforpedro",
-		Description: "You should always vote for Pedro",
-		Mask:        0x6, // 0b0110
-		Choices: []Choice{
-			{
-				Id:          "Abstain",
-				Description: "Abstain voting for Pedro",
-				Bits:        0x0, // 0b0000
-				IsAbstain:   true,
-				IsNo:        false,
-			},
-			{
-				Id:          "Yes",
-				Description: "Vote for Pedro",
-				Bits:        0x2, // 0b0010
-				IsAbstain:   false,
-				IsNo:        false,
-			},
-			{
-				Id:          "No",
-				Description: "Dont vote for Pedro",
-				Bits:        0x4, // 0b0100
-				IsAbstain:   false,
-				IsNo:        true,
-			},
-			{
-				Id:          "Maybe",
-				Description: "Vote for Pedro",
-				Bits:        0x6, // 0b0110
-				IsAbstain:   false,
-				IsNo:        false,
-			},
-			// XXX here we go out of mask bounds
-			{
-				Id:          "Hmmmm",
-				Description: "Vote for Pedro",
-				Bits:        0x6, // 0b0110 XXX invalid bits too
-				IsAbstain:   false,
-				IsNo:        false,
-			},
-		},
-	}
-
-	notConsec = Vote{
-		Id:          "voteforpedro",
-		Description: "You should always vote for Pedro",
-		Mask:        0x6, // 0b0110
-		Choices: []Choice{
-			{
-				Id:          "Abstain",
-				Description: "Abstain voting for Pedro",
-				Bits:        0x0, // 0b0000
-				IsAbstain:   true,
-				IsNo:        false,
-			},
-			{
-				Id:          "Yes",
-				Description: "Vote for Pedro",
-				Bits:        0x2, // 0b0010
-				IsAbstain:   false,
-				IsNo:        false,
-			},
-			// XXX not consecutive
-			{
-				Id:          "Maybe",
-				Description: "Vote for Pedro",
-				Bits:        0x6, // 0b0110
-				IsAbstain:   false,
-				IsNo:        false,
-			},
-		},
-	}
-
-	invalidAbstain = Vote{
-		Id:          "voteforpedro",
-		Description: "You should always vote for Pedro",
-		Mask:        0x6, // 0b0110
-		Choices: []Choice{
-			{
-				Id:          "Abstain",
-				Description: "Abstain voting for Pedro",
-				Bits:        0x0,   // 0b0000
-				IsAbstain:   false, // XXX this is the invalid bit
-				IsNo:        false,
-			},
-			{
-				Id:          "Yes",
-				Description: "Vote for Pedro",
-				Bits:        0x2, // 0b0010
-				IsAbstain:   false,
-				IsNo:        false,
-			},
-			{
-				Id:          "No",
-				Description: "Dont vote for Pedro",
-				Bits:        0x4, // 0b0100
-				IsAbstain:   false,
-				IsNo:        true,
-			},
-		},
-	}
-
-	invalidVoteBits = Vote{
-		Id:          "voteforpedro",
-		Description: "You should always vote for Pedro",
-		Mask:        0x6, // 0b0110
-		Choices: []Choice{
-			{
-				Id:          "Abstain",
-				Description: "Abstain voting for Pedro",
-				Bits:        0x0, // 0b0000
-				IsAbstain:   true,
-				IsNo:        false,
-			},
-			{
-				Id:          "Yes",
-				Description: "Vote for Pedro",
-				Bits:        0x2, // 0b0010
-				IsAbstain:   false,
-				IsNo:        false,
-			},
-			{
-				Id:          "No",
-				Description: "Dont vote for Pedro",
-				Bits:        0xc, // 0b1100 XXX invalid bits
-				IsAbstain:   false,
-				IsNo:        true,
-			},
-		},
-	}
-
-	twoIsAbstain = Vote{
-		Id:          "voteforpedro",
-		Description: "You should always vote for Pedro",
-		Mask:        0x6, // 0b0110
-		Choices: []Choice{
-			{
-				Id:          "Abstain",
-				Description: "Abstain voting for Pedro",
-				Bits:        0x0, // 0b0000
-				IsAbstain:   true,
-				IsNo:        false,
-			},
-			{
-				Id:          "Yes",
-				Description: "Vote for Pedro",
-				Bits:        0x2,  // 0b0010
-				IsAbstain:   true, // XXX this is the invalid choice
-				IsNo:        false,
-			},
-			{
-				Id:          "No",
-				Description: "Dont vote for Pedro",
-				Bits:        0x4, // 0b0100
-				IsAbstain:   false,
-				IsNo:        true,
-			},
-		},
-	}
-
-	twoIsNo = Vote{
-		Id:          "voteforpedro",
-		Description: "You should always vote for Pedro",
-		Mask:        0x6, // 0b0110
-		Choices: []Choice{
-			{
-				Id:          "Abstain",
-				Description: "Abstain voting for Pedro",
-				Bits:        0x0, // 0b0000
-				IsAbstain:   true,
-				IsNo:        false,
-			},
-			{
-				Id:          "Yes",
-				Description: "Vote for Pedro",
-				Bits:        0x2, // 0b0010
-				IsAbstain:   false,
-				IsNo:        true, // XXX this is the invalid choice
-			},
-			{
-				Id:          "No",
-				Description: "Dont vote for Pedro",
-				Bits:        0x4, // 0b0100
-				IsAbstain:   false,
-				IsNo:        true,
-			},
-		},
+// mockVote is a convenience func to return a mock vote that is used as a base
+// throughout the tests.
+func mockVote() Vote {
+	return Vote{
+		Id:   "testvote",
+		Mask: 0x6,
+		Choices: []Choice{{
+			Id:          "abstain",
+			Description: "abstain voting for change",
+			Bits:        0x0000,
+			IsAbstain:   true,
+			IsNo:        false,
+		}, {
+			Id:          "no",
+			Description: "keep the existing rules",
+			Bits:        0x0002, // Bit 1
+			IsAbstain:   false,
+			IsNo:        true,
+		}, {
+			Id:          "yes",
+			Description: "change to the new rules",
+			Bits:        0x0004, // Bit 2
+			IsAbstain:   false,
+			IsNo:        false,
+		}},
 	}
+}
 
-	bothFlags = Vote{
-		Id:          "voteforpedro",
-		Description: "You should always vote for Pedro",
-		Mask:        0x6, // 0b0110
-		Choices: []Choice{
-			{
-				Id:          "Abstain",
-				Description: "Abstain voting for Pedro",
-				Bits:        0x0, // 0b0000
-				IsAbstain:   true,
-				IsNo:        false,
-			},
-			{
-				Id:          "Yes",
-				Description: "Vote for Pedro",
-				Bits:        0x2,  // 0b0010
-				IsAbstain:   true, // XXX this is the invalid choice
-				IsNo:        true, // XXX this is the invalid choice
-			},
-			{
-				Id:          "No",
-				Description: "Dont vote for Pedro",
-				Bits:        0x4, // 0b0100
-				IsAbstain:   false,
-				IsNo:        true,
-			},
-		},
-	}
-
-	dupChoice = Vote{
-		Id:          "voteforpedro",
-		Description: "You should always vote for Pedro",
-		Mask:        0x6, // 0b0110
-		Choices: []Choice{
-			{
-				Id:          "Abstain",
-				Description: "Abstain voting for Pedro",
-				Bits:        0x0, // 0b0000
-				IsAbstain:   true,
-				IsNo:        false,
-			},
-			{
-				Id:          "Yes",
-				Description: "Vote for Pedro",
-				Bits:        0x2, // 0b0010
-				IsAbstain:   false,
-				IsNo:        false,
-			},
-			{
-				Id:          "Yes", // XXX this is the invalid ID
-				Description: "Dont vote for Pedro",
-				Bits:        0x4, // 0b0100
-				IsAbstain:   false,
-				IsNo:        true,
-			},
-		},
-	}
-)
-
-func TestChoices(t *testing.T) {
+// TestValidateChoices ensures the validation logic for enforcing choice
+// semantics works as intended.
+func TestValidateChoices(t *testing.T) {
 	tests := []struct {
 		name     string
-		vote     Vote
+		munger   func(*Vote)
 		expected error
-	}{
-		{
-			name:     "consecutive mask",
-			vote:     consecMask,
-			expected: errInvalidMask,
-		},
-		{
-			name:     "not consecutive choices",
-			vote:     notConsec,
-			expected: errNotConsecutive,
-		},
-		{
-			name:     "too many choices",
-			vote:     tooManyChoices,
-			expected: errTooManyChoices,
-		},
-		{
-			name:     "invalid ignore",
-			vote:     invalidAbstain,
-			expected: errInvalidAbstain,
-		},
-		{
-			name:     "invalid vote bits",
-			vote:     invalidVoteBits,
-			expected: errInvalidBits,
-		},
-		{
-			name:     "2 IsAbstain",
-			vote:     twoIsAbstain,
-			expected: errInvalidIsAbstain,
-		},
-		{
-			name:     "2 IsNo",
-			vote:     twoIsNo,
-			expected: errInvalidIsNo,
-		},
-		{
-			name:     "both IsAbstain IsNo",
-			vote:     bothFlags,
-			expected: errInvalidBothFlags,
-		},
-		{
-			name:     "duplicate choice id",
-			vote:     dupChoice,
-			expected: errDuplicateChoiceId,
-		},
-	}
+	}{{
+		name: "require consecutive mask",
+		munger: func(vote *Vote) {
+			vote.Mask = 0xa // 0b1010 -- non-consecutive mask
+		},
+		expected: errInvalidMask,
+	}, {
+		name: "require consecutive choices",
+		munger: func(vote *Vote) {
+			// The other choices in the mock vote are for abstain and yes, so
+			// the next one should be for bit 2 (0x0004).
+			vote.Choices[2].Bits = 0x6
+		},
+		expected: errNotConsecutive,
+	}, {
+		name: "reject too many choices",
+		munger: func(vote *Vote) {
+			vote.Choices = append(vote.Choices, Choice{
+				Id:   "maybe",
+				Bits: 0x0006,
+			})
+			vote.Choices = append(vote.Choices, Choice{
+				Id:   "another",
+				Bits: 0x0006, // invalid bits too
+			})
+		},
+		expected: errTooManyChoices,
+	}, {
+		name: "reject choice with abstain bits but no abstain flag",
+		munger: func(vote *Vote) {
+			// Mark the abstain choice as not being abstain.
+			vote.Choices[0].IsAbstain = false
+		},
+		expected: errInvalidAbstain,
+	}, {
+		name: "reject choice with bits that do not conform to the mask",
+		munger: func(vote *Vote) {
+			// Assign invalid bits per the mask to a choice.
+			vote.Choices[2].Bits = 0xc // 0b1100
+		},
+		expected: errInvalidBits,
+	}, {
+		name: "require an abstain choice",
+		munger: func(vote *Vote) {
+			// Remove all choices.
+			vote.Choices = nil
+		},
+		expected: errMissingAbstain,
+	}, {
+		name: "reject more than one abstain choice",
+		munger: func(vote *Vote) {
+			// Mark a second choice as abstain.
+			vote.Choices[1].IsAbstain = true
+			vote.Choices[1].IsNo = false
+		},
+		expected: errTooManyAbstain,
+	}, {
+		name: "require a no choice",
+		munger: func(vote *Vote) {
+			// Replace the no choice with the yes choice.
+			vote.Choices = vote.Choices[:2]
+			vote.Choices[1].Id = "yes"
+			vote.Choices[1].IsNo = false
+		},
+		expected: errMissingNo,
+	}, {
+		name: "reject more than one no choice",
+		munger: func(vote *Vote) {
+			// Mark a second choice as no.
+			vote.Choices[2].IsNo = true
+		},
+		expected: errTooManyNo,
+	}, {
+		name: "require exclusive abstain and no flags",
+		munger: func(vote *Vote) {
+			// Mark a choice with both flags.
+			vote.Choices[1].IsAbstain = true
+			vote.Choices[1].IsNo = true
+		},
+		expected: errBothFlags,
+	}, {
+		name: "require unique choice ids",
+		munger: func(vote *Vote) {
+			// Assign duplicate id.
+			vote.Choices[2].Id = vote.Choices[1].Id
+		},
+		expected: errDuplicateChoiceId,
+	}}
 
 	for _, test := range tests {
-		t.Logf("running: %v", test.name)
-		err := validateAgenda(test.vote)
+		// Create the vote and mutate it per the test.
+		vote := mockVote()
+		if test.munger != nil {
+			test.munger(&vote)
+		}
+
+		err := validateChoices(vote.Mask, vote.Choices)
 		if !errors.Is(err, test.expected) {
-			t.Fatalf("%v: got '%v' expected '%v'", test.name, err,
+			t.Fatalf("%q: unexpected err -- got %v, want %v", test.name, err,
 				test.expected)
 		}
 	}
 }
 
-var (
-	dupVote = []ConsensusDeployment{
-		{Vote: Vote{Id: "moo"}},
-		{Vote: Vote{Id: "moo"}},
-	}
-)
-
-func TestDeployments(t *testing.T) {
+// TestRejectDuplicateVoteIDs ensures the validation logic for enforcing unique
+// deployment/vote IDs across the deployments for a given deployment version
+// works as intended.
+func TestRejectDuplicateVoteIDs(t *testing.T) {
 	tests := []struct {
 		name        string
-		deployments []ConsensusDeployment
+		deployments map[uint32][]ConsensusDeployment
 		expected    error
-	}{
-		{
-			name:        "duplicate vote id",
-			deployments: dupVote,
-			expected:    errDuplicateVoteId,
+	}{{
+		name: "reject duplicate vote id in the same version",
+		deployments: map[uint32][]ConsensusDeployment{
+			1: {
+				{Vote: mockVote()},
+				{Vote: mockVote()},
+			},
 		},
-	}
+		expected: errDuplicateVoteId,
+	}, {
+		name: "allow duplicate vote id in different versions",
+		deployments: map[uint32][]ConsensusDeployment{
+			1: {{Vote: mockVote()}},
+			2: {{Vote: mockVote()}},
+		},
+		expected: nil,
+	}}
 
 	for _, test := range tests {
-		t.Logf("running: %v", test.name)
-		_, err := validateDeployments(test.deployments)
+		err := validateDeployments(test.deployments)
 		if !errors.Is(err, test.expected) {
-			t.Fatalf("%v: got '%v' expected '%v'", test.name, err,
+			t.Fatalf("%q: unexpected err -- got %v, want %v", test.name, err,
 				test.expected)
 		}
 	}