Merge pull request #7065 from bobanetwork/e2e-externals-specify-test-skips

op-e2e: allow external processes to skip tests

op-e2e/Makefile

@@ -13,7 +13,7 @@ test: pre-test test-ws
 
 test-external-%: pre-test
 	make -C ./external_$*/
-	$(go_test) $(go_test_flags) --externalL2 ./external_$*/shim
+	$(go_test) $(go_test_flags) --externalL2 ./external_$*/
 
 test-ws: pre-test
 	$(go_test) $(go_test_flags) ./...

op-e2e/config/init.go

 package config
 
 import (
+	"encoding/json"
+	"errors"
 	"flag"
 	"fmt"
 	"os"
@@ -9,6 +11,7 @@ import (
 	"time"
 
 	"github.com/ethereum-optimism/optimism/op-chain-ops/genesis"
+	"github.com/ethereum-optimism/optimism/op-e2e/external"
 	"github.com/ethereum/go-ethereum/common/hexutil"
 	"github.com/ethereum/go-ethereum/core/state"
 )
@@ -26,21 +29,18 @@ var (
 	L1Deployments *genesis.L1Deployments
 	// DeployConfig represents the deploy config used by the system.
 	DeployConfig *genesis.DeployConfig
-	// ExternalL2Nodes is the shim to use if external ethereum client testing is
+	// ExternalL2Shim is the shim to use if external ethereum client testing is
 	// enabled
-	ExternalL2Nodes string
+	ExternalL2Shim string
+	// ExternalL2TestParms is additional metadata for executing external L2
+	// tests.
+	ExternalL2TestParms external.TestParms
 	// EthNodeVerbosity is the level of verbosity to output
 	EthNodeVerbosity int
 )
 
-// Init testing to enable test flags
-var _ = func() bool {
-	testing.Init()
-	return true
-}()
-
 func init() {
-	var l1AllocsPath, l1DeploymentsPath, deployConfigPath string
+	var l1AllocsPath, l1DeploymentsPath, deployConfigPath, externalL2 string
 
 	cwd, err := os.Getwd()
 	if err != nil {
@@ -58,8 +58,9 @@ func init() {
 	flag.StringVar(&l1AllocsPath, "l1-allocs", defaultL1AllocsPath, "")
 	flag.StringVar(&l1DeploymentsPath, "l1-deployments", defaultL1DeploymentsPath, "")
 	flag.StringVar(&deployConfigPath, "deploy-config", defaultDeployConfigPath, "")
-	flag.StringVar(&ExternalL2Nodes, "externalL2", "", "Enable tests with external L2")
+	flag.StringVar(&externalL2, "externalL2", "", "Enable tests with external L2")
 	flag.IntVar(&EthNodeVerbosity, "ethLogVerbosity", 3, "The level of verbosity to use for the eth node logs")
+	testing.Init() // Register test flags before parsing
 	flag.Parse()
 
 	if err := allExist(l1AllocsPath, l1DeploymentsPath, deployConfigPath); err != nil {
@@ -92,6 +93,40 @@ func init() {
 	if L1Deployments != nil {
 		DeployConfig.SetDeployments(L1Deployments)
 	}
+
+	if externalL2 != "" {
+		if err := initExternalL2(externalL2); err != nil {
+			panic(fmt.Errorf("could not initialize external L2: %w", err))
+		}
+	}
+}
+
+func initExternalL2(externalL2 string) error {
+	var err error
+	ExternalL2Shim, err = filepath.Abs(filepath.Join(externalL2, "shim"))
+	if err != nil {
+		return fmt.Errorf("could not compute abs of externalL2Nodes shim: %w", err)
+	}
+
+	_, err = os.Stat(ExternalL2Shim)
+	if err != nil {
+		return fmt.Errorf("failed to stat externalL2Nodes path: %w", err)
+	}
+
+	file, err := os.Open(filepath.Join(externalL2, "test_parms.json"))
+	if err != nil {
+		if errors.Is(err, os.ErrNotExist) {
+			return nil
+		}
+		return fmt.Errorf("could not open external L2 test parms: %w", err)
+	}
+	defer file.Close()
+
+	if err := json.NewDecoder(file).Decode(&ExternalL2TestParms); err != nil {
+		return fmt.Errorf("could not decode external L2 test parms: %w", err)
+	}
+
+	return nil
 }
 
 func allExist(filenames ...string) error {

op-e2e/external/config.go

 package external
 
 import (
+	"bytes"
 	"encoding/json"
 	"os"
+	"strings"
+	"testing"
 )
 
 type Config struct {
@@ -40,3 +43,26 @@ type Endpoints struct {
 	HTTPAuthEndpoint string `json:"http_auth_endpoint"`
 	WSAuthEndpoint   string `json:"ws_auth_endpoint"`
 }
+
+type TestParms struct {
+	// SkipTests is a map from test name to skip message.  The skip message may
+	// be arbitrary, but the test name should match the skipped test (either
+	// base, or a sub-test) exactly.  Precisely, the skip name must match rune for
+	// rune starting with the first rune.  If the skip name does not match all
+	// runes, the first mismatched rune must be a '/'.
+	SkipTests map[string]string `json:"skip_tests"`
+}
+
+func (tp TestParms) SkipIfNecessary(t *testing.T) {
+	if len(tp.SkipTests) == 0 {
+		return
+	}
+	var base bytes.Buffer
+	for _, name := range strings.Split(t.Name(), "/") {
+		base.WriteString(name)
+		if msg, ok := tp.SkipTests[base.String()]; ok {
+			t.Skip(msg)
+		}
+		base.WriteRune('/')
+	}
+}

op-e2e/external_geth/README.md

@@ -41,6 +41,16 @@ process and looks for the lines indicating that the HTTP server and Auth HTTP
 server have started up.  It then reads the ports which were allocated (because
 the requested ports were passed in as ephemeral via the CLI arguments).
 
+## Skipping tests
+
+Although ideally, all tests would be structured such that they may execute
+either with an in-process op-geth or with an extra-process ethereum client,
+this is not always the case.  You may optionally create a `test_parms.json`
+file in the `external_<your-client>` directory, as there is in the
+`external_geth` directory which specifies a map of tests to skip, and
+accompanying skip text.  See the `op-e2e/external/config.go` file for more
+details.
+
 ## Generalization
 
 This shim is included to help document an demonstrate the usage of the

op-e2e/external_geth/test_parms.json

+{
+  "skip_tests":{
+    "TestPendingGasLimit":"This test requires directly modifying go structures and cannot be implemented with flags"
+  }
+}

op-e2e/setup.go

@@ -78,6 +78,8 @@ func newTxMgrConfig(l1Addr string, privKey *ecdsa.PrivateKey) txmgr.CLIConfig {
 }
 
 func DefaultSystemConfig(t *testing.T) SystemConfig {
+	config.ExternalL2TestParms.SkipIfNecessary(t)
+
 	secrets, err := e2eutils.DefaultMnemonicConfig.Secrets()
 	require.NoError(t, err)
 	deployConfig := config.DeployConfig.Copy()
@@ -139,7 +141,7 @@ func DefaultSystemConfig(t *testing.T) SystemConfig {
 		GethOptions:                map[string][]GethOption{},
 		P2PTopology:                nil, // no P2P connectivity by default
 		NonFinalizedProposals:      false,
-		ExternalL2Nodes:            config.ExternalL2Nodes,
+		ExternalL2Shim:             config.ExternalL2Shim,
 		BatcherTargetL1TxSizeBytes: 100_000,
 	}
 }
@@ -175,7 +177,7 @@ type SystemConfig struct {
 	ProposerLogger log.Logger
 	BatcherLogger  log.Logger
 
-	ExternalL2Nodes string
+	ExternalL2Shim string
 
 	// map of outbound connections to other nodes. Node names prefixed with "~" are unconnected but linked.
 	// A nil map disables P2P completely.
@@ -438,7 +440,7 @@ func (cfg SystemConfig) Start(t *testing.T, _opts ...SystemConfigOption) (*Syste
 
 	for name := range cfg.Nodes {
 		var ethClient EthInstance
-		if cfg.ExternalL2Nodes == "" {
+		if cfg.ExternalL2Shim == "" {
 			node, backend, err := initL2Geth(name, big.NewInt(int64(cfg.DeployConfig.L2ChainID)), l2Genesis, cfg.JWTFilePath, cfg.GethOptions[name]...)
 			if err != nil {
 				return nil, err
@@ -459,7 +461,7 @@ func (cfg SystemConfig) Start(t *testing.T, _opts ...SystemConfigOption) (*Syste
 			}
 			ethClient = (&ExternalRunner{
 				Name:    name,
-				BinPath: cfg.ExternalL2Nodes,
+				BinPath: cfg.ExternalL2Shim,
 				Genesis: l2Genesis,
 				JWTPath: cfg.JWTFilePath,
 			}).Run(t)

op-e2e/system_test.go

@@ -5,7 +5,6 @@ import (
 	"fmt"
 	"math/big"
 	"os"
-	"path/filepath"
 	"runtime"
 	"testing"
 	"time"
@@ -45,23 +44,8 @@ import (
 )
 
 func TestMain(m *testing.M) {
-	if config.ExternalL2Nodes != "" {
-		fmt.Println("Running tests with external L2 process adapter at ", config.ExternalL2Nodes)
-		shimPath, err := filepath.Abs(config.ExternalL2Nodes)
-		if err != nil {
-			fmt.Printf("Could not compute abs of externalL2Nodes shim: %s\n", err)
-			os.Exit(2)
-		}
-		// We convert the passed in path to an absolute path, as it simplifies
-		// the path handling logic for the rest of the testing
-		config.ExternalL2Nodes = shimPath
-
-		_, err = os.Stat(config.ExternalL2Nodes)
-		if err != nil {
-			fmt.Printf("Failed to stat externalL2Nodes path: %s\n", err)
-			os.Exit(3)
-		}
-
+	if config.ExternalL2Shim != "" {
+		fmt.Println("Running tests with external L2 process adapter at ", config.ExternalL2Shim)
 		// As these are integration tests which launch many other processes, the
 		// default parallelism makes the tests flaky.  This change aims to
 		// reduce the flakiness of these tests.
@@ -273,13 +257,6 @@ func TestPendingGasLimit(t *testing.T) {
 	InitParallel(t)
 
 	cfg := DefaultSystemConfig(t)
-	if cfg.ExternalL2Nodes != "" {
-		// Some eth clients such as Erigon don't currently build blocks until
-		// they receive the engine call which includes the gas limit.  After we
-		// provide a mechanism for external clients to advertise test support we
-		// should enable for those which support it.
-		t.Skip()
-	}
 
 	// configure the L2 gas limit to be high, and the pending gas limits to be lower for resource saving.
 	cfg.DeployConfig.L2GenesisBlockGasLimit = 30_000_000