From 51d74542dfc94bb2234d842db58ef4035077265a Mon Sep 17 00:00:00 2001 From: Khalil Claybon Date: Fri, 1 Mar 2024 13:31:14 -0500 Subject: [PATCH 01/23] enable flakey test to run in CI --- .../rpc_inspector/validation_inspector_test.go | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/insecure/integration/functional/test/gossipsub/rpc_inspector/validation_inspector_test.go b/insecure/integration/functional/test/gossipsub/rpc_inspector/validation_inspector_test.go index a5b8b86ed3f..6fda06727f8 100644 --- a/insecure/integration/functional/test/gossipsub/rpc_inspector/validation_inspector_test.go +++ b/insecure/integration/functional/test/gossipsub/rpc_inspector/validation_inspector_test.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "math" + "os" "testing" "time" @@ -11,6 +12,7 @@ import ( pb "github.com/libp2p/go-libp2p-pubsub/pb" pubsub_pb "github.com/libp2p/go-libp2p-pubsub/pb" "github.com/libp2p/go-libp2p/core/peer" + "github.com/rs/zerolog" mockery "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" corrupt "github.com/yhassanzadeh13/go-libp2p-pubsub" @@ -964,7 +966,7 @@ func TestValidationInspector_InspectRpcPublishMessages(t *testing.T) { // The victim node is configured to use the GossipSubInspector to detect spam and the scoring system to mitigate spam. // The test ensures that the victim node is disconnected from the spammer node on the GossipSub mesh after the spam detection is triggered. func TestGossipSubSpamMitigationIntegration(t *testing.T) { - unittest.SkipUnless(t, unittest.TEST_FLAKY, "https://github.com/dapperlabs/flow-go/issues/6949") + //unittest.SkipUnless(t, unittest.TEST_FLAKY, "https://github.com/dapperlabs/flow-go/issues/6949") t.Run("gossipsub spam mitigation invalid grafts", func(t *testing.T) { testGossipSubSpamMitigationIntegration(t, p2pmsg.CtrlMsgGraft) }) @@ -983,6 +985,8 @@ func TestGossipSubSpamMitigationIntegration(t *testing.T) { // The victim node is configured to use the GossipSubInspector to detect spam and the scoring system to mitigate spam. // The test ensures that the victim node is disconnected from the spammer node on the GossipSub mesh after the spam detection is triggered. func testGossipSubSpamMitigationIntegration(t *testing.T, msgType p2pmsg.ControlMessageType) { + logger := zerolog.New(os.Stdout).Level(zerolog.TraceLevel) + logger.Trace().Msg(fmt.Sprintf("STARTING GOSSIPSUB SPAM MITIGATION: %s", msgType.String())) idProvider := mock.NewIdentityProvider(t) sporkID := unittest.IdentifierFixture() spammer := corruptlibp2p.NewGossipSubRouterSpammer(t, sporkID, flow.RoleConsensus, idProvider) @@ -994,12 +998,15 @@ func testGossipSubSpamMitigationIntegration(t *testing.T, msgType p2pmsg.Control // set the scoring parameters to be more aggressive to speed up the test cfg.NetworkConfig.GossipSub.RpcTracer.ScoreTracerInterval = 100 * time.Millisecond cfg.NetworkConfig.GossipSub.ScoringParameters.ScoringRegistryParameters.AppSpecificScore.ScoreTTL = 100 * time.Millisecond + cfg.NetworkConfig.GossipSub.ScoringParameters.ScoringRegistryParameters.SpamRecordCache.Decay.MaximumSpamPenaltyDecayFactor = .99 + victimNode, victimId := p2ptest.NodeFixture(t, sporkID, t.Name(), idProvider, p2ptest.WithRole(flow.RoleConsensus), - p2ptest.OverrideFlowConfig(cfg)) + p2ptest.OverrideFlowConfig(cfg), + p2ptest.WithLogger(logger)) ids := flow.IdentityList{&victimId, &spammer.SpammerId} idProvider.On("ByPeerID", mockery.Anything).Return(func(peerId peer.ID) *flow.Identity { @@ -1105,4 +1112,5 @@ func testGossipSubSpamMitigationIntegration(t *testing.T, msgType p2pmsg.Control func() interface{} { return unittest.ProposalFixture() }) + logger.Trace().Msg(fmt.Sprintf("FINISHED GOSSIPSUB SPAM MITIGATION: %s", msgType.String())) } From 949888a873545bb491e2493b3ba97bf746074253 Mon Sep 17 00:00:00 2001 From: Khalil Claybon Date: Fri, 1 Mar 2024 13:51:10 -0500 Subject: [PATCH 02/23] add debug log --- .../rpc_inspector/validation_inspector_test.go | 2 +- .../functional/test/gossipsub/scoring/scoring_test.go | 10 +++++++--- .../validation/control_message_validation_inspector.go | 1 + 3 files changed, 9 insertions(+), 4 deletions(-) diff --git a/insecure/integration/functional/test/gossipsub/rpc_inspector/validation_inspector_test.go b/insecure/integration/functional/test/gossipsub/rpc_inspector/validation_inspector_test.go index 6fda06727f8..7aeb39e5f97 100644 --- a/insecure/integration/functional/test/gossipsub/rpc_inspector/validation_inspector_test.go +++ b/insecure/integration/functional/test/gossipsub/rpc_inspector/validation_inspector_test.go @@ -1074,7 +1074,7 @@ func testGossipSubSpamMitigationIntegration(t *testing.T, msgType p2pmsg.Control case p2pmsg.CtrlMsgPrune: unknownTopicSpam = spammer.GenerateCtlMessages(int(spamCtrlMsgCount), p2ptest.WithPrune(spamRpcCount, unknownTopic.String())) malformedTopicSpam = spammer.GenerateCtlMessages(int(spamCtrlMsgCount), p2ptest.WithPrune(spamRpcCount, malformedTopic.String())) - invalidSporkIDTopicSpam = spammer.GenerateCtlMessages(int(spamCtrlMsgCount), p2ptest.WithGraft(spamRpcCount, invalidSporkIDTopic.String())) + invalidSporkIDTopicSpam = spammer.GenerateCtlMessages(int(spamCtrlMsgCount), p2ptest.WithPrune(spamRpcCount, invalidSporkIDTopic.String())) duplicateTopicSpam = spammer.GenerateCtlMessages(int(spamCtrlMsgCount), // sets duplicate to +2 above the threshold to ensure that the victim node will penalize the spammer node p2ptest.WithPrune(cfg.NetworkConfig.GossipSub.RpcInspector.Validation.GraftPrune.DuplicateTopicIdThreshold+2, duplicateTopic.String())) case p2pmsg.CtrlMsgIHave: diff --git a/insecure/integration/functional/test/gossipsub/scoring/scoring_test.go b/insecure/integration/functional/test/gossipsub/scoring/scoring_test.go index fc16e985312..e3ad27ac76d 100644 --- a/insecure/integration/functional/test/gossipsub/scoring/scoring_test.go +++ b/insecure/integration/functional/test/gossipsub/scoring/scoring_test.go @@ -2,12 +2,14 @@ package scoring import ( "context" + "os" "testing" "time" pubsub "github.com/libp2p/go-libp2p-pubsub" pubsub_pb "github.com/libp2p/go-libp2p-pubsub/pb" "github.com/libp2p/go-libp2p/core/peer" + "github.com/rs/zerolog" "github.com/stretchr/testify/require" "github.com/onflow/flow-go/config" @@ -27,7 +29,7 @@ import ( // a spammer peer, the victim will eventually penalize the spammer and stop receiving messages from them. // Note: the term integration is used here because it requires integrating all components of the libp2p stack. func TestGossipSubInvalidMessageDelivery_Integration(t *testing.T) { - unittest.SkipUnless(t, unittest.TEST_FLAKY, "https://github.com/dapperlabs/flow-go/issues/6949") + //unittest.SkipUnless(t, unittest.TEST_FLAKY, "https://github.com/dapperlabs/flow-go/issues/6949") tt := []struct { name string spamMsgFactory func(spammerId peer.ID, victimId peer.ID, topic channels.Topic) *pubsub_pb.Message @@ -96,6 +98,7 @@ func TestGossipSubInvalidMessageDelivery_Integration(t *testing.T) { // - t: the test instance. // - spamMsgFactory: a function that creates unique invalid messages to spam the victim with. func testGossipSubInvalidMessageDeliveryScoring(t *testing.T, spamMsgFactory func(peer.ID, peer.ID, channels.Topic) *pubsub_pb.Message) { + logger := zerolog.New(os.Stdout).Level(zerolog.TraceLevel) role := flow.RoleConsensus sporkId := unittest.IdentifierFixture() @@ -118,7 +121,8 @@ func testGossipSubInvalidMessageDeliveryScoring(t *testing.T, spamMsgFactory fun t.Name(), idProvider, p2ptest.WithRole(role), - p2ptest.OverrideFlowConfig(cfg)) + p2ptest.OverrideFlowConfig(cfg), + p2ptest.WithLogger(logger)) ids := flow.IdentityList{&spammer.SpammerId, &victimIdentity} idProvider.SetIdentities(ids) @@ -143,7 +147,7 @@ func testGossipSubInvalidMessageDeliveryScoring(t *testing.T, spamMsgFactory fun msgs = append(msgs, spamMsgFactory(spammer.SpammerNode.ID(), victimNode.ID(), blockTopic)) } - // sends all 2000 spam messages to the victim node over 1 RPC. + // sends all 3000 spam messages to the victim node over 1 RPC. spammer.SpamControlMessage(t, victimNode, spammer.GenerateCtlMessages(1), msgs...) diff --git a/network/p2p/inspector/validation/control_message_validation_inspector.go b/network/p2p/inspector/validation/control_message_validation_inspector.go index ffdabe38ce9..15ce2344a5f 100644 --- a/network/p2p/inspector/validation/control_message_validation_inspector.go +++ b/network/p2p/inspector/validation/control_message_validation_inspector.go @@ -1098,6 +1098,7 @@ func (c *ControlMsgValidationInspector) logAndDistributeAsyncInspectErrs(req *In default: c.notificationConsumer.OnInvalidControlMessageNotification(p2p.NewInvalidControlMessageNotification(req.Peer, ctlMsgType, err, count, topicType)) lg.Error().Msg("rpc control message async inspection failed, notification sent") + lg.Trace().Msg("rpc control message async inspection failed, notification sent") c.metrics.OnInvalidControlMessageNotificationSent() } } From 95d64212965c5a084ec827e564a7c7eb39aab2e9 Mon Sep 17 00:00:00 2001 From: Khalil Claybon Date: Fri, 1 Mar 2024 15:13:07 -0500 Subject: [PATCH 03/23] Update validation_inspector_test.go --- .../test/gossipsub/rpc_inspector/validation_inspector_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/insecure/integration/functional/test/gossipsub/rpc_inspector/validation_inspector_test.go b/insecure/integration/functional/test/gossipsub/rpc_inspector/validation_inspector_test.go index 7aeb39e5f97..6811720defd 100644 --- a/insecure/integration/functional/test/gossipsub/rpc_inspector/validation_inspector_test.go +++ b/insecure/integration/functional/test/gossipsub/rpc_inspector/validation_inspector_test.go @@ -1030,7 +1030,7 @@ func testGossipSubSpamMitigationIntegration(t *testing.T, msgType p2pmsg.Control } }) - spamRpcCount := 50000 // total number of individual rpc messages to send + spamRpcCount := 1000 // total number of individual rpc messages to send spamCtrlMsgCount := int64(1000) // total number of control messages to send on each RPC // unknownTopic is an unknown topic to the victim node but shaped like a valid topic (i.e., it has the correct prefix and spork ID). From ac241f6e76a8dfadce845261e1be35b80b7a8e0e Mon Sep 17 00:00:00 2001 From: Khalil Claybon Date: Fri, 1 Mar 2024 15:44:35 -0500 Subject: [PATCH 04/23] Update validation_inspector_test.go --- .../validation_inspector_test.go | 30 ++++++++----------- 1 file changed, 13 insertions(+), 17 deletions(-) diff --git a/insecure/integration/functional/test/gossipsub/rpc_inspector/validation_inspector_test.go b/insecure/integration/functional/test/gossipsub/rpc_inspector/validation_inspector_test.go index 6811720defd..953504c47b9 100644 --- a/insecure/integration/functional/test/gossipsub/rpc_inspector/validation_inspector_test.go +++ b/insecure/integration/functional/test/gossipsub/rpc_inspector/validation_inspector_test.go @@ -959,23 +959,19 @@ func TestValidationInspector_InspectRpcPublishMessages(t *testing.T) { require.Equal(t, uint64(1), notificationCount.Load()) } -// TestGossipSubSpamMitigationIntegration tests that the spam mitigation feature of GossipSub is working as expected. -// The test puts toghether the spam detection (through the GossipSubInspector) and the spam mitigation (through the -// scoring system) and ensures that the mitigation is triggered when the spam detection detects spam. -// The test scenario involves a spammer node that sends a large number of control messages to a victim node. -// The victim node is configured to use the GossipSubInspector to detect spam and the scoring system to mitigate spam. -// The test ensures that the victim node is disconnected from the spammer node on the GossipSub mesh after the spam detection is triggered. -func TestGossipSubSpamMitigationIntegration(t *testing.T) { - //unittest.SkipUnless(t, unittest.TEST_FLAKY, "https://github.com/dapperlabs/flow-go/issues/6949") - t.Run("gossipsub spam mitigation invalid grafts", func(t *testing.T) { - testGossipSubSpamMitigationIntegration(t, p2pmsg.CtrlMsgGraft) - }) - t.Run("gossipsub spam mitigation invalid prunes", func(t *testing.T) { - testGossipSubSpamMitigationIntegration(t, p2pmsg.CtrlMsgPrune) - }) - t.Run("gossipsub spam mitigation invalid ihaves", func(t *testing.T) { - testGossipSubSpamMitigationIntegration(t, p2pmsg.CtrlMsgIHave) - }) +// TestGossipSubSpamMitigationIntegration_Grafts tests that the spam mitigation feature of GossipSub is working as expected for Graft control messages. +func TestGossipSubSpamMitigationIntegration_Grafts(t *testing.T) { + testGossipSubSpamMitigationIntegration(t, p2pmsg.CtrlMsgGraft) +} + +// TestGossipSubSpamMitigationIntegration_Prunes tests that the spam mitigation feature of GossipSub is working as expected for Prune control messages. +func TestGossipSubSpamMitigationIntegration_Prunes(t *testing.T) { + testGossipSubSpamMitigationIntegration(t, p2pmsg.CtrlMsgPrune) +} + +// TestGossipSubSpamMitigationIntegration_IHaves tests that the spam mitigation feature of GossipSub is working as expected for IHaves control messages. +func TestGossipSubSpamMitigationIntegration_IHaves(t *testing.T) { + testGossipSubSpamMitigationIntegration(t, p2pmsg.CtrlMsgIHave) } // testGossipSubSpamMitigationIntegration tests that the spam mitigation feature of GossipSub is working as expected. From 30b8c9d0a37c18e748cb013e4aee3bbc8139becf Mon Sep 17 00:00:00 2001 From: Khalil Claybon Date: Fri, 1 Mar 2024 16:14:45 -0500 Subject: [PATCH 05/23] Update scoring_test.go --- .../functional/test/gossipsub/scoring/scoring_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/insecure/integration/functional/test/gossipsub/scoring/scoring_test.go b/insecure/integration/functional/test/gossipsub/scoring/scoring_test.go index e3ad27ac76d..b1533356280 100644 --- a/insecure/integration/functional/test/gossipsub/scoring/scoring_test.go +++ b/insecure/integration/functional/test/gossipsub/scoring/scoring_test.go @@ -29,7 +29,7 @@ import ( // a spammer peer, the victim will eventually penalize the spammer and stop receiving messages from them. // Note: the term integration is used here because it requires integrating all components of the libp2p stack. func TestGossipSubInvalidMessageDelivery_Integration(t *testing.T) { - //unittest.SkipUnless(t, unittest.TEST_FLAKY, "https://github.com/dapperlabs/flow-go/issues/6949") + unittest.SkipUnless(t, unittest.TEST_FLAKY, "https://github.com/dapperlabs/flow-go/issues/6949") tt := []struct { name string spamMsgFactory func(spammerId peer.ID, victimId peer.ID, topic channels.Topic) *pubsub_pb.Message From ed4bb30ad8cb22dc1ea253f22ca70b46c7ca4852 Mon Sep 17 00:00:00 2001 From: Khalil Claybon Date: Mon, 4 Mar 2024 10:41:09 -0500 Subject: [PATCH 06/23] update test matrix generator script - allow matrix generation based on package configuration - add tests - document code --- .github/workflows/ci.yml | 2 +- .../default-test-matrix-config.json | 33 +++ .../insecure-module-test-matrix-config.json | 10 + ...integration-module-test-matrix-config.json | 9 + tools/test_matrix_generator/matrix.go | 235 ++++++++++++++++++ tools/test_matrix_generator/matrix_test.go | 162 ++++++++++++ 6 files changed, 450 insertions(+), 1 deletion(-) create mode 100644 tools/test_matrix_generator/default-test-matrix-config.json create mode 100644 tools/test_matrix_generator/insecure-module-test-matrix-config.json create mode 100644 tools/test_matrix_generator/integration-module-test-matrix-config.json create mode 100644 tools/test_matrix_generator/matrix.go create mode 100644 tools/test_matrix_generator/matrix_test.go diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index de4e11e6554..0f4415a1283 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -89,7 +89,7 @@ jobs: cache: true - name: Set Test Matrix id: set-test-matrix - run: go run utils/test_matrix/test_matrix.go admin cmd consensus engine/access engine/collection engine/common engine/consensus engine/execution/ingestion:buildjet-8vcpu-ubuntu-2204 engine/execution/computation engine/execution engine/verification engine:buildjet-4vcpu-ubuntu-2204 fvm ledger module/dkg module:buildjet-4vcpu-ubuntu-2204 network/alsp network/test/cohort1:buildjet-16vcpu-ubuntu-2204 network/test/cohort2:buildjet-4vcpu-ubuntu-2204 network/p2p/connection network/p2p/node:buildjet-4vcpu-ubuntu-2204 network/p2p/scoring network/p2p network state storage utils + run: go run tools/test_matrix_generator/matrix.go unit-test: name: Unit Tests (${{ matrix.targets.name }}) diff --git a/tools/test_matrix_generator/default-test-matrix-config.json b/tools/test_matrix_generator/default-test-matrix-config.json new file mode 100644 index 00000000000..37fd79ead20 --- /dev/null +++ b/tools/test_matrix_generator/default-test-matrix-config.json @@ -0,0 +1,33 @@ +{ + "includeOthers": true, + "packages": [ + {"name": "admin"}, + {"name": "cmd"}, + {"name": "consensus"}, + {"name": "fvm"}, + {"name": "ledger"}, + {"name": "state"}, + {"name": "storage"}, + {"name": "utils"}, + {"name": "engine", "runner": "buildjet-4vcpu-ubuntu-2204" ,"subpackages": [ + {"name": "engine/access"}, + {"name": "engine/collection"}, + {"name": "engine/common"}, + {"name": "engine/consensus"}, + {"name": "engine/execution/computation"}, + {"name": "engine/execution"}, + {"name": "engine/verification"}, + {"name": "engine/execution/ingestion", "runner": "buildjet-8vcpu-ubuntu-2204"} + ]}, + {"name": "module", "runner": "buildjet-4vcpu-ubuntu-2204" ,"subpackages": [{"name": "module/dkg"}]}, + {"name": "network", "subpackages": [ + {"name": "network/alsp"}, + {"name": "network/p2p/connection"}, + {"name": "network/p2p/scoring"}, + {"name": "network/p2p"}, + {"name": "network/test/cohort1", "runner": "buildjet-16vcpu-ubuntu-2204"}, + {"name": "network/test/cohort2", "runner": "buildjet-4vcpu-ubuntu-2204"}, + {"name": "network/p2p/node", "runner": "buildjet-4vcpu-ubuntu-2204"} + ]} + ] +} diff --git a/tools/test_matrix_generator/insecure-module-test-matrix-config.json b/tools/test_matrix_generator/insecure-module-test-matrix-config.json new file mode 100644 index 00000000000..0b63293a31f --- /dev/null +++ b/tools/test_matrix_generator/insecure-module-test-matrix-config.json @@ -0,0 +1,10 @@ +{ + "packagesPath": "./insecure", + "includeOthers": false, + "packages": [ + {"name": "insecure", "runner": "buildjet-4vcpu-ubuntu-2204" ,"subpackages": [ + {"name": "insecure/integration/functional/test/gossipsub/rpc_inspector", "runner": "buildjet-8vcpu-ubuntu-2204"}, + {"name": "insecure/integration/functional/test/gossipsub/scoring", "runner": "buildjet-8vcpu-ubuntu-2204"} + ]} + ] +} diff --git a/tools/test_matrix_generator/integration-module-test-matrix-config.json b/tools/test_matrix_generator/integration-module-test-matrix-config.json new file mode 100644 index 00000000000..af53cfa1ec8 --- /dev/null +++ b/tools/test_matrix_generator/integration-module-test-matrix-config.json @@ -0,0 +1,9 @@ +{ + "packagesPath": "./integration", + "includeOthers": false, + "packages": [{ + "name": "integration", + "runner": "buildjet-4vcpu-ubuntu-2204", + "exclude": ["integration/tests"] + }] +} diff --git a/tools/test_matrix_generator/matrix.go b/tools/test_matrix_generator/matrix.go new file mode 100644 index 00000000000..fd0915f622d --- /dev/null +++ b/tools/test_matrix_generator/matrix.go @@ -0,0 +1,235 @@ +package main + +import ( + "bytes" + _ "embed" + "encoding/json" + "fmt" + "strings" + + "github.com/spf13/pflag" + "golang.org/x/tools/go/packages" +) + +var ( + //go:embed default-test-matrix-config.json + defaultTestMatrixConfig string + + //go:embed insecure-module-test-matrix-config.json + insecureModuleTestMatrixConfig string + + //go:embed integration-module-test-matrix-config.json + integrationModuleTestMatrixConfig string + + matrixConfigFile string +) + +// flowGoPackage configuration for a package to be tested. +type flowGoPackage struct { + // Name the name of the package where test are located. + Name string `json:"name"` + // Runner the runner used for the top level github actions job that runs the tests all the tests in the parent package. + Runner string `json:"runner,omitempty"` + // Exclude list of packages to exclude from top level parent package test matrix. + Exclude []string `json:"exclude,omitempty"` + // Subpackages list of subpackages of the parent package that should be run in their own github actions job. + Subpackages []*subpackage `json:"subpackages,omitempty"` +} + +// subpackage configuration for a subpackage. +type subpackage struct { + Name string `json:"name"` + Runner string `json:"runner,omitempty"` +} + +// config the test matrix configuration for a package. +type config struct { + // PackagesPath director where to load packages from. + PackagesPath string `json:"packagesPath,omitempty"` + // IncludeOthers when set to true will put all packages and subpackages of the packages path into a test matrix that will run in a job called others. + IncludeOthers bool `json:"includeOthers,omitempty"` + // Packages configurations for all packages that test should be run from. + Packages []*flowGoPackage `json:"packages"` +} + +// testMatrix represents a single GitHub Actions test matrix combination that consists of a name and a list of flow-go packages associated with that name. +type testMatrix struct { + Name string `json:"name"` + Packages string `json:"packages"` + Runner string `json:"runner"` +} + +// newTestMatrix returns a new testMatrix, if runner is empty "" set the runner to the defaultCIRunner. +func newTestMatrix(name, runner string) *testMatrix { + t := &testMatrix{ + Name: name, + Packages: "", + Runner: runner, + } + + if t.Runner == "" { + t.Runner = defaultCIRunner + } + + return t +} + +const flowPackagePrefix = "github.com/onflow/flow-go/" +const ciMatrixName = "dynamicMatrix" +const defaultCIRunner = "ubuntu-latest" + +// Generates a list of packages to test that will be passed to GitHub Actions +func main() { + // Parse command-line arguments + pflag.Parse() + + var configFile string + switch matrixConfigFile { + case "insecure": + configFile = insecureModuleTestMatrixConfig + case "integration": + configFile = integrationModuleTestMatrixConfig + default: + configFile = defaultTestMatrixConfig + } + + packageConfig := loadPackagesConfig(configFile) + + testMatrices := buildTestMatrices(packageConfig, listAllFlowPackages) + printCIString(testMatrices) +} + +// printCIString encodes the test matrices and prints the json string to stdout. The CI runner will read this json string +// and make the data available for our github workflows. +func printCIString(testMatrices []*testMatrix) { + // generate JSON output that will be read in by CI matrix + // can't use json.MarshalIndent because fromJSON() in CI can’t read JSON with any spaces + b, err := json.Marshal(testMatrices) + if err != nil { + panic(fmt.Errorf("failed to marshal test matrices json: %w", err)) + } + // this string will be read by CI to generate groups of tests to run in separate CI jobs + testMatrixStr := "::set-output name=" + ciMatrixName + "::" + string(b) + // very important to add newline character at the end of the compacted JSON - otherwise fromJSON() in CI will throw unmarshalling error + fmt.Println(testMatrixStr) +} + +// buildTestMatrices builds the test matrices. +func buildTestMatrices(packageConfig *config, flowPackages func(dir string) []*packages.Package) []*testMatrix { + testMatrices := make([]*testMatrix, 0) + seenPaths := make(map[string]struct{}) + seenPath := func(p string) { + seenPaths[p] = struct{}{} + } + seen := func(p string) bool { + _, seen := seenPaths[p] + return seen + } + + for _, topLevelPkg := range packageConfig.Packages { + allPackages := flowPackages(topLevelPkg.Name) + // first build test matrix for each of the subpackages and mark all complete paths seen + subPkgMatrices := processSubpackages(topLevelPkg.Subpackages, allPackages, seenPath) + testMatrices = append(testMatrices, subPkgMatrices...) + // now build top level test matrix + topLevelTestMatrix := processTopLevelPackage(topLevelPkg, allPackages, seenPath, seen) + testMatrices = append(testMatrices, topLevelTestMatrix) + } + + // any packages left out of the explicit Packages field will be run together as "others" from the config PackagesPath + if packageConfig.IncludeOthers { + allPkgs := flowPackages(packageConfig.PackagesPath) + if othersTestMatrix := buildOthersTestMatrix(allPkgs, seen); othersTestMatrix != nil { + testMatrices = append(testMatrices, othersTestMatrix) + } + } + return testMatrices +} + +// processSubpackages creates a test matrix for all subpackages provided. +func processSubpackages(subPkgs []*subpackage, allPkgs []*packages.Package, seenPath func(p string)) []*testMatrix { + testMatrices := make([]*testMatrix, 0) + for _, subPkg := range subPkgs { + pkgPath := fullGoPackagePath(subPkg.Name) + subPkgTestMatrix := newTestMatrix(subPkg.Name, subPkg.Runner) + // this is the list of allPackages that used with the go test command + var testPkgStrBuilder strings.Builder + for _, p := range allPkgs { + if strings.HasPrefix(p.PkgPath, pkgPath) { + testPkgStrBuilder.WriteString(fmt.Sprintf("%s ", p.PkgPath)) + seenPath(p.PkgPath) + } + } + subPkgTestMatrix.Packages = testPkgStrBuilder.String() + testMatrices = append(testMatrices, subPkgTestMatrix) + } + return testMatrices +} + +// processTopLevelPackages creates test matrix for the top level package excluding any packages from the exclude list. +func processTopLevelPackage(pkg *flowGoPackage, allPkgs []*packages.Package, seenPath func(p string), seen func(p string) bool) *testMatrix { + topLevelTestMatrix := newTestMatrix(pkg.Name, pkg.Runner) + var topLevelTestPkgStrBuilder strings.Builder + for _, p := range allPkgs { + if !seen(p.PkgPath) { + includePkg := true + for _, exclude := range pkg.Exclude { + if strings.HasPrefix(p.PkgPath, fullGoPackagePath(exclude)) { + includePkg = false + } + } + + if includePkg && strings.HasPrefix(p.PkgPath, fullGoPackagePath(pkg.Name)) { + topLevelTestPkgStrBuilder.WriteString(fmt.Sprintf("%s ", p.PkgPath)) + seenPath(p.PkgPath) + } + } + } + topLevelTestMatrix.Packages = topLevelTestPkgStrBuilder.String() + return topLevelTestMatrix +} + +// buildOthersTestMatrix builds an others test matrix that includes all packages in a path not explicitly set in the packages list of a config. +func buildOthersTestMatrix(allPkgs []*packages.Package, seen func(p string) bool) *testMatrix { + othersTestMatrix := newTestMatrix("others", "") + var othersTestPkgStrBuilder strings.Builder + for _, otherPkg := range allPkgs { + if !seen(otherPkg.PkgPath) { + othersTestPkgStrBuilder.WriteString(fmt.Sprintf("%s ", otherPkg.PkgPath)) + } + } + + if othersTestPkgStrBuilder.Len() > 0 { + othersTestMatrix.Packages = othersTestPkgStrBuilder.String() + return othersTestMatrix + } + + return nil +} + +func listAllFlowPackages(dir string) []*packages.Package { + flowPackages, err := packages.Load(&packages.Config{Dir: dir}, "./...") + if err != nil { + panic(err) + } + return flowPackages +} + +func loadPackagesConfig(configFile string) *config { + var packageConfig config + buf := bytes.NewBufferString(configFile) + err := json.NewDecoder(buf).Decode(&packageConfig) + if err != nil { + panic(fmt.Errorf("failed to decode package config json %w: %s", err, configFile)) + } + return &packageConfig +} + +func fullGoPackagePath(pkg string) string { + return fmt.Sprintf("%s%s", flowPackagePrefix, pkg) +} + +func init() { + // Add flags to the FlagSet + pflag.StringVarP(&matrixConfigFile, "config", "c", "", "the config file used to generate the test matrix") +} diff --git a/tools/test_matrix_generator/matrix_test.go b/tools/test_matrix_generator/matrix_test.go new file mode 100644 index 00000000000..d01ca085031 --- /dev/null +++ b/tools/test_matrix_generator/matrix_test.go @@ -0,0 +1,162 @@ +package main + +import ( + "fmt" + "testing" + + "github.com/stretchr/testify/require" + "golang.org/x/tools/go/packages" +) + +// TestLoadPackagesConfig ensure packages config json loads as expected. +func TestLoadPackagesConfig(t *testing.T) { + configFile := `{"packagesPath": ".", "includeOthers": true, "packages": [{"name": "testPackage"}]}` + config := loadPackagesConfig(configFile) + if config.PackagesPath != "." || !config.IncludeOthers || len(config.Packages) != 1 { + t.Errorf("loadPackagesConfig failed for valid input") + } + + invalidConfigFile := "invalidJSON" + defer func() { + if recover() == nil { + t.Errorf("loadPackagesConfig did not panic for invalid JSON input") + } + }() + loadPackagesConfig(invalidConfigFile) +} + +// TestBuildMatrices ensures test matrices are built from config json as expected. +func TestBuildMatrices(t *testing.T) { + t.Run("top level package only default runner", func(t *testing.T) { + name := "counter" + configFile := fmt.Sprintf(`{"packagesPath": ".", "includeOthers": true, "packages": [{"name": "%s"}]}`, name) + allPackges := goPackageFixture("counter/count", "counter/print/int", "counter/log") + cfg := loadPackagesConfig(configFile) + matrices := buildTestMatrices(cfg, func(dir string) []*packages.Package { + return allPackges + }) + require.Equal(t, name, matrices[0].Name) + require.Equal(t, defaultCIRunner, matrices[0].Runner) + require.Equal(t, fmt.Sprintf("%s %s %s ", allPackges[0].PkgPath, allPackges[1].PkgPath, allPackges[2].PkgPath), matrices[0].Packages) + fmt.Println(matrices[0].Name, matrices[0].Runner, matrices[0].Packages) + }) + t.Run("top level package only override runner", func(t *testing.T) { + name := "counter" + runner := "buildjet-4vcpu-ubuntu-2204" + configFile := fmt.Sprintf(`{"packagesPath": ".", "packages": [{"name": "%s", "runner": "%s"}]}`, name, runner) + allPackges := goPackageFixture("counter/count", "counter/print/int", "counter/log") + cfg := loadPackagesConfig(configFile) + matrices := buildTestMatrices(cfg, func(dir string) []*packages.Package { + return allPackges + }) + require.Equal(t, name, matrices[0].Name) + require.Equal(t, runner, matrices[0].Runner) + require.Equal(t, fmt.Sprintf("%s %s %s ", allPackges[0].PkgPath, allPackges[1].PkgPath, allPackges[2].PkgPath), matrices[0].Packages) + fmt.Println(matrices[0].Name, matrices[0].Runner, matrices[0].Packages) + }) + t.Run("top level package with sub packages include others", func(t *testing.T) { + topLevelPkgName := "network" + subPkg1 := "network/p2p/node" + subPkg2 := "module/chunks" + subPkg3 := "crypto/hash" + subPkg4 := "model/bootstrap" + subPkg1Runner := "buildjet-4vcpu-ubuntu-2204" + configFile := fmt.Sprintf(` + {"packagesPath": ".", "includeOthers": true, "packages": [{"name": "%s", "subpackages": [{"name": "%s", "runner": "%s"}, {"name": "%s"}, {"name": "%s"}, {"name": "%s"}]}]}`, + topLevelPkgName, subPkg1, subPkg1Runner, subPkg2, subPkg3, subPkg4) + allPackges := goPackageFixture( + "network", + "network/alsp", + "network/cache", + "network/channels", + "network/p2p/node", + "network/p2p/node/internal", + "module", + "module/chunks/chunky", + "crypto/hash", + "crypto/random", + "crypto/hash/ecc", + "model/bootstrap", + "model/bootstrap/info", + "model", + ) + cfg := loadPackagesConfig(configFile) + matrices := buildTestMatrices(cfg, func(dir string) []*packages.Package { + return allPackges + }) + require.Len(t, matrices, 6) + for _, matrix := range matrices { + switch matrix.Name { + case topLevelPkgName: + require.Equal(t, defaultCIRunner, matrix.Runner) + require.Equal(t, fmt.Sprintf("%s %s %s %s ", allPackges[0].PkgPath, allPackges[1].PkgPath, allPackges[2].PkgPath, allPackges[3].PkgPath), matrix.Packages) + case subPkg1: + require.Equal(t, subPkg1Runner, matrix.Runner) + require.Equal(t, fmt.Sprintf("%s %s ", allPackges[4].PkgPath, allPackges[5].PkgPath), matrix.Packages) + case subPkg2: + require.Equal(t, defaultCIRunner, matrix.Runner) + require.Equal(t, fmt.Sprintf("%s ", allPackges[7].PkgPath), matrix.Packages) + case subPkg3: + require.Equal(t, defaultCIRunner, matrix.Runner) + require.Equal(t, fmt.Sprintf("%s %s ", allPackges[8].PkgPath, allPackges[10].PkgPath), matrix.Packages) + case subPkg4: + require.Equal(t, defaultCIRunner, matrix.Runner) + require.Equal(t, fmt.Sprintf("%s %s ", allPackges[11].PkgPath, allPackges[12].PkgPath), matrix.Packages) + case "others": + require.Equal(t, defaultCIRunner, matrix.Runner) + require.Equal(t, fmt.Sprintf("%s %s %s ", allPackges[6].PkgPath, allPackges[9].PkgPath, allPackges[13].PkgPath), matrix.Packages) + default: + require.Fail(t, fmt.Sprintf("unexpected matrix name: %s", matrix.Name)) + } + } + }) + t.Run("top level package with sub packages and exclude", func(t *testing.T) { + topLevelPkgName := "network" + subPkg1 := "network/p2p/node" + subPkg1Runner := "buildjet-4vcpu-ubuntu-2204" + configFile := fmt.Sprintf(` + {"packagesPath": ".", "packages": [{"name": "%s", "exclude": ["network/alsp"], "subpackages": [{"name": "%s", "runner": "%s"}]}]}`, + topLevelPkgName, subPkg1, subPkg1Runner) + allPackges := goPackageFixture( + "network", + "network/alsp", + "network/cache", + "network/channels", + "network/p2p/node", + "network/p2p/node/internal", + "module", + "module/chunks/chunky", + "crypto/hash", + "crypto/random", + "crypto/hash/ecc", + "model/bootstrap", + "model/bootstrap/info", + "model", + ) + cfg := loadPackagesConfig(configFile) + matrices := buildTestMatrices(cfg, func(dir string) []*packages.Package { + return allPackges + }) + require.Len(t, matrices, 2) + for _, matrix := range matrices { + switch matrix.Name { + case topLevelPkgName: + require.Equal(t, defaultCIRunner, matrix.Runner) + require.Equal(t, fmt.Sprintf("%s %s %s ", allPackges[0].PkgPath, allPackges[2].PkgPath, allPackges[3].PkgPath), matrix.Packages) + case subPkg1: + require.Equal(t, subPkg1Runner, matrix.Runner) + require.Equal(t, fmt.Sprintf("%s %s ", allPackges[4].PkgPath, allPackges[5].PkgPath), matrix.Packages) + default: + require.Fail(t, fmt.Sprintf("unexpected matrix name: %s", matrix.Name)) + } + } + }) +} + +func goPackageFixture(pkgs ...string) []*packages.Package { + goPkgs := make([]*packages.Package, len(pkgs)) + for i, pkg := range pkgs { + goPkgs[i] = &packages.Package{PkgPath: fullGoPackagePath(pkg)} + } + return goPkgs +} From 425c8016897b00cd0034e8bb9651e2ad5c702a98 Mon Sep 17 00:00:00 2001 From: Khalil Claybon Date: Mon, 4 Mar 2024 11:07:42 -0500 Subject: [PATCH 07/23] create insure package dynamic test matrix - add separate job for insecure package test matrix --- .github/workflows/ci.yml | 63 ++++++++++++++++++++++++++++++++++++---- 1 file changed, 58 insertions(+), 5 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 0f4415a1283..ab9a4688526 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -91,6 +91,24 @@ jobs: id: set-test-matrix run: go run tools/test_matrix_generator/matrix.go + create-insecure-dynamic-test-matrix: + name: Create Dynamic Unit Test Insecure Package Matrix + runs-on: ubuntu-latest + outputs: + dynamic-matrix: ${{ steps.set-test-matrix.outputs.dynamicMatrix }} + steps: + - name: Checkout repo + uses: actions/checkout@v3 + - name: Setup Go + uses: actions/setup-go@v4 + timeout-minutes: 10 # fail fast. sometimes this step takes an extremely long time + with: + go-version: ${{ env.GO_VERSION }} + cache: true + - name: Set Test Matrix + id: set-test-matrix + run: go run tools/test_matrix_generator/matrix.go -c insecure + unit-test: name: Unit Tests (${{ matrix.targets.name }}) needs: create-dynamic-test-matrix @@ -127,17 +145,52 @@ jobs: flags: unittests name: codecov-umbrella + unit-test-module-insecure: + name: Unit Tests (Insecure) + strategy: + fail-fast: false + matrix: + targets: ${{ fromJSON(needs.create-insecure-dynamic-test-matrix.outputs.dynamic-matrix)}} + runs-on: ${{ matrix.runner }} + steps: + - name: Checkout repo + uses: actions/checkout@v3 + - name: Setup Go + uses: actions/setup-go@v4 + timeout-minutes: 10 # fail fast. sometimes this step takes an extremely long time + with: + go-version: ${{ env.GO_VERSION }} + cache: true + - name: Setup tests (${{ matrix.name }}) + run: make ${{ matrix.setup }} + - name: Run tests (${{ matrix.name }}) + # TODO(rbtz): re-enable when we fix exisiting races. + #env: + # RACE_DETECTOR: 1 + uses: nick-fields/retry@v2 + with: + timeout_minutes: 35 + max_attempts: ${{ matrix.retries }} + # run test target inside each module's root + command: VERBOSE=1 make -C ${{ matrix.name }} test + - name: Upload coverage report + uses: codecov/codecov-action@v3 + with: + file: ./coverage.txt + flags: unittests + name: codecov-umbrella + unit-test-modules: name: Unit Tests (Modules) strategy: fail-fast: false matrix: include: - - name: insecure - setup: install-tools - retries: 5 - race: 0 - runner: buildjet-4vcpu-ubuntu-2204 +# - name: insecure +# setup: install-tools +# retries: 5 +# race: 0 +# runner: buildjet-4vcpu-ubuntu-2204 - name: integration setup: install-tools retries: 5 From 9f54fd830ce9426592446c1c3250c5ba493bfc28 Mon Sep 17 00:00:00 2001 From: Khalil Claybon Date: Mon, 4 Mar 2024 11:46:19 -0500 Subject: [PATCH 08/23] Update ci.yml --- .github/workflows/ci.yml | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ab9a4688526..aba674e5a35 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -145,13 +145,15 @@ jobs: flags: unittests name: codecov-umbrella - unit-test-module-insecure: - name: Unit Tests (Insecure) + unit-test-insecure: + name: Unit Tests (${{ matrix.targets.name }}) + needs: create-dynamic-test-matrix strategy: fail-fast: false matrix: targets: ${{ fromJSON(needs.create-insecure-dynamic-test-matrix.outputs.dynamic-matrix)}} - runs-on: ${{ matrix.runner }} + ## need to set image explicitly due to GitHub logging issue as described in https://github.com/onflow/flow-go/pull/3087#issuecomment-1234383202 + runs-on: ${{ matrix.targets.runner }} steps: - name: Checkout repo uses: actions/checkout@v3 @@ -161,18 +163,17 @@ jobs: with: go-version: ${{ env.GO_VERSION }} cache: true - - name: Setup tests (${{ matrix.name }}) - run: make ${{ matrix.setup }} - - name: Run tests (${{ matrix.name }}) - # TODO(rbtz): re-enable when we fix exisiting races. - #env: - # RACE_DETECTOR: 1 + - name: Setup tests (${{ matrix.targets.name }}) + run: VERBOSE=1 make -e GO_TEST_PACKAGES="${{ matrix.targets.packages }}" install-tools + - name: Run tests (${{ matrix.targets.name }}) uses: nick-fields/retry@v2 with: timeout_minutes: 35 - max_attempts: ${{ matrix.retries }} - # run test target inside each module's root - command: VERBOSE=1 make -C ${{ matrix.name }} test + max_attempts: 5 + command: VERBOSE=1 make -e GO_TEST_PACKAGES="${{ matrix.targets.packages }}" test + # TODO(rbtz): re-enable when we fix exisiting races. + #env: + # RACE_DETECTOR: 1 - name: Upload coverage report uses: codecov/codecov-action@v3 with: From 248075c94ac6762f5499550acf433cd9b7b66bd5 Mon Sep 17 00:00:00 2001 From: Khalil Claybon Date: Mon, 4 Mar 2024 11:47:37 -0500 Subject: [PATCH 09/23] Update ci.yml --- .github/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index aba674e5a35..713b7d7806c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -146,7 +146,7 @@ jobs: name: codecov-umbrella unit-test-insecure: - name: Unit Tests (${{ matrix.targets.name }}) + name: Unit Tests Insecure (${{ matrix.targets.name }}) needs: create-dynamic-test-matrix strategy: fail-fast: false From 6b9abd14c9f185f7d1e80e78bc0c6f3af6d085ed Mon Sep 17 00:00:00 2001 From: Khalil Claybon Date: Mon, 4 Mar 2024 12:01:11 -0500 Subject: [PATCH 10/23] use ubuntu 20.04 instead of latest for default ci build --- .github/workflows/ci.yml | 2 +- tools/test_matrix_generator/default-test-matrix-config.json | 2 +- tools/test_matrix_generator/matrix.go | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 713b7d7806c..055bcd9dab3 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -147,7 +147,7 @@ jobs: unit-test-insecure: name: Unit Tests Insecure (${{ matrix.targets.name }}) - needs: create-dynamic-test-matrix + needs: create-insecure-dynamic-test-matrix strategy: fail-fast: false matrix: diff --git a/tools/test_matrix_generator/default-test-matrix-config.json b/tools/test_matrix_generator/default-test-matrix-config.json index 37fd79ead20..824267a5d05 100644 --- a/tools/test_matrix_generator/default-test-matrix-config.json +++ b/tools/test_matrix_generator/default-test-matrix-config.json @@ -27,7 +27,7 @@ {"name": "network/p2p"}, {"name": "network/test/cohort1", "runner": "buildjet-16vcpu-ubuntu-2204"}, {"name": "network/test/cohort2", "runner": "buildjet-4vcpu-ubuntu-2204"}, - {"name": "network/p2p/node", "runner": "buildjet-4vcpu-ubuntu-2204"} + {"name": "network/p2p/node", "runner": "buildjet-4vcpu-ubuntu-2004"} ]} ] } diff --git a/tools/test_matrix_generator/matrix.go b/tools/test_matrix_generator/matrix.go index fd0915f622d..114b3f586e8 100644 --- a/tools/test_matrix_generator/matrix.go +++ b/tools/test_matrix_generator/matrix.go @@ -76,7 +76,7 @@ func newTestMatrix(name, runner string) *testMatrix { const flowPackagePrefix = "github.com/onflow/flow-go/" const ciMatrixName = "dynamicMatrix" -const defaultCIRunner = "ubuntu-latest" +const defaultCIRunner = "ubuntu-20.04" // Generates a list of packages to test that will be passed to GitHub Actions func main() { From 9cde6e71ce982668922fd5cf35c53df2882efe68 Mon Sep 17 00:00:00 2001 From: Khalil Claybon Date: Mon, 4 Mar 2024 12:26:03 -0500 Subject: [PATCH 11/23] use ubuntu 20.04 for all runners, fix insecure run test script in ci yml --- .github/workflows/ci.yml | 2 +- .../default-test-matrix-config.json | 10 +++++----- .../insecure-module-test-matrix-config.json | 6 +++--- .../integration-module-test-matrix-config.json | 2 +- 4 files changed, 10 insertions(+), 10 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 055bcd9dab3..a08ba5708f4 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -170,7 +170,7 @@ jobs: with: timeout_minutes: 35 max_attempts: 5 - command: VERBOSE=1 make -e GO_TEST_PACKAGES="${{ matrix.targets.packages }}" test + command: VERBOSE=1 make -C ${{ matrix.name }} -e GO_TEST_PACKAGES="${{ matrix.targets.packages }}" test # TODO(rbtz): re-enable when we fix exisiting races. #env: # RACE_DETECTOR: 1 diff --git a/tools/test_matrix_generator/default-test-matrix-config.json b/tools/test_matrix_generator/default-test-matrix-config.json index 824267a5d05..dc981c0bd75 100644 --- a/tools/test_matrix_generator/default-test-matrix-config.json +++ b/tools/test_matrix_generator/default-test-matrix-config.json @@ -9,7 +9,7 @@ {"name": "state"}, {"name": "storage"}, {"name": "utils"}, - {"name": "engine", "runner": "buildjet-4vcpu-ubuntu-2204" ,"subpackages": [ + {"name": "engine", "runner": "buildjet-4vcpu-ubuntu-2004" ,"subpackages": [ {"name": "engine/access"}, {"name": "engine/collection"}, {"name": "engine/common"}, @@ -17,16 +17,16 @@ {"name": "engine/execution/computation"}, {"name": "engine/execution"}, {"name": "engine/verification"}, - {"name": "engine/execution/ingestion", "runner": "buildjet-8vcpu-ubuntu-2204"} + {"name": "engine/execution/ingestion", "runner": "buildjet-8vcpu-ubuntu-2004"} ]}, {"name": "module", "runner": "buildjet-4vcpu-ubuntu-2204" ,"subpackages": [{"name": "module/dkg"}]}, {"name": "network", "subpackages": [ {"name": "network/alsp"}, {"name": "network/p2p/connection"}, {"name": "network/p2p/scoring"}, - {"name": "network/p2p"}, - {"name": "network/test/cohort1", "runner": "buildjet-16vcpu-ubuntu-2204"}, - {"name": "network/test/cohort2", "runner": "buildjet-4vcpu-ubuntu-2204"}, + {"name": "network/p2p", "runner": "buildjet-16vcpu-ubuntu-2004"}, + {"name": "network/test/cohort1", "runner": "buildjet-16vcpu-ubuntu-2004"}, + {"name": "network/test/cohort2", "runner": "buildjet-4vcpu-ubuntu-2004"}, {"name": "network/p2p/node", "runner": "buildjet-4vcpu-ubuntu-2004"} ]} ] diff --git a/tools/test_matrix_generator/insecure-module-test-matrix-config.json b/tools/test_matrix_generator/insecure-module-test-matrix-config.json index 0b63293a31f..59e7aa6ecb0 100644 --- a/tools/test_matrix_generator/insecure-module-test-matrix-config.json +++ b/tools/test_matrix_generator/insecure-module-test-matrix-config.json @@ -2,9 +2,9 @@ "packagesPath": "./insecure", "includeOthers": false, "packages": [ - {"name": "insecure", "runner": "buildjet-4vcpu-ubuntu-2204" ,"subpackages": [ - {"name": "insecure/integration/functional/test/gossipsub/rpc_inspector", "runner": "buildjet-8vcpu-ubuntu-2204"}, - {"name": "insecure/integration/functional/test/gossipsub/scoring", "runner": "buildjet-8vcpu-ubuntu-2204"} + {"name": "insecure", "runner": "buildjet-4vcpu-ubuntu-2004" ,"subpackages": [ + {"name": "insecure/integration/functional/test/gossipsub/rpc_inspector", "runner": "buildjet-8vcpu-ubuntu-2004"}, + {"name": "insecure/integration/functional/test/gossipsub/scoring", "runner": "buildjet-8vcpu-ubuntu-2004"} ]} ] } diff --git a/tools/test_matrix_generator/integration-module-test-matrix-config.json b/tools/test_matrix_generator/integration-module-test-matrix-config.json index af53cfa1ec8..379ee6ab64e 100644 --- a/tools/test_matrix_generator/integration-module-test-matrix-config.json +++ b/tools/test_matrix_generator/integration-module-test-matrix-config.json @@ -3,7 +3,7 @@ "includeOthers": false, "packages": [{ "name": "integration", - "runner": "buildjet-4vcpu-ubuntu-2204", + "runner": "buildjet-4vcpu-ubuntu-2004", "exclude": ["integration/tests"] }] } From 998cda4da35eb8960316cd5c2aa55052c2306143 Mon Sep 17 00:00:00 2001 From: Khalil Claybon Date: Mon, 4 Mar 2024 12:52:15 -0500 Subject: [PATCH 12/23] Update ci.yml --- .github/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a08ba5708f4..3ea05ed223f 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -170,7 +170,7 @@ jobs: with: timeout_minutes: 35 max_attempts: 5 - command: VERBOSE=1 make -C ${{ matrix.name }} -e GO_TEST_PACKAGES="${{ matrix.targets.packages }}" test + command: VERBOSE=1 make -C ./insecure -e GO_TEST_PACKAGES="${{ matrix.targets.packages }}" test # TODO(rbtz): re-enable when we fix exisiting races. #env: # RACE_DETECTOR: 1 From 0ef8d4be4d38c3d40b9703eb652cbb946d1436f2 Mon Sep 17 00:00:00 2001 From: Khalil Claybon Date: Mon, 4 Mar 2024 13:21:04 -0500 Subject: [PATCH 13/23] update makefile command to run insecure tests with go package list --- insecure/Makefile | 6 +++++- .../functional/test/gossipsub/scoring/scoring_test.go | 1 - 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/insecure/Makefile b/insecure/Makefile index f38a03381b3..f731c036dc2 100644 --- a/insecure/Makefile +++ b/insecure/Makefile @@ -1,6 +1,10 @@ # Name of the cover profile COVER_PROFILE := cover.out +# By default, this will run all tests in all packages, but we have a way to override this in CI so that we can +# dynamically split up CI jobs into smaller jobs that can be run in parallel +GO_TEST_PACKAGES := ./... + # allows CI to specify whether to have race detection on / off ifeq ($(RACE_DETECTOR),1) RACE_FLAG := -race @@ -15,7 +19,7 @@ CGO_FLAG := CGO_CFLAGS=$(CRYPTO_FLAG) # runs all unit tests of the insecure module .PHONY: test test: - $(CGO_FLAG) go test $(if $(VERBOSE),-v,) -coverprofile=$(COVER_PROFILE) $(RACE_FLAG) $(if $(JSON_OUTPUT),-json,) $(if $(NUM_RUNS),-count $(NUM_RUNS),) ./... + $(CGO_FLAG) go test $(if $(VERBOSE),-v,) -coverprofile=$(COVER_PROFILE) $(RACE_FLAG) $(if $(JSON_OUTPUT),-json,) $(if $(NUM_RUNS),-count $(NUM_RUNS),) $(GO_TEST_PACKAGES) .PHONY: lint lint: tidy diff --git a/insecure/integration/functional/test/gossipsub/scoring/scoring_test.go b/insecure/integration/functional/test/gossipsub/scoring/scoring_test.go index b1533356280..2c287f20034 100644 --- a/insecure/integration/functional/test/gossipsub/scoring/scoring_test.go +++ b/insecure/integration/functional/test/gossipsub/scoring/scoring_test.go @@ -35,7 +35,6 @@ func TestGossipSubInvalidMessageDelivery_Integration(t *testing.T) { spamMsgFactory func(spammerId peer.ID, victimId peer.ID, topic channels.Topic) *pubsub_pb.Message }{ { - name: "unknown peer, invalid signature", spamMsgFactory: func(spammerId peer.ID, _ peer.ID, topic channels.Topic) *pubsub_pb.Message { return p2ptest.PubsubMessageFixture(t, p2ptest.WithTopic(topic.String())) From cd8c5b9f33c5fca02fc4fb74b8e39b81debbdbef Mon Sep 17 00:00:00 2001 From: Khalil Claybon Date: Mon, 4 Mar 2024 13:38:49 -0500 Subject: [PATCH 14/23] re-enable flaky tests --- .../functional/test/gossipsub/scoring/scoring_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/insecure/integration/functional/test/gossipsub/scoring/scoring_test.go b/insecure/integration/functional/test/gossipsub/scoring/scoring_test.go index 2c287f20034..9cb04baad01 100644 --- a/insecure/integration/functional/test/gossipsub/scoring/scoring_test.go +++ b/insecure/integration/functional/test/gossipsub/scoring/scoring_test.go @@ -29,7 +29,6 @@ import ( // a spammer peer, the victim will eventually penalize the spammer and stop receiving messages from them. // Note: the term integration is used here because it requires integrating all components of the libp2p stack. func TestGossipSubInvalidMessageDelivery_Integration(t *testing.T) { - unittest.SkipUnless(t, unittest.TEST_FLAKY, "https://github.com/dapperlabs/flow-go/issues/6949") tt := []struct { name string spamMsgFactory func(spammerId peer.ID, victimId peer.ID, topic channels.Topic) *pubsub_pb.Message From b647f94e926f073b655362d11470ddf08b40521c Mon Sep 17 00:00:00 2001 From: Khalil Claybon Date: Mon, 4 Mar 2024 14:09:57 -0500 Subject: [PATCH 15/23] add integration test others job that runs all other integration tests not run in the integration-test job --- .github/workflows/ci.yml | 59 ++++++++++++++++++++++++++++++++++++---- 1 file changed, 54 insertions(+), 5 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 3ea05ed223f..7dace5d2f52 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -109,6 +109,24 @@ jobs: id: set-test-matrix run: go run tools/test_matrix_generator/matrix.go -c insecure + create-integration-dynamic-test-matrix: + name: Create Dynamic Integration Test Package Matrix + runs-on: ubuntu-latest + outputs: + dynamic-matrix: ${{ steps.set-test-matrix.outputs.dynamicMatrix }} + steps: + - name: Checkout repo + uses: actions/checkout@v3 + - name: Setup Go + uses: actions/setup-go@v4 + timeout-minutes: 10 # fail fast. sometimes this step takes an extremely long time + with: + go-version: ${{ env.GO_VERSION }} + cache: true + - name: Set Test Matrix + id: set-test-matrix + run: go run tools/test_matrix_generator/matrix.go -c integration + unit-test: name: Unit Tests (${{ matrix.targets.name }}) needs: create-dynamic-test-matrix @@ -187,11 +205,6 @@ jobs: fail-fast: false matrix: include: -# - name: insecure -# setup: install-tools -# retries: 5 -# race: 0 -# runner: buildjet-4vcpu-ubuntu-2204 - name: integration setup: install-tools retries: 5 @@ -266,6 +279,42 @@ jobs: # use the workflow run id as part of the cache key to ensure these docker images will only be used for a single workflow run key: flow-docker-images-${{ hashFiles('**/Dockerfile') }}-${{ github.run_id }} + integration-test-others: + name: Integration Test Others (${{ matrix.targets.name }}) + needs: create-integration-dynamic-test-matrix + strategy: + fail-fast: false + matrix: + targets: ${{ fromJSON(needs.create-integration-dynamic-test-matrix.outputs.dynamic-matrix)}} + ## need to set image explicitly due to GitHub logging issue as described in https://github.com/onflow/flow-go/pull/3087#issuecomment-1234383202 + runs-on: ${{ matrix.targets.runner }} + steps: + - name: Checkout repo + uses: actions/checkout@v3 + - name: Setup Go + uses: actions/setup-go@v4 + timeout-minutes: 10 # fail fast. sometimes this step takes an extremely long time + with: + go-version: ${{ env.GO_VERSION }} + cache: true + - name: Setup tests (${{ matrix.targets.name }}) + run: VERBOSE=1 make -e GO_TEST_PACKAGES="${{ matrix.targets.packages }}" install-tools + - name: Run tests (${{ matrix.targets.name }}) + uses: nick-fields/retry@v2 + with: + timeout_minutes: 35 + max_attempts: 5 + command: VERBOSE=1 make -C ./integration -e GO_TEST_PACKAGES="${{ matrix.targets.packages }}" test + # TODO(rbtz): re-enable when we fix exisiting races. + #env: + # RACE_DETECTOR: 1 + - name: Upload coverage report + uses: codecov/codecov-action@v3 + with: + file: ./coverage.txt + flags: unittests + name: codecov-umbrella + integration-test: name: Integration Tests needs: docker-build From bff893894f9ee55c6a4cd494aa34abcdefdcd0b5 Mon Sep 17 00:00:00 2001 From: Khalil Claybon Date: Mon, 4 Mar 2024 14:52:59 -0500 Subject: [PATCH 16/23] remove debug logs - remove unused ci job --- .github/workflows/ci.yml | 41 +------------------ .../validation_inspector_test.go | 8 +--- .../test/gossipsub/scoring/scoring_test.go | 7 +--- .../control_message_validation_inspector.go | 1 - .../default-test-matrix-config.json | 2 +- tools/test_matrix_generator/matrix.go | 11 ++--- 6 files changed, 10 insertions(+), 60 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 7dace5d2f52..b14ba2d69c5 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -199,45 +199,6 @@ jobs: flags: unittests name: codecov-umbrella - unit-test-modules: - name: Unit Tests (Modules) - strategy: - fail-fast: false - matrix: - include: - - name: integration - setup: install-tools - retries: 5 - race: 0 - runner: buildjet-4vcpu-ubuntu-2204 - runs-on: ${{ matrix.runner }} - steps: - - name: Checkout repo - uses: actions/checkout@v3 - - name: Setup Go - uses: actions/setup-go@v4 - timeout-minutes: 10 # fail fast. sometimes this step takes an extremely long time - with: - go-version: ${{ env.GO_VERSION }} - cache: true - - name: Setup tests (${{ matrix.name }}) - run: make ${{ matrix.setup }} - - name: Run tests (${{ matrix.name }}) - env: - RACE_DETECTOR: ${{ matrix.race }} - uses: nick-fields/retry@v2 - with: - timeout_minutes: 35 - max_attempts: ${{ matrix.retries }} - # run test target inside each module's root - command: VERBOSE=1 make -C ${{ matrix.name }} test - - name: Upload coverage report - uses: codecov/codecov-action@v3 - with: - file: ./coverage.txt - flags: unittests - name: codecov-umbrella - docker-build: name: Docker Build runs-on: buildjet-16vcpu-ubuntu-2204 @@ -280,7 +241,7 @@ jobs: key: flow-docker-images-${{ hashFiles('**/Dockerfile') }}-${{ github.run_id }} integration-test-others: - name: Integration Test Others (${{ matrix.targets.name }}) + name: Integration Tests Others (${{ matrix.targets.name }}) needs: create-integration-dynamic-test-matrix strategy: fail-fast: false diff --git a/insecure/integration/functional/test/gossipsub/rpc_inspector/validation_inspector_test.go b/insecure/integration/functional/test/gossipsub/rpc_inspector/validation_inspector_test.go index 953504c47b9..1c43a9999e7 100644 --- a/insecure/integration/functional/test/gossipsub/rpc_inspector/validation_inspector_test.go +++ b/insecure/integration/functional/test/gossipsub/rpc_inspector/validation_inspector_test.go @@ -4,7 +4,6 @@ import ( "context" "fmt" "math" - "os" "testing" "time" @@ -12,7 +11,6 @@ import ( pb "github.com/libp2p/go-libp2p-pubsub/pb" pubsub_pb "github.com/libp2p/go-libp2p-pubsub/pb" "github.com/libp2p/go-libp2p/core/peer" - "github.com/rs/zerolog" mockery "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" corrupt "github.com/yhassanzadeh13/go-libp2p-pubsub" @@ -981,8 +979,6 @@ func TestGossipSubSpamMitigationIntegration_IHaves(t *testing.T) { // The victim node is configured to use the GossipSubInspector to detect spam and the scoring system to mitigate spam. // The test ensures that the victim node is disconnected from the spammer node on the GossipSub mesh after the spam detection is triggered. func testGossipSubSpamMitigationIntegration(t *testing.T, msgType p2pmsg.ControlMessageType) { - logger := zerolog.New(os.Stdout).Level(zerolog.TraceLevel) - logger.Trace().Msg(fmt.Sprintf("STARTING GOSSIPSUB SPAM MITIGATION: %s", msgType.String())) idProvider := mock.NewIdentityProvider(t) sporkID := unittest.IdentifierFixture() spammer := corruptlibp2p.NewGossipSubRouterSpammer(t, sporkID, flow.RoleConsensus, idProvider) @@ -1001,8 +997,7 @@ func testGossipSubSpamMitigationIntegration(t *testing.T, msgType p2pmsg.Control t.Name(), idProvider, p2ptest.WithRole(flow.RoleConsensus), - p2ptest.OverrideFlowConfig(cfg), - p2ptest.WithLogger(logger)) + p2ptest.OverrideFlowConfig(cfg)) ids := flow.IdentityList{&victimId, &spammer.SpammerId} idProvider.On("ByPeerID", mockery.Anything).Return(func(peerId peer.ID) *flow.Identity { @@ -1108,5 +1103,4 @@ func testGossipSubSpamMitigationIntegration(t *testing.T, msgType p2pmsg.Control func() interface{} { return unittest.ProposalFixture() }) - logger.Trace().Msg(fmt.Sprintf("FINISHED GOSSIPSUB SPAM MITIGATION: %s", msgType.String())) } diff --git a/insecure/integration/functional/test/gossipsub/scoring/scoring_test.go b/insecure/integration/functional/test/gossipsub/scoring/scoring_test.go index 9cb04baad01..184f365f60c 100644 --- a/insecure/integration/functional/test/gossipsub/scoring/scoring_test.go +++ b/insecure/integration/functional/test/gossipsub/scoring/scoring_test.go @@ -2,14 +2,12 @@ package scoring import ( "context" - "os" "testing" "time" pubsub "github.com/libp2p/go-libp2p-pubsub" pubsub_pb "github.com/libp2p/go-libp2p-pubsub/pb" "github.com/libp2p/go-libp2p/core/peer" - "github.com/rs/zerolog" "github.com/stretchr/testify/require" "github.com/onflow/flow-go/config" @@ -96,8 +94,6 @@ func TestGossipSubInvalidMessageDelivery_Integration(t *testing.T) { // - t: the test instance. // - spamMsgFactory: a function that creates unique invalid messages to spam the victim with. func testGossipSubInvalidMessageDeliveryScoring(t *testing.T, spamMsgFactory func(peer.ID, peer.ID, channels.Topic) *pubsub_pb.Message) { - logger := zerolog.New(os.Stdout).Level(zerolog.TraceLevel) - role := flow.RoleConsensus sporkId := unittest.IdentifierFixture() blockTopic := channels.TopicFromChannel(channels.PushBlocks, sporkId) @@ -119,8 +115,7 @@ func testGossipSubInvalidMessageDeliveryScoring(t *testing.T, spamMsgFactory fun t.Name(), idProvider, p2ptest.WithRole(role), - p2ptest.OverrideFlowConfig(cfg), - p2ptest.WithLogger(logger)) + p2ptest.OverrideFlowConfig(cfg)) ids := flow.IdentityList{&spammer.SpammerId, &victimIdentity} idProvider.SetIdentities(ids) diff --git a/network/p2p/inspector/validation/control_message_validation_inspector.go b/network/p2p/inspector/validation/control_message_validation_inspector.go index 15ce2344a5f..ffdabe38ce9 100644 --- a/network/p2p/inspector/validation/control_message_validation_inspector.go +++ b/network/p2p/inspector/validation/control_message_validation_inspector.go @@ -1098,7 +1098,6 @@ func (c *ControlMsgValidationInspector) logAndDistributeAsyncInspectErrs(req *In default: c.notificationConsumer.OnInvalidControlMessageNotification(p2p.NewInvalidControlMessageNotification(req.Peer, ctlMsgType, err, count, topicType)) lg.Error().Msg("rpc control message async inspection failed, notification sent") - lg.Trace().Msg("rpc control message async inspection failed, notification sent") c.metrics.OnInvalidControlMessageNotificationSent() } } diff --git a/tools/test_matrix_generator/default-test-matrix-config.json b/tools/test_matrix_generator/default-test-matrix-config.json index dc981c0bd75..a62ac6aec5a 100644 --- a/tools/test_matrix_generator/default-test-matrix-config.json +++ b/tools/test_matrix_generator/default-test-matrix-config.json @@ -19,7 +19,7 @@ {"name": "engine/verification"}, {"name": "engine/execution/ingestion", "runner": "buildjet-8vcpu-ubuntu-2004"} ]}, - {"name": "module", "runner": "buildjet-4vcpu-ubuntu-2204" ,"subpackages": [{"name": "module/dkg"}]}, + {"name": "module", "runner": "buildjet-4vcpu-ubuntu-2004" ,"subpackages": [{"name": "module/dkg"}]}, {"name": "network", "subpackages": [ {"name": "network/alsp"}, {"name": "network/p2p/connection"}, diff --git a/tools/test_matrix_generator/matrix.go b/tools/test_matrix_generator/matrix.go index 114b3f586e8..f63999b378a 100644 --- a/tools/test_matrix_generator/matrix.go +++ b/tools/test_matrix_generator/matrix.go @@ -24,6 +24,12 @@ var ( matrixConfigFile string ) +const ( + flowPackagePrefix = "github.com/onflow/flow-go/" + ciMatrixName = "dynamicMatrix" + defaultCIRunner = "ubuntu-20.04" +) + // flowGoPackage configuration for a package to be tested. type flowGoPackage struct { // Name the name of the package where test are located. @@ -74,13 +80,8 @@ func newTestMatrix(name, runner string) *testMatrix { return t } -const flowPackagePrefix = "github.com/onflow/flow-go/" -const ciMatrixName = "dynamicMatrix" -const defaultCIRunner = "ubuntu-20.04" - // Generates a list of packages to test that will be passed to GitHub Actions func main() { - // Parse command-line arguments pflag.Parse() var configFile string From 153695937abd52f989d70be1355be72fd1ad795d Mon Sep 17 00:00:00 2001 From: Khalil Claybon Date: Mon, 4 Mar 2024 14:55:59 -0500 Subject: [PATCH 17/23] use buildjet/setup-go action --- .github/workflows/ci.yml | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index b14ba2d69c5..69702c0d55b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -37,7 +37,7 @@ jobs: - name: Checkout repo uses: actions/checkout@v3 - name: Setup Go - uses: actions/setup-go@v4 + uses: buildjet/setup-go@v4 timeout-minutes: 10 # fail fast. sometimes this step takes an extremely long time with: go-version: ${{ env.GO_VERSION }} @@ -63,7 +63,7 @@ jobs: - name: Checkout repo uses: actions/checkout@v3 - name: Setup Go - uses: actions/setup-go@v4 + uses: buildjet/setup-go@v4 timeout-minutes: 10 # fail fast. sometimes this step takes an extremely long time with: go-version: ${{ env.GO_VERSION }} @@ -82,7 +82,7 @@ jobs: - name: Checkout repo uses: actions/checkout@v3 - name: Setup Go - uses: actions/setup-go@v4 + uses: buildjet/setup-go@v4 timeout-minutes: 10 # fail fast. sometimes this step takes an extremely long time with: go-version: ${{ env.GO_VERSION }} @@ -100,7 +100,7 @@ jobs: - name: Checkout repo uses: actions/checkout@v3 - name: Setup Go - uses: actions/setup-go@v4 + uses: buildjet/setup-go@v4 timeout-minutes: 10 # fail fast. sometimes this step takes an extremely long time with: go-version: ${{ env.GO_VERSION }} @@ -118,7 +118,7 @@ jobs: - name: Checkout repo uses: actions/checkout@v3 - name: Setup Go - uses: actions/setup-go@v4 + uses: buildjet/setup-go@v4 timeout-minutes: 10 # fail fast. sometimes this step takes an extremely long time with: go-version: ${{ env.GO_VERSION }} @@ -140,7 +140,7 @@ jobs: - name: Checkout repo uses: actions/checkout@v3 - name: Setup Go - uses: actions/setup-go@v4 + uses: buildjet/setup-go@v4 timeout-minutes: 10 # fail fast. sometimes this step takes an extremely long time with: go-version: ${{ env.GO_VERSION }} @@ -176,7 +176,7 @@ jobs: - name: Checkout repo uses: actions/checkout@v3 - name: Setup Go - uses: actions/setup-go@v4 + uses: buildjet/setup-go@v4 timeout-minutes: 10 # fail fast. sometimes this step takes an extremely long time with: go-version: ${{ env.GO_VERSION }} @@ -211,7 +211,7 @@ jobs: # all tags are needed for integration tests fetch-depth: 0 - name: Setup Go - uses: actions/setup-go@v4 + uses: buildjet/setup-go@v4 timeout-minutes: 10 # fail fast. sometimes this step takes an extremely long time with: go-version: ${{ env.GO_VERSION }} @@ -253,7 +253,7 @@ jobs: - name: Checkout repo uses: actions/checkout@v3 - name: Setup Go - uses: actions/setup-go@v4 + uses: buildjet/setup-go@v4 timeout-minutes: 10 # fail fast. sometimes this step takes an extremely long time with: go-version: ${{ env.GO_VERSION }} @@ -340,7 +340,7 @@ jobs: # all tags are needed for integration tests fetch-depth: 0 - name: Setup Go - uses: actions/setup-go@v4 + uses: buildjet/setup-go@v4 timeout-minutes: 10 # fail fast. sometimes this step takes an extremely long time with: go-version: ${{ env.GO_VERSION }} From b99c6fd9149189e49ed979af782df3cb39714845 Mon Sep 17 00:00:00 2001 From: Khalil Claybon Date: Mon, 4 Mar 2024 17:10:56 -0500 Subject: [PATCH 18/23] remove old matrix generation script --- utils/test_matrix/test_matrix.go | 159 ----------------- utils/test_matrix/test_matrix_test.go | 245 -------------------------- 2 files changed, 404 deletions(-) delete mode 100644 utils/test_matrix/test_matrix.go delete mode 100644 utils/test_matrix/test_matrix_test.go diff --git a/utils/test_matrix/test_matrix.go b/utils/test_matrix/test_matrix.go deleted file mode 100644 index 9a5cb9f9713..00000000000 --- a/utils/test_matrix/test_matrix.go +++ /dev/null @@ -1,159 +0,0 @@ -package main - -import ( - "encoding/json" - "fmt" - "os" - "strings" - - "golang.org/x/tools/go/packages" -) - -const flowPackagePrefix = "github.com/onflow/flow-go/" -const ciMatrixName = "dynamicMatrix" -const ciDefaultRunner = "ubuntu-latest" - -// testMatrix represents a single GitHub Actions test matrix combination that consists of a name and a list of flow-go packages associated with that name. -type testMatrix struct { - Name string `json:"name"` - Packages string `json:"packages"` - Runner string `json:"runner"` -} - -type targets struct { - runners map[string]string - packages map[string][]string -} - -// Generates a list of packages to test that will be passed to GitHub Actions -func main() { - if len(os.Args) == 1 { - fmt.Fprintln(os.Stderr, "must have at least 1 package listed") - return - } - - allFlowPackages := listAllFlowPackages() - - targetPackages, seenPackages := listTargetPackages(os.Args[1:], allFlowPackages) - - otherPackages := listOtherPackages(allFlowPackages, seenPackages) - - testMatrix := generateTestMatrix(targetPackages, otherPackages) - - // generate JSON output that will be read in by CI matrix - // can't use json.MarshalIndent because fromJSON() in CI can’t read JSON with any spaces - testMatrixBytes, err := json.Marshal(testMatrix) - if err != nil { - panic(err) - } - - // this string will be read by CI to generate groups of tests to run in separate CI jobs - testMatrixStr := "::set-output name=" + ciMatrixName + "::" + string(testMatrixBytes) - - // very important to add newline character at the end of the compacted JSON - otherwise fromJSON() in CI will throw unmarshalling error - fmt.Println(testMatrixStr) -} - -func generateTestMatrix(targetPackages targets, otherPackages []string) []testMatrix { - var testMatrices []testMatrix - - for packageName := range targetPackages.packages { - targetTestMatrix := testMatrix{ - Name: packageName, - Packages: strings.Join(targetPackages.packages[packageName], " "), - Runner: targetPackages.runners[packageName], - } - testMatrices = append(testMatrices, targetTestMatrix) - } - - // add the other packages after all target packages added - otherTestMatrix := testMatrix{ - Name: "others", - Packages: strings.Join(otherPackages, " "), - Runner: ciDefaultRunner, - } - - testMatrices = append(testMatrices, otherTestMatrix) - - return testMatrices -} - -// listTargetPackages returns a map-list of target packages to run as separate CI jobs, based on a list of target package prefixes. -// It also returns a list of the "seen" packages that can then be used to extract the remaining packages to run (in a separate CI job). -func listTargetPackages(targetPackagePrefixes []string, allFlowPackages []string) (targets, map[string]string) { - targetPackages := make(map[string][]string) - targetRunners := make(map[string]string) - - // Stores list of packages already seen / allocated to other lists. Needed for the last package which will - // have all the leftover packages that weren't allocated to a separate list (CI job). - // It's a map, not a list, to make it easier to check if a package was seen or not. - seenPackages := make(map[string]string) - - // iterate over the target packages to run as separate CI jobs - for _, targetPackagePrefix := range targetPackagePrefixes { - var targetPackage []string - - // assume package name specified without runner - targetPackagePrefixNoRunner := targetPackagePrefix - - // check if specify CI runner to use for the package, otherwise use default - colonIndex := strings.Index(targetPackagePrefix, ":") - if colonIndex != -1 { - targetPackagePrefixNoRunner = targetPackagePrefix[:colonIndex] // strip out runner from package name - targetRunners[targetPackagePrefixNoRunner] = targetPackagePrefix[colonIndex+1:] - } else { - // use default CI runner if didn't specify runner - targetRunners[targetPackagePrefix] = ciDefaultRunner - } - - // go through all packages to see which ones to pull out - for _, allPackage := range allFlowPackages { - if strings.HasPrefix(allPackage, flowPackagePrefix+targetPackagePrefixNoRunner) { - // if the package was already seen, don't append it to the list - // this is to support listing sub-sub packages in a CI job without duplicating those sub-sub packages - // in the parent package job - _, seen := seenPackages[allPackage] - if seen { - continue - } - targetPackage = append(targetPackage, allPackage) - seenPackages[allPackage] = allPackage - } - } - if len(targetPackage) == 0 { - panic("no packages exist with prefix " + targetPackagePrefixNoRunner) - } - targetPackages[targetPackagePrefixNoRunner] = targetPackage - } - return targets{targetRunners, targetPackages}, seenPackages -} - -// listOtherPackages compiles the remaining packages that don't match any of the target packages. -func listOtherPackages(allFlowPackages []string, seenPackages map[string]string) []string { - var otherPackages []string - - for _, allFlowPackage := range allFlowPackages { - _, seen := seenPackages[allFlowPackage] - if !seen { - otherPackages = append(otherPackages, allFlowPackage) - } - } - - if len(otherPackages) == 0 { - panic("other packages list can't be 0") - } - return otherPackages -} - -func listAllFlowPackages() []string { - flowPackages, err := packages.Load(&packages.Config{}, "./...") - - if err != nil { - panic(err) - } - var flowPackagesStr []string - for _, p := range flowPackages { - flowPackagesStr = append(flowPackagesStr, p.PkgPath) - } - return flowPackagesStr -} diff --git a/utils/test_matrix/test_matrix_test.go b/utils/test_matrix/test_matrix_test.go deleted file mode 100644 index db5accb6fc2..00000000000 --- a/utils/test_matrix/test_matrix_test.go +++ /dev/null @@ -1,245 +0,0 @@ -package main - -import ( - "testing" - - "github.com/stretchr/testify/require" -) - -// Can't have a const []string so resorting to using a test helper function. -func getAllFlowPackages() []string { - return []string{ - flowPackagePrefix + "abc", - flowPackagePrefix + "abc/123", - flowPackagePrefix + "abc/def", - flowPackagePrefix + "abc/def/ghi", - flowPackagePrefix + "abc/def/ghi/jkl", - flowPackagePrefix + "abc/def/ghi/jkl/mno", - flowPackagePrefix + "abc/def/ghi/jkl/mno/pqr", - flowPackagePrefix + "abc/def/ghi/mno/abc", - flowPackagePrefix + "abc/def/ghi/mno/def", - flowPackagePrefix + "abc/def/ghi/mno/ghi", - flowPackagePrefix + "abc/def/jkl", - flowPackagePrefix + "abc/def/jkl/mno", - flowPackagePrefix + "abc/def/jkl/mno/pqr", - flowPackagePrefix + "def", - flowPackagePrefix + "def/abc", - flowPackagePrefix + "ghi", - flowPackagePrefix + "jkl", - flowPackagePrefix + "mno/abc", - flowPackagePrefix + "pqr", - flowPackagePrefix + "stu", - flowPackagePrefix + "vwx", - flowPackagePrefix + "vwx/ghi", - flowPackagePrefix + "yz", - } -} - -// TestListTargetPackages_DefaultRunners tests that the target packages are included in the target packages and seen packages. -// All packages use default CI runners. -func TestListTargetPackages_DefaultRunners(t *testing.T) { - target, seenPackages := listTargetPackages([]string{"abc", "ghi"}, getAllFlowPackages()) - require.Equal(t, 2, len(target.packages)) - - // check all TARGET - // these are the expected target packages that start with "abc" - require.Equal(t, 13, len(target.packages["abc"])) - require.Contains(t, target.packages["abc"], flowPackagePrefix+"abc") - require.Contains(t, target.packages["abc"], flowPackagePrefix+"abc/123") - require.Contains(t, target.packages["abc"], flowPackagePrefix+"abc/def") - require.Contains(t, target.packages["abc"], flowPackagePrefix+"abc/def/ghi") - require.Contains(t, target.packages["abc"], flowPackagePrefix+"abc/def/ghi/jkl") - require.Contains(t, target.packages["abc"], flowPackagePrefix+"abc/def/ghi/jkl/mno") - require.Contains(t, target.packages["abc"], flowPackagePrefix+"abc/def/ghi/jkl/mno/pqr") - require.Contains(t, target.packages["abc"], flowPackagePrefix+"abc/def/ghi/mno/abc") - require.Contains(t, target.packages["abc"], flowPackagePrefix+"abc/def/ghi/mno/def") - require.Contains(t, target.packages["abc"], flowPackagePrefix+"abc/def/ghi/mno/ghi") - require.Contains(t, target.packages["abc"], flowPackagePrefix+"abc/def/jkl") - require.Contains(t, target.packages["abc"], flowPackagePrefix+"abc/def/jkl/mno") - require.Contains(t, target.packages["abc"], flowPackagePrefix+"abc/def/jkl/mno/pqr") - - // there should be 1 package that starts with "ghi" - require.Equal(t, 1, len(target.packages["ghi"])) - require.Contains(t, target.packages["ghi"], flowPackagePrefix+"ghi") - - // check all CI RUNNERS for each target package - require.Equal(t, 2, len(target.runners)) - require.Equal(t, target.runners["abc"], ciDefaultRunner) - require.Equal(t, target.runners["ghi"], ciDefaultRunner) - - // check all SEEN packages - // these are all expected packages that start with "abc" or "ghi" - require.Equal(t, 14, len(seenPackages)) - require.Contains(t, seenPackages, flowPackagePrefix+"abc") - require.Contains(t, seenPackages, flowPackagePrefix+"abc/123") - require.Contains(t, seenPackages, flowPackagePrefix+"abc/def") - require.Contains(t, seenPackages, flowPackagePrefix+"abc/def/ghi") - require.Contains(t, seenPackages, flowPackagePrefix+"abc/def/ghi/jkl") - require.Contains(t, seenPackages, flowPackagePrefix+"abc/def/ghi/jkl/mno") - require.Contains(t, seenPackages, flowPackagePrefix+"abc/def/ghi/jkl/mno/pqr") - require.Contains(t, seenPackages, flowPackagePrefix+"abc/def/ghi/mno/abc") - require.Contains(t, seenPackages, flowPackagePrefix+"abc/def/ghi/mno/def") - require.Contains(t, seenPackages, flowPackagePrefix+"abc/def/ghi/mno/ghi") - require.Contains(t, seenPackages, flowPackagePrefix+"abc/def/jkl") - require.Contains(t, seenPackages, flowPackagePrefix+"abc/def/jkl/mno") - require.Contains(t, seenPackages, flowPackagePrefix+"abc/def/jkl/mno/pqr") - - require.Contains(t, seenPackages, flowPackagePrefix+"ghi") -} - -// TestListTargetSubPackages_CustomRunners tests that if a subpackage is specified as a target package, then the sub package and -// all children of the sub package are also included in the target packages. -func TestListTargetSubPackages_CustomRunners(t *testing.T) { - target, seenPackages := listTargetPackages([]string{"abc/def:foo_runner"}, getAllFlowPackages()) - require.Equal(t, 1, len(target.packages)) - - // check all TARGET packages - // there should be 2 target subpackages that starts with "abc/def" - require.Equal(t, 11, len(target.packages["abc/def"])) - require.Contains(t, target.packages["abc/def"], flowPackagePrefix+"abc/def") - require.Contains(t, target.packages["abc/def"], flowPackagePrefix+"abc/def/ghi") - require.Contains(t, target.packages["abc/def"], flowPackagePrefix+"abc/def/ghi/jkl") - require.Contains(t, target.packages["abc/def"], flowPackagePrefix+"abc/def/ghi/jkl/mno") - require.Contains(t, target.packages["abc/def"], flowPackagePrefix+"abc/def/ghi/jkl/mno/pqr") - require.Contains(t, target.packages["abc/def"], flowPackagePrefix+"abc/def/ghi/mno/abc") - require.Contains(t, target.packages["abc/def"], flowPackagePrefix+"abc/def/ghi/mno/def") - require.Contains(t, target.packages["abc/def"], flowPackagePrefix+"abc/def/ghi/mno/ghi") - require.Contains(t, target.packages["abc/def"], flowPackagePrefix+"abc/def/jkl") - require.Contains(t, target.packages["abc/def"], flowPackagePrefix+"abc/def/jkl/mno") - require.Contains(t, target.packages["abc/def"], flowPackagePrefix+"abc/def/jkl/mno/pqr") - - // check all CI RUNNERS for each target package - require.Equal(t, 1, len(target.runners)) - require.Equal(t, target.runners["abc/def"], "foo_runner") - - // check all SEEN packages - // there should be 11 seen subpackages that start with "abc/def" - require.Equal(t, 11, len(seenPackages)) - require.Contains(t, seenPackages, flowPackagePrefix+"abc/def") - require.Contains(t, seenPackages, flowPackagePrefix+"abc/def/ghi") - require.Contains(t, seenPackages, flowPackagePrefix+"abc/def/ghi/jkl") - require.Contains(t, seenPackages, flowPackagePrefix+"abc/def/ghi/jkl/mno") - require.Contains(t, seenPackages, flowPackagePrefix+"abc/def/ghi/jkl/mno/pqr") - require.Contains(t, seenPackages, flowPackagePrefix+"abc/def/ghi/mno/abc") - require.Contains(t, seenPackages, flowPackagePrefix+"abc/def/ghi/mno/def") - require.Contains(t, seenPackages, flowPackagePrefix+"abc/def/ghi/mno/ghi") - require.Contains(t, seenPackages, flowPackagePrefix+"abc/def/jkl") - require.Contains(t, seenPackages, flowPackagePrefix+"abc/def/jkl/mno") - require.Contains(t, seenPackages, flowPackagePrefix+"abc/def/jkl/mno/pqr") -} - -// TestListOtherPackages tests that the remaining packages that don't match any of the target packages are included -func TestListOtherPackages(t *testing.T) { - var seenPackages = make(map[string]string) - seenPackages[flowPackagePrefix+"abc/def"] = flowPackagePrefix + "abc/def" - seenPackages[flowPackagePrefix+"abc/def/ghi"] = flowPackagePrefix + "abc/def/ghi" - seenPackages[flowPackagePrefix+"ghi"] = flowPackagePrefix + "ghi" - seenPackages[flowPackagePrefix+"mno/abc"] = flowPackagePrefix + "mno/abc" - seenPackages[flowPackagePrefix+"stu"] = flowPackagePrefix + "stu" - - otherPackages := listOtherPackages(getAllFlowPackages(), seenPackages) - - require.Equal(t, 18, len(otherPackages)) - - require.Contains(t, otherPackages, flowPackagePrefix+"abc") - require.Contains(t, otherPackages, flowPackagePrefix+"abc/123") - require.Contains(t, otherPackages, flowPackagePrefix+"abc/def/ghi/jkl") - require.Contains(t, otherPackages, flowPackagePrefix+"abc/def/ghi/jkl/mno") - require.Contains(t, otherPackages, flowPackagePrefix+"abc/def/ghi/jkl/mno/pqr") - require.Contains(t, otherPackages, flowPackagePrefix+"abc/def/ghi/mno/abc") - require.Contains(t, otherPackages, flowPackagePrefix+"abc/def/ghi/mno/def") - require.Contains(t, otherPackages, flowPackagePrefix+"abc/def/ghi/mno/ghi") - require.Contains(t, otherPackages, flowPackagePrefix+"abc/def/jkl") - require.Contains(t, otherPackages, flowPackagePrefix+"abc/def/jkl/mno") - require.Contains(t, otherPackages, flowPackagePrefix+"abc/def/jkl/mno/pqr") - require.Contains(t, otherPackages, flowPackagePrefix+"def") - require.Contains(t, otherPackages, flowPackagePrefix+"def/abc") - require.Contains(t, otherPackages, flowPackagePrefix+"jkl") - require.Contains(t, otherPackages, flowPackagePrefix+"pqr") - require.Contains(t, otherPackages, flowPackagePrefix+"vwx") - require.Contains(t, otherPackages, flowPackagePrefix+"vwx/ghi") - require.Contains(t, otherPackages, flowPackagePrefix+"yz") -} - -// TestGenerateTestMatrix_CustomRunners tests that the test matrix is generated correctly where the target packages include top level -// packages as well as sub packages. It also tests having 2 different custom CI runners, as well as default runners. -func TestGenerateTestMatrix_CustomRunners(t *testing.T) { - target, seenPackages := listTargetPackages([]string{"abc/def", "def:foo-runner", "ghi", "vwx/ghi:foo-runner2"}, getAllFlowPackages()) - require.Equal(t, 4, len(target.packages)) - require.Equal(t, 4, len(target.runners)) - require.Equal(t, 15, len(seenPackages)) - - otherPackages := listOtherPackages(getAllFlowPackages(), seenPackages) - - matrix := generateTestMatrix(target, otherPackages) - - // should be 4 groups in test matrix: abc/def, def, ghi, vwx/ghi, others - require.Equal(t, 5, len(matrix)) - - require.Contains(t, matrix, testMatrix{ - Name: "abc/def", - Packages: "github.com/onflow/flow-go/abc/def github.com/onflow/flow-go/abc/def/ghi github.com/onflow/flow-go/abc/def/ghi/jkl github.com/onflow/flow-go/abc/def/ghi/jkl/mno github.com/onflow/flow-go/abc/def/ghi/jkl/mno/pqr github.com/onflow/flow-go/abc/def/ghi/mno/abc github.com/onflow/flow-go/abc/def/ghi/mno/def github.com/onflow/flow-go/abc/def/ghi/mno/ghi github.com/onflow/flow-go/abc/def/jkl github.com/onflow/flow-go/abc/def/jkl/mno github.com/onflow/flow-go/abc/def/jkl/mno/pqr", - Runner: "ubuntu-latest"}, - ) - require.Contains(t, matrix, testMatrix{ - Name: "def", - Packages: "github.com/onflow/flow-go/def github.com/onflow/flow-go/def/abc", - Runner: "foo-runner"}, - ) - require.Contains(t, matrix, testMatrix{ - Name: "ghi", - Packages: "github.com/onflow/flow-go/ghi", - Runner: "ubuntu-latest"}, - ) - require.Contains(t, matrix, testMatrix{ - Name: "vwx/ghi", - Packages: "github.com/onflow/flow-go/vwx/ghi", - Runner: "foo-runner2"}, - ) - require.Contains(t, matrix, testMatrix{ - Name: "others", - Packages: "github.com/onflow/flow-go/abc github.com/onflow/flow-go/abc/123 github.com/onflow/flow-go/jkl github.com/onflow/flow-go/mno/abc github.com/onflow/flow-go/pqr github.com/onflow/flow-go/stu github.com/onflow/flow-go/vwx github.com/onflow/flow-go/yz", - Runner: "ubuntu-latest"}, - ) -} - -// TestGenerateTestMatrix_SubSubPackages tests that the test matrix is generated correctly where the target packages -// include 2nd and 3rd level sub packages. It also tests having 2 different custom CI runners, as well as default runners. -func TestGenerateTestMatrix_SubSubPackages(t *testing.T) { - target, seenPackages := listTargetPackages([]string{"abc/def/ghi:foo-runner1", "abc/def/jkl:foo-runner2", "abc"}, getAllFlowPackages()) - require.Equal(t, 3, len(target.packages)) - require.Equal(t, 3, len(target.runners)) - require.Equal(t, 13, len(seenPackages)) - - otherPackages := listOtherPackages(getAllFlowPackages(), seenPackages) - - matrix := generateTestMatrix(target, otherPackages) - - // should be 4 groups in test matrix: abc/def/ghi, abc/def/jkl, abc, others - require.Equal(t, 4, len(matrix)) - - require.Contains(t, matrix, testMatrix{ - Name: "abc/def/ghi", - Packages: "github.com/onflow/flow-go/abc/def/ghi github.com/onflow/flow-go/abc/def/ghi/jkl github.com/onflow/flow-go/abc/def/ghi/jkl/mno github.com/onflow/flow-go/abc/def/ghi/jkl/mno/pqr github.com/onflow/flow-go/abc/def/ghi/mno/abc github.com/onflow/flow-go/abc/def/ghi/mno/def github.com/onflow/flow-go/abc/def/ghi/mno/ghi", - Runner: "foo-runner1"}, - ) - - require.Contains(t, matrix, testMatrix{ - Name: "abc/def/jkl", - Packages: "github.com/onflow/flow-go/abc/def/jkl github.com/onflow/flow-go/abc/def/jkl/mno github.com/onflow/flow-go/abc/def/jkl/mno/pqr", - Runner: "foo-runner2"}, - ) - - // parent package should not have any packages from its sub packages because they were already included in the sub package groups - require.Contains(t, matrix, testMatrix{ - Name: "abc", - Packages: "github.com/onflow/flow-go/abc github.com/onflow/flow-go/abc/123 github.com/onflow/flow-go/abc/def", - Runner: "ubuntu-latest"}, - ) - - require.Contains(t, matrix, testMatrix{ - Name: "others", - Packages: "github.com/onflow/flow-go/def github.com/onflow/flow-go/def/abc github.com/onflow/flow-go/ghi github.com/onflow/flow-go/jkl github.com/onflow/flow-go/mno/abc github.com/onflow/flow-go/pqr github.com/onflow/flow-go/stu github.com/onflow/flow-go/vwx github.com/onflow/flow-go/vwx/ghi github.com/onflow/flow-go/yz", - Runner: "ubuntu-latest"}, - ) -} From 20edd3afd6fb1fe33303fabfe5b3d4ab40ad9014 Mon Sep 17 00:00:00 2001 From: Khalil Claybon Date: Mon, 4 Mar 2024 17:21:25 -0500 Subject: [PATCH 19/23] Update Makefile --- integration/Makefile | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/integration/Makefile b/integration/Makefile index 91be524b251..39c84f02a43 100644 --- a/integration/Makefile +++ b/integration/Makefile @@ -1,6 +1,8 @@ # Name of the cover profile COVER_PROFILE := cover.out +GO_TEST_PACKAGES := `go list ./... | grep -v -e integration/tests` + # allows CI to specify whether to have race detection on / off ifeq ($(RACE_DETECTOR),1) RACE_FLAG := -race @@ -19,7 +21,7 @@ integration-test: access-tests ghost-tests mvp-tests execution-tests verificatio # Run unit tests for test utilities in this module .PHONY: test test: - $(CGO_FLAG) go test $(if $(VERBOSE),-v,) -coverprofile=$(COVER_PROFILE) $(RACE_FLAG) $(if $(JSON_OUTPUT),-json,) $(if $(NUM_RUNS),-count $(NUM_RUNS),) `go list ./... | grep -v -e integration/tests` + $(CGO_FLAG) go test $(if $(VERBOSE),-v,) -coverprofile=$(COVER_PROFILE) $(RACE_FLAG) $(if $(JSON_OUTPUT),-json,) $(if $(NUM_RUNS),-count $(NUM_RUNS),) $(GO_TEST_PACKAGES) .PHONY: access-tests access-tests: access-cohort1-tests access-cohort2-tests access-cohort3-tests From 02f4351b804adf6b3598f546bc3b640827da8ae6 Mon Sep 17 00:00:00 2001 From: ramtinms Date: Tue, 5 Mar 2024 19:15:34 -0800 Subject: [PATCH 20/23] add more checks on the result --- fvm/evm/handler/handler.go | 40 +++++++++++++++++++++++++++++++++----- fvm/evm/types/errors.go | 3 +++ 2 files changed, 38 insertions(+), 5 deletions(-) diff --git a/fvm/evm/handler/handler.go b/fvm/evm/handler/handler.go index 249609da7c6..477edbc9a63 100644 --- a/fvm/evm/handler/handler.go +++ b/fvm/evm/handler/handler.go @@ -96,6 +96,10 @@ func (h *ContractHandler) deployCOA(uuid uint64) (types.Address, error) { if err != nil { return types.Address{}, err } + if res == nil || res.Failed() { + return types.Address{}, types.ErrDirectCallExecutionFailed + } + return res.DeployedContractAddress, nil } @@ -485,8 +489,17 @@ func (a *Account) deposit(v *types.FLOWTokenVault) error { if err != nil { return err } - _, err = a.fch.executeAndHandleCall(ctx, call, v.Balance(), false) - return err + + res, err := a.fch.executeAndHandleCall(ctx, call, v.Balance(), false) + if err != nil { + return err + } + + if res == nil || res.Failed() { + return types.ErrDirectCallExecutionFailed + } + + return nil } // Withdraw deducts the balance from the account and @@ -515,11 +528,15 @@ func (a *Account) withdraw(b types.Balance) (*types.FLOWTokenVault, error) { return nil, types.ErrWithdrawBalanceRounding } - _, err = a.fch.executeAndHandleCall(ctx, call, b, true) + res, err := a.fch.executeAndHandleCall(ctx, call, b, true) if err != nil { return nil, err } + if res == nil || res.Failed() { + return nil, types.ErrDirectCallExecutionFailed + } + return types.NewFlowTokenVault(b), nil } @@ -540,8 +557,16 @@ func (a *Account) transfer(to types.Address, balance types.Balance) error { if err != nil { return err } - _, err = a.fch.executeAndHandleCall(ctx, call, nil, false) - return err + res, err := a.fch.executeAndHandleCall(ctx, call, nil, false) + if err != nil { + return err + } + + if res == nil || res.Failed() { + return types.ErrDirectCallExecutionFailed + } + + return nil } // Deploy deploys a contract to the EVM environment @@ -570,6 +595,11 @@ func (a *Account) deploy(code types.Code, gaslimit types.GasLimit, balance types if err != nil { return types.Address{}, err } + + if res == nil || res.Failed() { + return types.Address{}, types.ErrDirectCallExecutionFailed + } + return types.Address(res.DeployedContractAddress), nil } diff --git a/fvm/evm/types/errors.go b/fvm/evm/types/errors.go index 6d2a89801e9..999e8f5c571 100644 --- a/fvm/evm/types/errors.go +++ b/fvm/evm/types/errors.go @@ -109,6 +109,9 @@ var ( // yeild to rounding error, i.e. the balance contains fractions smaller than 10^8 Flow (smallest unit allowed to transfer). ErrWithdrawBalanceRounding = NewEVMValidationError(errors.New("withdraw failed! the balance is susceptible to the rounding error")) + // ErrDirectCallExecutionFailed is returned when the direct call execution has failed. + ErrDirectCallExecutionFailed = NewEVMValidationError(errors.New("direct call execution failed")) + // ErrInsufficientTotalSupply is returned when flow token // is withdraw request is there but not enough balance is on EVM vault // this should never happen but its a saftey measure to protect Flow against EVM issues. From 6e5bf109a2b891fb00ebbcce702dc3241c2abade Mon Sep 17 00:00:00 2001 From: Khalil Claybon Date: Wed, 6 Mar 2024 01:58:15 -0500 Subject: [PATCH 21/23] Update tools/test_matrix_generator/matrix_test.go Co-authored-by: Jordan Schalm --- tools/test_matrix_generator/matrix_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/tools/test_matrix_generator/matrix_test.go b/tools/test_matrix_generator/matrix_test.go index d01ca085031..a155b541f62 100644 --- a/tools/test_matrix_generator/matrix_test.go +++ b/tools/test_matrix_generator/matrix_test.go @@ -52,7 +52,6 @@ func TestBuildMatrices(t *testing.T) { require.Equal(t, name, matrices[0].Name) require.Equal(t, runner, matrices[0].Runner) require.Equal(t, fmt.Sprintf("%s %s %s ", allPackges[0].PkgPath, allPackges[1].PkgPath, allPackges[2].PkgPath), matrices[0].Packages) - fmt.Println(matrices[0].Name, matrices[0].Runner, matrices[0].Packages) }) t.Run("top level package with sub packages include others", func(t *testing.T) { topLevelPkgName := "network" From d05ec885168f631a6c9171deafc07d0d1bdb4d57 Mon Sep 17 00:00:00 2001 From: Khalil Claybon Date: Wed, 6 Mar 2024 02:03:02 -0500 Subject: [PATCH 22/23] Update matrix.go --- tools/test_matrix_generator/matrix.go | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/tools/test_matrix_generator/matrix.go b/tools/test_matrix_generator/matrix.go index f63999b378a..2a0aeef3797 100644 --- a/tools/test_matrix_generator/matrix.go +++ b/tools/test_matrix_generator/matrix.go @@ -66,10 +66,10 @@ type testMatrix struct { } // newTestMatrix returns a new testMatrix, if runner is empty "" set the runner to the defaultCIRunner. -func newTestMatrix(name, runner string) *testMatrix { +func newTestMatrix(name, runner, pkgs string) *testMatrix { t := &testMatrix{ Name: name, - Packages: "", + Packages: pkgs, Runner: runner, } @@ -152,7 +152,6 @@ func processSubpackages(subPkgs []*subpackage, allPkgs []*packages.Package, seen testMatrices := make([]*testMatrix, 0) for _, subPkg := range subPkgs { pkgPath := fullGoPackagePath(subPkg.Name) - subPkgTestMatrix := newTestMatrix(subPkg.Name, subPkg.Runner) // this is the list of allPackages that used with the go test command var testPkgStrBuilder strings.Builder for _, p := range allPkgs { @@ -161,15 +160,13 @@ func processSubpackages(subPkgs []*subpackage, allPkgs []*packages.Package, seen seenPath(p.PkgPath) } } - subPkgTestMatrix.Packages = testPkgStrBuilder.String() - testMatrices = append(testMatrices, subPkgTestMatrix) + testMatrices = append(testMatrices, newTestMatrix(subPkg.Name, subPkg.Runner, testPkgStrBuilder.String())) } return testMatrices } // processTopLevelPackages creates test matrix for the top level package excluding any packages from the exclude list. func processTopLevelPackage(pkg *flowGoPackage, allPkgs []*packages.Package, seenPath func(p string), seen func(p string) bool) *testMatrix { - topLevelTestMatrix := newTestMatrix(pkg.Name, pkg.Runner) var topLevelTestPkgStrBuilder strings.Builder for _, p := range allPkgs { if !seen(p.PkgPath) { @@ -186,13 +183,11 @@ func processTopLevelPackage(pkg *flowGoPackage, allPkgs []*packages.Package, see } } } - topLevelTestMatrix.Packages = topLevelTestPkgStrBuilder.String() - return topLevelTestMatrix + return newTestMatrix(pkg.Name, pkg.Runner, topLevelTestPkgStrBuilder.String()) } // buildOthersTestMatrix builds an others test matrix that includes all packages in a path not explicitly set in the packages list of a config. func buildOthersTestMatrix(allPkgs []*packages.Package, seen func(p string) bool) *testMatrix { - othersTestMatrix := newTestMatrix("others", "") var othersTestPkgStrBuilder strings.Builder for _, otherPkg := range allPkgs { if !seen(otherPkg.PkgPath) { @@ -201,8 +196,7 @@ func buildOthersTestMatrix(allPkgs []*packages.Package, seen func(p string) bool } if othersTestPkgStrBuilder.Len() > 0 { - othersTestMatrix.Packages = othersTestPkgStrBuilder.String() - return othersTestMatrix + return newTestMatrix("others", "", othersTestPkgStrBuilder.String()) } return nil From 4078e67808e59cb03eef85eb179cb80fda8784a1 Mon Sep 17 00:00:00 2001 From: Khalil Claybon Date: Wed, 6 Mar 2024 02:49:13 -0500 Subject: [PATCH 23/23] Update ci.yml --- .github/workflows/ci.yml | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 69702c0d55b..b14ba2d69c5 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -37,7 +37,7 @@ jobs: - name: Checkout repo uses: actions/checkout@v3 - name: Setup Go - uses: buildjet/setup-go@v4 + uses: actions/setup-go@v4 timeout-minutes: 10 # fail fast. sometimes this step takes an extremely long time with: go-version: ${{ env.GO_VERSION }} @@ -63,7 +63,7 @@ jobs: - name: Checkout repo uses: actions/checkout@v3 - name: Setup Go - uses: buildjet/setup-go@v4 + uses: actions/setup-go@v4 timeout-minutes: 10 # fail fast. sometimes this step takes an extremely long time with: go-version: ${{ env.GO_VERSION }} @@ -82,7 +82,7 @@ jobs: - name: Checkout repo uses: actions/checkout@v3 - name: Setup Go - uses: buildjet/setup-go@v4 + uses: actions/setup-go@v4 timeout-minutes: 10 # fail fast. sometimes this step takes an extremely long time with: go-version: ${{ env.GO_VERSION }} @@ -100,7 +100,7 @@ jobs: - name: Checkout repo uses: actions/checkout@v3 - name: Setup Go - uses: buildjet/setup-go@v4 + uses: actions/setup-go@v4 timeout-minutes: 10 # fail fast. sometimes this step takes an extremely long time with: go-version: ${{ env.GO_VERSION }} @@ -118,7 +118,7 @@ jobs: - name: Checkout repo uses: actions/checkout@v3 - name: Setup Go - uses: buildjet/setup-go@v4 + uses: actions/setup-go@v4 timeout-minutes: 10 # fail fast. sometimes this step takes an extremely long time with: go-version: ${{ env.GO_VERSION }} @@ -140,7 +140,7 @@ jobs: - name: Checkout repo uses: actions/checkout@v3 - name: Setup Go - uses: buildjet/setup-go@v4 + uses: actions/setup-go@v4 timeout-minutes: 10 # fail fast. sometimes this step takes an extremely long time with: go-version: ${{ env.GO_VERSION }} @@ -176,7 +176,7 @@ jobs: - name: Checkout repo uses: actions/checkout@v3 - name: Setup Go - uses: buildjet/setup-go@v4 + uses: actions/setup-go@v4 timeout-minutes: 10 # fail fast. sometimes this step takes an extremely long time with: go-version: ${{ env.GO_VERSION }} @@ -211,7 +211,7 @@ jobs: # all tags are needed for integration tests fetch-depth: 0 - name: Setup Go - uses: buildjet/setup-go@v4 + uses: actions/setup-go@v4 timeout-minutes: 10 # fail fast. sometimes this step takes an extremely long time with: go-version: ${{ env.GO_VERSION }} @@ -253,7 +253,7 @@ jobs: - name: Checkout repo uses: actions/checkout@v3 - name: Setup Go - uses: buildjet/setup-go@v4 + uses: actions/setup-go@v4 timeout-minutes: 10 # fail fast. sometimes this step takes an extremely long time with: go-version: ${{ env.GO_VERSION }} @@ -340,7 +340,7 @@ jobs: # all tags are needed for integration tests fetch-depth: 0 - name: Setup Go - uses: buildjet/setup-go@v4 + uses: actions/setup-go@v4 timeout-minutes: 10 # fail fast. sometimes this step takes an extremely long time with: go-version: ${{ env.GO_VERSION }}