ci/tests for #1784

protocol/v2/qbft/spectest/controller_type.go

@@ -10,22 +10,20 @@ import (
 	"reflect"
 	"testing"
 
-	"github.com/ssvlabs/ssv/exporter/convert"
-
 	specqbft "github.com/ssvlabs/ssv-spec/qbft"
 	spectests "github.com/ssvlabs/ssv-spec/qbft/spectest/tests"
 	spectypes "github.com/ssvlabs/ssv-spec/types"
 	spectestingutils "github.com/ssvlabs/ssv-spec/types/testingutils"
 	typescomparable "github.com/ssvlabs/ssv-spec/types/testingutils/comparable"
-	"github.com/stretchr/testify/require"
-	"go.uber.org/zap"
-
+	"github.com/ssvlabs/ssv/exporter/convert"
 	"github.com/ssvlabs/ssv/logging"
 	"github.com/ssvlabs/ssv/protocol/v2/qbft"
 	"github.com/ssvlabs/ssv/protocol/v2/qbft/controller"
 	"github.com/ssvlabs/ssv/protocol/v2/qbft/roundtimer"
 	qbfttesting "github.com/ssvlabs/ssv/protocol/v2/qbft/testing"
 	protocoltesting "github.com/ssvlabs/ssv/protocol/v2/testing"
+	"github.com/stretchr/testify/require"
+	"go.uber.org/zap"
 )
 
 func RunControllerSpecTest(t *testing.T, test *spectests.ControllerSpecTest) {
@@ -50,12 +48,7 @@ func RunControllerSpecTest(t *testing.T, test *spectests.ControllerSpecTest) {
 		}
 		height++
 	}
