e2e: Improve test isolation with per-test funded key allocation

Author: maru-ava

scripts/tests.e2e.sh

@@ -17,6 +17,9 @@ if [[ -z "${AVALANCHEGO_PATH}" ]]; then
   echo "Usage: ${0} [AVALANCHEGO_PATH]" >> /dev/stderr
   exit 255
 fi
+# Avoid requiring test execution to use the same working directory as
+# the script by resolving a relative path.
+AVALANCHEGO_PATH="$(realpath ${AVALANCHEGO_PATH})"
 
 # Set the CGO flags to use the portable version of BLST
 #
@@ -39,10 +42,11 @@ ACK_GINKGO_RC=true ginkgo build ./tests/e2e
 
 #################################
 echo "running e2e tests against the local cluster with ${AVALANCHEGO_PATH}"
-./tests/e2e/e2e.test \
---ginkgo.v \
---avalanchego-path=${AVALANCHEGO_PATH} \
---test-keys-file=tests/test.insecure.secp256k1.keys \
+# - Execute in parallel (-p) with the ginkgo cli to minimize execution time.
+#   The test binary by itself isn't capable of running specs in parallel.
+# - Execute in random order to identify unwanted dependency
+ginkgo -p -v --randomize-all ./tests/e2e/e2e.test -- \
+ --avalanchego-path=${AVALANCHEGO_PATH} \
 && EXIT_CODE=$? || EXIT_CODE=$?
 
 if [[ ${EXIT_CODE} -gt 0 ]]; then

tests/e2e/banff/suites.go

