test: add import compliance tests #4537

JonathanOppenheimer · 2025-11-21T23:28:19Z

Why this should be merged

This PR accomplishes three things:

Prevent files inside of the vms/evm directory from importing packages within the graft/ directory.
- This is important because with the removal of coreth, there is not longer a natural boundary between the uplifted and non-uplifted code. We need to enforce such a boundary.
- This boundary is important, especially as other people individuals start work on properly uplfiting code post monorepo. Reviewers and developers may not be familiar with why we want this distinction to exist and programmatically enforcing this boundary will prevent mistakes and leakage of legacy EVM code into AvalancheGo.
Prevent libevm/pseudo from being imported anywhere in AvalancheGo

See this issue for additional context Remove libevm import restriction test from graft/ post merge. #4541

How this works

The boundaries are as follows:

graft/coreth can be imported everywhere in AvalancheGo except vms/evm.
- However, vms/evm/emulate is an exception to this rule
graft/subnet-evm can not be imported anywhere in AvalancheGo
- However, vms/evm/emulate is an exception to this rule

How this was tested

Existing CI. I also added a sample "bad" commit (404acd5) to show how CI would behave.

edit: this is no longer a good example and is out of date but I kept it to show the formatting -- pretend the file is in vms/evm/sample_bad.go.

Need to be documented in RELEASES.md?

No

tests/imports_test.go

graft/scripts/forbidden-geth-files.txt

maru-ava · 2025-11-24T19:02:31Z

graft/scripts/libevm-allowed-packages.txt

@@ -0,0 +1,49 @@
+# Allowed libevm packages that can be imported in the graft directory.


grafts/scripts should only be used for graft-related functionality and is likely to go away at some point once we've migrated everything we care about.

Yeah, this should also be vms/evm - I'll generalize to EVM code?

Yeah, vms/evm is a likely candidate. A key consideration of where to locate code is who will own it (i.e. be capable of approving changes). No point in requiring non-evm maintainers to approve evm code.

maru-ava · 2025-11-24T19:04:34Z

vms/evm/imports_test.go

@@ -0,0 +1,232 @@
+// Copyright (C) 2019-2025, Ava Labs, Inc. All rights reserved.


Please find somewhere else for this too, the root of tests/ is for utility functions that get imported, not unit tests like this one.

yup, vm/evm also seems appropriate

vms/evm/scripts/libevm-allowed-packages.txt

maru-ava · 2025-11-25T11:21:51Z

vms/evm/imports_test.go

+)
+
+// TestDoNotImportFromGraft ensures proper import rules for graft packages:
+// - graft/coreth can be imported anywhere EXCEPT vms/evm (but vms/evm/emulate is an exception)


Myabe provide more detail as to why these restrictions are necessary?

maru-ava · 2025-11-25T11:24:22Z

vms/evm/imports_test.go

+			return true
+		}
+
+		isInEmulate := strings.Contains(path, "/vms/evm/emulate/")


(No action required) Maybe localize the following 3 definitions to the conditionals that use them?

maru-ava · 2025-11-25T11:25:19Z

vms/evm/imports_test.go

+		isInEmulate := strings.Contains(path, "/vms/evm/emulate/")
+		isInVmsEvm := strings.Contains(path, "/vms/evm/")
+		isCoreth := strings.Contains(importPath, "/graft/coreth")
+		isSubnetEVM := strings.Contains(importPath, "/graft/subnet-evm")


(No action required) Not a biggie, but would prefer to see subnet-evm specific stuff as part of the migration not in advance.

But also, why is testing for these independent grafts conflated? Maybe implement 2 tests that use the same helpers, one for each graft location?

maru-ava · 2025-11-25T11:32:42Z

vms/evm/imports_test.go

+	require.Fail(t, formatImportViolations(foundImports, header))
+}
+
+// TestDoNotImportLibevmPseudo ensures that no code in the repository imports