-
-	if len(test.ExpectedError) != 0 {
-		require.EqualError(t, lastErr, test.ExpectedError)
-	} else {
-		require.NoError(t, lastErr)
-	}
+	validateError(t, lastErr, test.Name, test.ExpectedError)
 }
 
 func generateController(logger *zap.Logger) *controller.Controller {

protocol/v2/qbft/spectest/create_msg_type.go

@@ -5,15 +5,13 @@ import (
 	"testing"
 
 	"github.com/pkg/errors"
-	"github.com/stretchr/testify/require"
-
-	"github.com/ssvlabs/ssv/protocol/v2/qbft/instance"
-
 	"github.com/ssvlabs/ssv-spec/qbft"
 	specqbft "github.com/ssvlabs/ssv-spec/qbft"
 	spectests "github.com/ssvlabs/ssv-spec/qbft/spectest/tests"
 	spectypes "github.com/ssvlabs/ssv-spec/types"
 	spectestingutils "github.com/ssvlabs/ssv-spec/types/testingutils"
+	"github.com/ssvlabs/ssv/protocol/v2/qbft/instance"
+	"github.com/stretchr/testify/require"
 )
 
 type CreateMsgSpecTest struct {
@@ -49,19 +47,10 @@ func (test *CreateMsgSpecTest) RunCreateMsg(t *testing.T) {
 	default:
 		t.Fail()
 	}
+	validateError(t, err, test.Name, test.ExpectedError)
 
-	if err != nil && len(test.ExpectedError) != 0 {
-		require.EqualError(t, err, test.ExpectedError)
-		return
-	}
+	r, err := msg.GetRoot()
 	require.NoError(t, err)
-
-	r, err2 := msg.GetRoot()
-	if len(test.ExpectedError) != 0 {
-		require.EqualError(t, err2, test.ExpectedError)
-		return
-	}
-	require.NoError(t, err2)
 	require.EqualValues(t, test.ExpectedRoot, hex.EncodeToString(r[:]))
 }
 

protocol/v2/qbft/spectest/error_override.go

@@ -0,0 +1,37 @@
+package qbft
+
+import (
+	"fmt"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+// expectedErrorOverride contains mapping testName -> "error substring(s)" that will be used to
+// replace the expected error (ExpectedError string) in spec tests.
+// This mapping allows us to manually track/approve the exact differences between what spec
+// tests expect against what we actually have in our implementation so we can have details in
+// our error messages (or totally different error message altogether) without accidentally
+// diverging from the spec (which might happen if we don't compare our impl errors vs spec).
+// For simplicity, the overriding pattern is just a bunch of substrings we expect actual error
+// to contain.
+var expectedErrorOverride = map[string][]string{}
+
+// validateError checks err against expectedErr, overriding it by a set of patterns to match
+// against (defined in expectedErrorOverride) in case test testName has been mapped in this way.
+func validateError(t *testing.T, err error, testName string, expectedErr string) {
+	if len(expectedErr) == 0 {
+		require.NoError(t, err)
+		return
+	}
+
+	require.Error(t, err, expectedErr)
+
+	wantErrors := []string{expectedErr}
+	if errOverride, ok := expectedErrorOverride[testName]; ok {
+		wantErrors = errOverride
+	}
+	for _, wantError := range wantErrors {
+		require.Contains(t, err.Error(), wantError, fmt.Sprintf("testName: %s", testName))
+	}
+}

protocol/v2/qbft/spectest/msg_processing_type.go

@@ -13,14 +13,13 @@ import (
 	spectypes "github.com/ssvlabs/ssv-spec/types"
 	spectestingutils "github.com/ssvlabs/ssv-spec/types/testingutils"
 	typescomparable "github.com/ssvlabs/ssv-spec/types/testingutils/comparable"
-	"github.com/stretchr/testify/require"
-
 	"github.com/ssvlabs/ssv/logging"
 	"github.com/ssvlabs/ssv/protocol/v2/qbft"
 	"github.com/ssvlabs/ssv/protocol/v2/qbft/instance"
 	qbfttesting "github.com/ssvlabs/ssv/protocol/v2/qbft/testing"
 	protocoltesting "github.com/ssvlabs/ssv/protocol/v2/testing"
 	"github.com/ssvlabs/ssv/utils/casts"
+	"github.com/stretchr/testify/require"
 )
 
 // RunMsgProcessing processes MsgProcessingSpecTest. It probably may be removed.
@@ -58,12 +57,7 @@ func RunMsgProcessing(t *testing.T, test *spectests.MsgProcessingSpecTest) {
 			lastErr = err
 		}
 	}
-
-	if len(test.ExpectedError) != 0 {
-		require.EqualError(t, lastErr, test.ExpectedError, "expected %v, but got %v", test.ExpectedError, lastErr)
-	} else {
-		require.NoError(t, lastErr)
-	}
+	validateError(t, lastErr, test.Name, test.ExpectedError)
 
 	postRoot, err := preInstance.State.GetRoot()
 	require.NoError(t, err)

protocol/v2/qbft/spectest/msg_type.go

@@ -4,14 +4,12 @@ import (
 	"testing"
 
 	specqbft "github.com/ssvlabs/ssv-spec/qbft"
-	"github.com/stretchr/testify/require"
-
 	spectests "github.com/ssvlabs/ssv-spec/qbft/spectest/tests"
+	"github.com/stretchr/testify/require"
 )
 
 func RunMsg(t *testing.T, test *spectests.MsgSpecTest) { // using only spec struct so this test can be imported
 	var lastErr error
-
 	for i, msg := range test.Messages {
 		if err := msg.Validate(); err != nil {
 			lastErr = err
@@ -37,12 +35,5 @@ func RunMsg(t *testing.T, test *spectests.MsgSpecTest) { // using only spec stru
 			require.EqualValues(t, test.ExpectedRoots[i], r)
 		}
 	}
-
-	// check error
-	if len(test.ExpectedError) != 0 {
-		t.Log("Expected error", test.ExpectedError)
-		require.EqualError(t, lastErr, test.ExpectedError)
-	} else {
-		require.NoError(t, lastErr)
-	}
+	validateError(t, lastErr, test.Name, test.ExpectedError)
 }

protocol/v2/qbft/spectest/qbft_mapping_test.go

@@ -7,17 +7,15 @@ import (
 	"strings"
 	"testing"
 
-	"github.com/ssvlabs/ssv/exporter/convert"
-
 	spectests "github.com/ssvlabs/ssv-spec/qbft/spectest/tests"
 	"github.com/ssvlabs/ssv-spec/qbft/spectest/tests/timeout"
 	"github.com/ssvlabs/ssv-spec/types/testingutils"
-	"github.com/stretchr/testify/require"
-
+	"github.com/ssvlabs/ssv/exporter/convert"
 	"github.com/ssvlabs/ssv/logging"
 	"github.com/ssvlabs/ssv/protocol/v2/qbft/instance"
 	testing2 "github.com/ssvlabs/ssv/protocol/v2/qbft/testing"
 	protocoltesting "github.com/ssvlabs/ssv/protocol/v2/testing"
+	"github.com/stretchr/testify/require"
 )
 
 func TestQBFTMapping(t *testing.T) {

protocol/v2/qbft/spectest/timeout_type.go

@@ -7,11 +7,9 @@ import (
 
 	spectypes "github.com/ssvlabs/ssv-spec/types"
 	"github.com/ssvlabs/ssv-spec/types/testingutils"
-
 	"github.com/ssvlabs/ssv/logging"
 	"github.com/ssvlabs/ssv/protocol/v2/qbft/instance"
 	"github.com/ssvlabs/ssv/protocol/v2/qbft/roundtimer"
-
 	"github.com/stretchr/testify/require"
 )
 
@@ -27,12 +25,7 @@ type SpecTest struct {
 func RunTimeout(t *testing.T, test *SpecTest) {
 	logger := logging.TestLogger(t)
 	err := test.Pre.UponRoundTimeout(logger)
-
-	if len(test.ExpectedError) != 0 {
-		require.EqualError(t, err, test.ExpectedError)
-	} else {
-		require.NoError(t, err)
-	}
+	validateError(t, err, test.Name, test.ExpectedError)
 
 	// test calling timeout
 	timer, ok := test.Pre.GetConfig().GetTimer().(*roundtimer.TestQBFTTimer)

protocol/v2/ssv/spectest/committee_msg_processing_type.go

@@ -9,6 +9,8 @@ import (
 
 	"github.com/attestantio/go-eth2-client/spec/phase0"
 	"github.com/pkg/errors"
+	"github.com/ssvlabs/ssv-spec/ssv"
+	"github.com/ssvlabs/ssv-spec/types"
 	spectypes "github.com/ssvlabs/ssv-spec/types"
 	spectestingutils "github.com/ssvlabs/ssv-spec/types/testingutils"
 	typescomparable "github.com/ssvlabs/ssv-spec/types/testingutils/comparable"
@@ -19,13 +21,11 @@ import (
 	protocoltesting "github.com/ssvlabs/ssv/protocol/v2/testing"
 	"github.com/stretchr/testify/require"
 	"go.uber.org/zap"
-
-	"github.com/ssvlabs/ssv-spec/ssv"
-	"github.com/ssvlabs/ssv-spec/types"
 )
 
 type CommitteeSpecTest struct {
 	Name                   string
+	ParentName             string
 	Committee              *validator.Committee
 	Input                  []interface{} // Can be a types.Duty or a *types.SignedSSVMessage
 	PostDutyCommitteeRoot  string
@@ -39,16 +39,15 @@ func (test *CommitteeSpecTest) TestName() string {
 	return test.Name
 }
 
+func (test *CommitteeSpecTest) FullName() string {
+	return strings.Replace(test.ParentName+"_"+test.Name, " ", "_", -1)
+}
+
 // RunAsPartOfMultiTest runs the test as part of a MultiCommitteeSpecTest
 func (test *CommitteeSpecTest) RunAsPartOfMultiTest(t *testing.T) {
 	logger := logging.TestLogger(t)
 	lastErr := test.runPreTesting(logger)
-
-	if len(test.ExpectedError) != 0 {
-		require.EqualError(t, lastErr, test.ExpectedError)
-	} else {
-		require.NoError(t, lastErr)
-	}
+	validateError(t, lastErr, test.FullName(), test.ExpectedError)
 
 	broadcastedMsgs := make([]*types.SignedSSVMessage, 0)
 	broadcastedRoots := make([]phase0.Root, 0)
@@ -140,6 +139,7 @@ func (tests *MultiCommitteeSpecTest) Run(t *testing.T) {
 
 	for _, test := range tests.Tests {
 		t.Run(test.TestName(), func(t *testing.T) {
+			test.ParentName = tests.Name
 			test.RunAsPartOfMultiTest(t)
 		})
 	}

protocol/v2/ssv/spectest/error_override.go

@@ -0,0 +1,92 @@
+package spectest
+
+import (
+	"fmt"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+// expectedErrorOverride contains mapping testName -> "error substring(s)" that will be used to
+// replace the expected error (ExpectedError string) in spec tests.
+// This mapping allows us to manually track/approve the exact differences between what spec
+// tests expect against what we actually have in our implementation so we can have details in
+// our error messages (or totally different error message altogether) without accidentally
+// diverging from the spec (which might happen if we don't compare our impl errors vs spec).
+// For simplicity, the overriding pattern is just a bunch of substrings we expect actual error
+// to contain.
+var expectedErrorOverride = map[string][]string{
+	"consensus_past_message_attester": {
+		"no runner found for consensus message's slot",
+	},
+	"consensus_past_message_sync_committee": {
+		"no runner found for consensus message's slot",
+	},
+	"consensus_past_message_attester_and_sync_committee": {
+		"no runner found for consensus message's slot",
+	},
+	"consensus_future_decided_no_running_instance_attester": {
+		"no runner found for consensus message's slot",
+	},
+	"consensus_future_decided_no_running_instance_sync_committee": {
+		"no runner found for consensus message's slot",
+	},
+	"consensus_future_decided_no_running_instance_attester_and_sync_committee": {
+		"no runner found for consensus message's slot",
+	},
+	"post_consensus_invalid_msg_slot_attester": {
+		"no runner found for post consensus sig message's slot",
+	},
+	"post_consensus_invalid_msg_slot_sync_committee": {
+		"no runner found for post consensus sig message's slot",
+	},
+	"post_consensus_invalid_msg_slot_attester_and_sync_committee": {
+		"no runner found for post consensus sig message's slot",
+	},
+	"consensus_future_decided_attester": {
+		"no runner found for consensus message's slot",
+	},
+	"consensus_future_decided_sync_committee": {
+		"no runner found for consensus message's slot",
+	},
+	"consensus_future_decided_attester_and_sync_committee": {
+		"no runner found for consensus message's slot",
+	},
+	"consensus_future_message_attester": {
+		"no runner found for consensus message's slot",
+	},
+	"consensus_future_message_sync_committee": {
+		"no runner found for consensus message's slot",
+	},
+	"consensus_future_message_attester_sync_committee": {
+		"no runner found for consensus message's slot",
+	},
+	"past_msg_duty_does_not_exist_30_attestation": {
+		"no runner found for consensus message's slot",
+	},
+	"past_msg_duty_does_not_exist_30_sync_committee": {
+		"no runner found for consensus message's slot",
+	},
+	"past_msg_duty_does_not_exist_30_attestation_30_sync_committee": {
+		"no runner found for consensus message's slot",
+	},
+}
+
+// validateError checks err against expectedErr, overriding it by a set of patterns to match
+// against (defined in expectedErrorOverride) in case test testName has been mapped in this way.
+func validateError(t *testing.T, err error, testName string, expectedErr string) {
+	if len(expectedErr) == 0 {
+		require.NoError(t, err)
+		return
+	}
+
+	require.Error(t, err, expectedErr)
+
+	wantErrors := []string{expectedErr}
+	if errOverride, ok := expectedErrorOverride[testName]; ok {
+		wantErrors = errOverride
+	}
+	for _, wantError := range wantErrors {
+		require.Contains(t, err.Error(), wantError, fmt.Sprintf("testName: %s", testName))
+	}
+}