@@ -11,7 +11,6 @@ import (
 
 	"github.com/onsi/gomega"
 
-	"github.com/ava-labs/avalanchego/genesis"
 	"github.com/ava-labs/avalanchego/ids"
 	"github.com/ava-labs/avalanchego/tests"
 	"github.com/ava-labs/avalanchego/tests/e2e"
@@ -24,10 +23,6 @@ import (
 )
 
 var _ = ginkgo.Describe("[Banff]", func() {
-	ginkgo.BeforeEach(func() {
-		e2e.Env.EnsurePristineNetwork()
-	})
-
 	ginkgo.It("can send custom assets X->P and P->X",
 		// use this for filtering tests by labels
 		// ref. https://onsi.github.io/ginkgo/#spec-labels
@@ -36,13 +31,11 @@ var _ = ginkgo.Describe("[Banff]", func() {
 			"banff",
 		),
 		func() {
-			uris := e2e.Env.GetURIsRW()
-			gomega.Expect(uris).ShouldNot(gomega.BeEmpty())
-
-			kc := secp256k1fx.NewKeychain(genesis.EWOQKey)
+			key := e2e.Env.AllocateTestKeys(1)[0]
+			kc := secp256k1fx.NewKeychain(key)
 			var wallet primary.Wallet
 			ginkgo.By("initialize wallet", func() {
-				walletURI := uris[0]
+				walletURI := e2e.Env.URIs[0]
 
 				// 5-second is enough to fetch initial UTXOs for test cluster in "primary.NewWallet"
 				ctx, cancel := context.WithTimeout(context.Background(), e2e.DefaultWalletCreationTimeout)
@@ -63,7 +56,7 @@ var _ = ginkgo.Describe("[Banff]", func() {
 			owner := &secp256k1fx.OutputOwners{
 				Threshold: 1,
 				Addrs: []ids.ShortID{
-					genesis.EWOQKey.PublicKey().Address(),
+					key.Address(),
 				},
 			}
 

tests/e2e/describe.go

@@ -13,6 +13,12 @@ func DescribeXChain(text string, body func()) bool {
 	return ginkgo.Describe("[X-Chain] "+text, body)
 }
 
+// DescribeXChainSerial annotates serial tests for X-Chain.
+// Can run with any type of cluster (e.g., local, fuji, mainnet).
+func DescribeXChainSerial(text string, body func()) bool {
+	return ginkgo.Describe("[X-Chain] "+text, ginkgo.Serial, body)
+}
+
 // DescribePChain annotates the tests for P-Chain.
 // Can run with any type of cluster (e.g., local, fuji, mainnet).
 func DescribePChain(text string, body func()) bool {

tests/e2e/e2e.go

@@ -5,21 +5,15 @@
 package e2e
 
 import (
-	"errors"
-	"fmt"
-	"os"
-	"sync"
 	"time"
 
 	ginkgo "github.com/onsi/ginkgo/v2"
-	"github.com/onsi/gomega"
+	"github.com/stretchr/testify/require"
 
-	"github.com/ava-labs/avalanchego/ids"
-	"github.com/ava-labs/avalanchego/tests"
+	"github.com/ava-labs/avalanchego/tests/fixture"
 	"github.com/ava-labs/avalanchego/tests/fixture/testnet"
 	"github.com/ava-labs/avalanchego/tests/fixture/testnet/local"
 	"github.com/ava-labs/avalanchego/utils/crypto/secp256k1"
-	"github.com/ava-labs/avalanchego/vms/secp256k1fx"
 )
 
 const (
@@ -29,256 +23,26 @@ const (
 	// Defines default tx confirmation timeout.
 	// Enough for test/custom networks.
 	DefaultConfirmTxTimeout = 20 * time.Second
-
-	DefaultShutdownTimeout = 2 * time.Minute
 )
 
-// Env is the global struct containing all we need to test
-var (
-	Env = &TestEnvironment{}
-
-	errNoKeyFile = errors.New("test keys file not provided")
-)
+// Env is used to access shared test fixture. Intended to be
+// initialized by SynchronizedBeforeSuite.
+var Env TestEnvironment
 
 type TestEnvironment struct {
-	avalancheGoExecPath string
-	isPersistentNetwork bool
-
-	setupCalled bool
-
-	urisMu sync.RWMutex
-	uris   []string
-
-	networkDir string
-
-	// isNetworkPristine is set to true after network creation and
-	// false after a call to GetURIsRW to know when a test needs the
-	// network to be recreated.
-	isNetworkPristine bool
-
-	testKeysMu sync.RWMutex
-	testKeys   []*secp256k1.PrivateKey
-}
-
-// Setup ensures the environment is configured with a network that can
-// be targeted for testing.
-func (te *TestEnvironment) Setup(
-	avalancheGoExecPath string,
-	testKeysFile string,
-	persistentNetworkDir string,
-) error {
-	// TODO(marun) Use testify instead of returning errors
-	if te.setupCalled {
-		return errors.New("setup has already been called and should only be called once")
-	} else {
-		te.setupCalled = true
-	}
-
-	// Need to always configure avalanchego.  Even if using a
-	// persistent network, avalanchego is required for operations like
-	// node addition.
-	if avalancheGoExecPath != "" {
-		if _, err := os.Stat(avalancheGoExecPath); err != nil {
-			return fmt.Errorf("could not find avalanchego binary: %w", err)
-		}
-	}
-	te.avalancheGoExecPath = avalancheGoExecPath
-
-	// TODO(marun) Does this really have to be configurable? Maybe just embed.
-	err := te.LoadKeys(testKeysFile)
-	if err != nil {
-		return err
-	}
-
-	te.networkDir = persistentNetworkDir
-	if len(te.networkDir) > 0 {
-		tests.Outf("{{yellow}}Using a pre-existing network{{/}}\n")
-		te.isPersistentNetwork = true
-
-		network, err := local.LoadNetwork(te.networkDir)
-		if err != nil {
-			return fmt.Errorf("failed to load network: %w", err)
-		}
-		uris := network.GetURIs()
-		if len(uris) == 0 {
-			return fmt.Errorf("network path contains no nodes: %s", te.networkDir)
-		}
-		te.setURIs(uris)
-	}
-
-	// Network setup will be performed just-in-time by the first
-	// test that needs it. This avoids the cost of starting the
-	// default network when iterating on tests that won't use it.
-
-	return nil
-}
-
-func (te *TestEnvironment) LoadKeys(testKeysFile string) error {
-	// load test keys
-	if len(testKeysFile) == 0 {
-		return errNoKeyFile
-	}
-	testKeys, err := tests.LoadHexTestKeys(testKeysFile)
-	if err != nil {
-		return fmt.Errorf("failed loading test keys: %w", err)
-	}
-	te.setTestKeys(testKeys)
-	return nil
-}
-
-// EnsureNetwork starts a network if one is not already running.
-func (te *TestEnvironment) EnsureNetwork() {
-	if te.isPersistentNetwork {
-		tests.Outf("{{yellow}}using pre-existing network.{{/}}\n")
-		return
-	}
-
-	// TODO(marun) Add locking to support just-in-time network
-	// provisioning among tests executing in parallel.
-
-	// The absence of URIs is an indication that the network has not yet been started.
-	if len(te.GetURIsRO()) == 0 {
-		err := te.startCluster()
-		gomega.Expect(err).Should(gomega.BeNil())
-	}
-}
-
-// EnsurePristineNetwork attempts to ensure that the currently active
-// network is compatible with tests that require a pristine
-// (unmodified) state.
-func (te *TestEnvironment) EnsurePristineNetwork() {
-	if te.isPersistentNetwork {
-		tests.Outf("{{yellow}}unable to ensure pristine initial state with a pre-existing network.{{/}}\n")
-		return
-	}
-
-	networkRunning := len(te.networkDir) != 0
-	if networkRunning {
-		if te.isNetworkPristine {
-			tests.Outf("{{green}}network is already pristine.{{/}}\n")
-			return
-		}
-
-		tests.Outf("{{yellow}}network state is not pristine, the current network needs to be replaced.{{/}}\n")
-
-		tests.Outf("{{magenta}}shutting down the current network.{{/}}\n")
-		err := local.StopNetwork(te.networkDir)
-		gomega.Expect(err).Should(gomega.BeNil())
-		tests.Outf("{{green}}network shutdown successful.{{/}}\n")
-	}
-
-	err := te.startCluster()
-	gomega.Expect(err).Should(gomega.BeNil())
-}
-
-// startCluster launches a new network
-func (te *TestEnvironment) startCluster() error {
-	tests.Outf("{{magenta}}starting network with %q{{/}}\n", te.avalancheGoExecPath)
-
-	// TODO(marun) Ensure removal of tmp dir
-	tmpDir, err := os.MkdirTemp("", "")
-	if err != nil {
-		return fmt.Errorf("failed to create tmp dir for network: %w", err)
-	}
-	networkID := uint32(0) // Network ID will be generated
-	nodeCount := 5
-	network, err := local.StartNetwork(
-		ginkgo.GinkgoWriter,
-		tmpDir,
-		&local.LocalNetwork{
-			LocalConfig: local.LocalConfig{
-				ExecPath:       te.avalancheGoExecPath,
-				UseStaticPorts: false, // Avoid conflicting with persistent local networks
-			},
-		},
-		networkID,
-		nodeCount,
-	)
-	if err != nil {
-		return err
-	}
-
-	// TODO(marun) Remove need for locking
-	te.networkDir = network.Dir
-
-	uris := network.GetURIs()
-	if len(uris) == 0 {
-		return fmt.Errorf("network %s has no running nodes", te.networkDir)
-	}
-	te.setURIs(uris)
-	tests.Outf("{{green}}successfully started network: {{/}} %+v\n", uris)
-
-	te.isNetworkPristine = true
-
-	return nil
-}
-
-func (te *TestEnvironment) setURIs(us []string) {
-	te.urisMu.Lock()
-	te.uris = us
-	te.urisMu.Unlock()
-}
-
-// GetURIsRW returns the current network URIs for read and write
-// operations. The network is assumed to have been modified from its
-// initial state after this function has been called.
-//
-// TODO(marun) Find a better way of determining whether a network has
-// been modified from its initial state. Or maybe ensure most (if not
-// all) tests are capable of targeting any network in a
-// non-pathological state?
-func (te *TestEnvironment) GetURIsRW() []string {
-	te.urisMu.Lock()
-	us := te.uris
-	te.isNetworkPristine = false
-	te.urisMu.Unlock()
-	return us
-}
-
-// GetURIsRO returns the current network URIs for read-only operations.
-func (te *TestEnvironment) GetURIsRO() []string {
-	te.urisMu.RLock()
-	us := te.uris
-	te.urisMu.RUnlock()
-	return us
-}
-
-func (te *TestEnvironment) setTestKeys(ks []*secp256k1.PrivateKey) {
-	te.testKeysMu.Lock()
-	te.testKeys = ks
-	te.testKeysMu.Unlock()
-}
-
-func (te *TestEnvironment) GetTestKeys() ([]*secp256k1.PrivateKey, []ids.ShortID, *secp256k1fx.Keychain) {
-	te.testKeysMu.RLock()
-	testKeys := te.testKeys
-	te.testKeysMu.RUnlock()
-	testKeyAddrs := make([]ids.ShortID, len(testKeys))
-	for i := range testKeyAddrs {
-		testKeyAddrs[i] = testKeys[i].PublicKey().Address()
-	}
-	keyChain := secp256k1fx.NewKeychain(testKeys...)
-	return testKeys, testKeyAddrs, keyChain
+	NetworkDir   string
+	URIs         []string
+	KeyServerURI string
 }
 
-func (te *TestEnvironment) Teardown() error {
-	gomega.Expect(te.setupCalled).Should(gomega.BeTrue())
-
-	if te.isPersistentNetwork {
-		tests.Outf("{{yellow}}teardown not required for persistent network{{/}}\n")
-		return nil
-	}
-
-	networkRunning := len(te.networkDir) != 0
-	if networkRunning {
-		// Teardown the active network.
-		tests.Outf("{{red}}shutting down network{{/}}\n")
-		return local.StopNetwork(te.networkDir)
-	}
-
-	return nil
+func (te *TestEnvironment) GetNetwork() testnet.Network {
+	network, err := local.LoadNetwork(te.NetworkDir)
+	require.NoError(ginkgo.GinkgoT(), err)
+	return network
 }
 
-func (te *TestEnvironment) GetNetwork() (testnet.Network, error) {
-	return local.LoadNetwork(te.networkDir)
+func (te *TestEnvironment) AllocateTestKeys(count int) []*secp256k1.PrivateKey {
+	keys, err := fixture.AllocateTestKeys(te.KeyServerURI, count)
+	require.NoError(ginkgo.GinkgoT(), err)
+	return keys
 }