(No action required) If it's libevm-internal only, why isn't the package called internal so that the restriction is enforced automatically by golang?

ARR4N · 2025-11-25T11:05:40Z

vms/evm/imports_test.go

+// Copyright (C) 2019-2025, Ava Labs, Inc. All rights reserved.
+// See the file LICENSE for licensing terms.
+
+package tests


Suggested change

package tests

package evm

ARR4N · 2025-11-25T11:07:22Z

vms/evm/imports_test.go

+// - graft/coreth can be imported anywhere EXCEPT vms/evm (but vms/evm/emulate is an exception)
+// - graft/subnet-evm cannot be imported anywhere EXCEPT vms/evm/emulate


Why are these the rules? I understand emulate being allowed to import, but not the other limitations.

Rule of thumb for all code, not just this instance: comments should explain "why", not "what" nor "how".

ARR4N · 2025-11-25T11:18:55Z

vms/evm/imports_test.go

+// - graft/coreth can be imported anywhere EXCEPT vms/evm (but vms/evm/emulate is an exception)
+// - graft/subnet-evm cannot be imported anywhere EXCEPT vms/evm/emulate
+func TestEnforceGraftImportBoundaries(t *testing.T) {
+	graftRegex := regexp.MustCompile(`^github\.com/ava-labs/avalanchego/graft(/|$)`)


(subjective, optional) I've seen enough bugs arising from regular expressions that could have been string comparisons that I tend to avoid them unless absolutely necessary. The functionality in this file could just be:

func isPackageIn(importPath, root string) bool { if importPath == root { return true } if len(importPath) < len(root)+1 { return false } return strings.HasPrefix(importPath, root) && importPath[len(root)] == '/' }

ARR4N · 2025-11-25T11:20:37Z

vms/evm/imports_test.go

+
+// findImportsMatchingPattern is a generalized function that finds all imports
+// matching a given regex pattern in the specified directory.
+// The filterFunc can be used to skip certain files or imports (return true to skip).


return true to skip

This is the opposite to what I'd expect when passing in a filter function:

func myFilter(file string) bool { return true // intuition says this is now included }

ARR4N · 2025-11-25T11:25:04Z

vms/evm/imports_test.go

+		if err != nil || !strings.HasSuffix(path, ".go") {
+			return err
+		}


Suggested change

if err != nil || !strings.HasSuffix(path, ".go") {

return err

}

if err != nil {

return err

}

if !strings.HasSuffix(path, ".go") {

return nil

}

Although functionally equivalent, this is much clearer and doesn't force the reader to reason about how the returned err will be nil in the non-Go instance.

ARR4N · 2025-11-25T11:43:05Z

vms/evm/imports_test.go

+
+// TestDoNotImportLibevmPseudo ensures that no code in the repository imports
+// the libevm/pseudo package, which is for internal libevm use only.
+func TestDoNotImportLibevmPseudo(t *testing.T) {


Suggested change

func TestDoNotImportLibevmPseudo(t *testing.T) {

func TestDoNotImportLibEVMPseudo(t *testing.T) {

Although the project name is stylised libevm, initialisms in Go identifiers should have all initials with the same capitalisation.

ARR4N · 2025-11-25T11:43:54Z

vms/evm/imports_test.go

+
+// TestDoNotImportLibevmPseudo ensures that no code in the repository imports
+// the libevm/pseudo package, which is for internal libevm use only.
+func TestDoNotImportLibevmPseudo(t *testing.T) {


Why does this warrant a separate test?

ARR4N · 2025-11-25T11:44:15Z

vms/evm/imports_test.go

+func findImportsMatchingPattern(
+	rootDir string,
+	importRegex *regexp.Regexp,
+	filterFunc func(filePath string, importPath string, importSpec *ast.ImportSpec) bool,


Suggested change

filterFunc func(filePath string, importPath string, importSpec *ast.ImportSpec) bool,

filterFunc func(filePath string, importPath string) bool,

It's never used.

ARR4N · 2025-11-25T11:53:48Z

vms/evm/imports_test.go

I've added comments based on the current macro-pattern, but that being said I think it's been overcomplicated by an unnecessary abstraction to support two tests. There's also no need to collect and then format the output when a t.Error() will do.

ARR4N · 2025-11-25T12:07:50Z

vms/evm/imports_test.go

+			strings.Contains(path, "graft/*/core/main_test.go") ||
+			strings.Contains(path, "graft/*/tempextrastest/") {


strings.Contains() doesn't support wild-card matching, but this is captured by the check below anyway.

ARR4N · 2025-11-25T12:11:00Z

vms/evm/imports_test.go

+	foundImports, err := findImportsMatchingPattern("..", graftRegex, func(path string, importPath string, _ *ast.ImportSpec) bool {
+		// Skip generated files and test-specific files
+		filename := filepath.Base(path)
+		if strings.HasPrefix(filename, "gen_") ||


https://pkg.go.dev/cmd/go#hdr-Generate_Go_files_by_processing_source:~:text=To%20convey%20to%20humans%20and%20machine%20tools%20that%20code%20is%20generated%2C%20generated%20source%20should%20have%20a%20line%20that%20matches%20the%20following%20regular%20expression%20(in%20Go%20syntax)%3A

That said, why should generated files be allowed to cross the import boundary?

ARR4N · 2025-11-25T12:25:19Z

I think the entirety of this PR can be reduced to something like this:

func TestImportViolations(t *testing.T) {
	const root ".."
	err := filepath.Walk(root, func(file string, info fs.FileInfo, err error) error {
		if err != nil {
			return err
		}
		if strings.ToLower(filepath.Ext(file)) != "go" {
			return nil
		}

		node, err := parser.ParseFile(token.NewFileSet(), file, nil, parser.ImportsOnly)
		if err != nil {
			return fmt.Errorf("parser.ParseFile(..., %q, ...): %v", file, err)
		}

		for _, spec := range node.Imports {
			if spec.Path == nil {
				continue
			}
			imp := strings.Trim(spec.Path.Value, `"`)

			inGraft := strings.Contains(file, "/graft/")
			inEmulate := strings.Contains(file, "/vms/evm/emulate/")
			inVMsEVM := strings.Contains(file, "/vms/evm/")
			importsPseudo := isPackageIn(imp, "github.com/ava-labs/libevm/libevm/pseudo")
			importsCoreth := isPackageIn(imp, "github.com/ava-labs/avalanchego/graft/coreth")
			importsSubnetEVM := isPackageIn(imp, "github.com/ava-labs/avalanchego/graft/subnet-evm")

			// TODO: before merging, this needs a comment explaining why each of them are a violation
			violations := []bool{
				importsPseudo,
				!inGraft && importsCoreth && inVMsEVM && !inEmulate,
				!inGraft && importsSubnetEVM && !inEmulate,
			}
			if slices.Contains(violations, true) {
				t.Errorf("File %q imports %q", file, imp)
			}
		}
		return nil
	})

	require.NoErrorf(t, err, "filepath.Walk(%q)", root)
}

func isPackageIn(importPath, root string) bool {
	if importPath == root {
		return true
	}
	if len(importPath) < len(root)+1 {
		return false
	}
	return strings.HasPrefix(importPath, root) && importPath[len(root)] == '/'
}

I'm not particularly happy with the relative path to walk nor the way of checking if something is in a sub-dir (they can be fixed together), but otherwise I think this covers all bases.

test: add import compliance tests

21b5aea

JonathanOppenheimer requested a review from alarso16 November 21, 2025 23:28

JonathanOppenheimer self-assigned this Nov 21, 2025

JonathanOppenheimer added the testing This primarily focuses on testing label Nov 21, 2025

github-project-automation bot added this to avalanchego Nov 21, 2025

JonathanOppenheimer added 3 commits November 21, 2025 18:35

test: tweak violation wording

7e33965

test: add violation to show failure example

404acd5

test: remove violation

dec282e

JonathanOppenheimer marked this pull request as ready for review November 21, 2025 23:37

JonathanOppenheimer requested review from joshua-kim and maru-ava as code owners November 21, 2025 23:37

JonathanOppenheimer commented Nov 21, 2025

View reviewed changes

tests/imports_test.go Outdated Show resolved Hide resolved

JonathanOppenheimer added 6 commits November 21, 2025 18:50

test: only enforce on vms/evm directory

234a039

test: clarify boundaries

9a39d6b

test: rename test

5017e1e

test: clarify comments

e490151

chore: lint

f411817

test: add license violation test

84fbb35

JonathanOppenheimer requested a review from StephenButtolph as a code owner November 22, 2025 00:50

JonathanOppenheimer added 3 commits November 21, 2025 20:10

test: inline once used type

498915c

tests: rename libevm test

f4fbfac

chore: fix typo

69e8354

JonathanOppenheimer commented Nov 22, 2025

View reviewed changes

graft/scripts/forbidden-geth-files.txt Outdated Show resolved Hide resolved

JonathanOppenheimer added 2 commits November 24, 2025 13:57

test: remove licensing test (excessive)

be5228a

Merge branch 'master' into JonathanOppenheimer/import-testing

d607b78

maru-ava reviewed Nov 24, 2025

View reviewed changes

test: check both evm code locations

99ed2ce

JonathanOppenheimer commented Nov 24, 2025

View reviewed changes

vms/evm/scripts/libevm-allowed-packages.txt Outdated Show resolved Hide resolved

test: move import tests to evm directory

e9cc48b

JonathanOppenheimer requested a review from ARR4N as a code owner November 24, 2025 19:15

JonathanOppenheimer requested review from maru-ava and removed request for maru-ava November 24, 2025 19:15

JonathanOppenheimer marked this pull request as draft November 24, 2025 20:13

test: reconsider boundary tests

cbcd663

JonathanOppenheimer added the monorepo label Nov 24, 2025

JonathanOppenheimer marked this pull request as ready for review November 24, 2025 23:06

maru-ava reviewed Nov 25, 2025

View reviewed changes

ARR4N requested changes Nov 25, 2025

View reviewed changes

github-project-automation bot moved this to In Progress 🏗️ in avalanchego Nov 25, 2025

ARR4N reviewed Nov 25, 2025

View reviewed changes

github-actions bot removed the monorepo label Nov 25, 2025

		@@ -0,0 +1,49 @@
		# Allowed libevm packages that can be imported in the graft directory.

		@@ -0,0 +1,232 @@
		// Copyright (C) 2019-2025, Ava Labs, Inc. All rights reserved.

		// - graft/coreth can be imported anywhere EXCEPT vms/evm (but vms/evm/emulate is an exception)
		// - graft/subnet-evm cannot be imported anywhere EXCEPT vms/evm/emulate

	func TestDoNotImportLibevmPseudo(t *testing.T) {
	func TestDoNotImportLibEVMPseudo(t *testing.T) {

	filterFunc func(filePath string, importPath string, importSpec *ast.ImportSpec) bool,
	filterFunc func(filePath string, importPath string) bool,

		strings.Contains(path, "graft/*/core/main_test.go") \|\|
		strings.Contains(path, "graft/*/tempextrastest/") {

test: add import compliance tests #4537

Are you sure you want to change the base?

test: add import compliance tests #4537

Uh oh!

Conversation

JonathanOppenheimer commented Nov 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why this should be merged

How this works

How this was tested

Need to be documented in RELEASES.md?

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ARR4N Nov 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ARR4N commented Nov 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

JonathanOppenheimer commented Nov 21, 2025 •

edited

Loading

ARR4N Nov 25, 2025 •

edited

Loading

ARR4N commented Nov 25, 2025 •

edited

Loading