Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

build: tests: add execution spec tests #26985

Merged
merged 9 commits into from
Aug 26, 2023

Conversation

MariusVanDerWijden
Copy link
Member

This PR adds the execution spec tests to our test suite.
Doesn't work under windows yet

build/ci.go Outdated Show resolved Hide resolved
@holiman
Copy link
Contributor

holiman commented Mar 30, 2023

This PR

ok  	github.com/ethereum/go-ethereum/tests	314.167s

A random other PR

ok  	github.com/ethereum/go-ethereum/tests	322.125s

(numbers taken from Appveyor ubuntu run)

Are you sure it is actually running the tests? Or are there just not a lot of them?

@MariusVanDerWijden
Copy link
Member Author

There's not a lot of them right now. I think on the order of 50 or so

build/ci.go Outdated
Comment on lines 332 to 351
csdb := build.MustLoadChecksums("build/checksums.txt")
ext := ".tar.gz"
base := "fixtures" // TODO(MariusVanDerWijden) rename once the version becomes part of the filename
url := fmt.Sprintf("https://github.com/ethereum/execution-spec-tests/releases/download/v%s/%s%s", version, base, ext)
archivePath := filepath.Join(cachedir, base+ext)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you take the version as a parameter, e.g. go run ./build/ci.go -test --test.spec "0.2.4". Then we don't have to muck around with making version part of the filename. In order to update the spec, we just bump the version and update the hash.

And then this PR would be good to go, without any TODO's left..(?)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I guess it is already good to go. TO update for a new release of the tests, we have to update the const version string and the hash. Doing it the way I proposed doesn't simplify anything

build/ci.go Outdated
coverage = flag.Bool("coverage", false, "Whether to record code coverage")
verbose = flag.Bool("v", false, "Whether to log verbosely")
race = flag.Bool("race", false, "Execute the race detector")
stateTestDir = "tests/spec-tests"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any reason for definining it here? If you move it into downloadSpecTestFixtures, you will have all related to spec-tests collected there, which might be nicer?

Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but maybe would prefer moving cachedir so it's

const(
    version = ... 
    cachedir = ""
)

@fjl
Copy link
Contributor

fjl commented May 2, 2023

It would be nice to have a script/tool like build/update-tests.go that

  • Downloads the latest tests release from the github repo
  • Updates the checksum file
  • Updates the version somewhere

@fjl fjl added this to the 1.13.0 milestone Aug 26, 2023
@fjl fjl merged commit f174ddb into ethereum:master Aug 26, 2023
1 check passed
devopsbo3 pushed a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
This makes it possible to run the execution-spec-tests (a.k.a. pyspec) in CI.

---------

Co-authored-by: Felix Lange <[email protected]>
devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants