refactor: use generics for consensus test specs testing to reduce code duplication #1621

KolbyML · 2025-01-07T16:19:55Z

What was wrong?

Currently every fork we must add thousands of lines of test data from ethereum/consensus-spec-tests and copy pastes tons of tests

This bloats our repo with large files, but also creates a bunch of almost duplicate tests.

This method also makes our test coverage limited as adding tests taking a lot of work to add and maintain so we often avoided adding additional test cases

This makes it very annoying and tedious to write tests for future forks

How was it fixed?

Creating a ef-testing crate which runs tests on data mostly generically, but manually for some cases.

we add a Makefile to download the data from ethereum/consensus-spec-tests
use generics to reduce code duplication and makes it easier to test future forks

These tests will also run in the CI which is good

Lighthouse and Reth use similar strategies for dealing with EF test data

In short

test coverage and maintainability is now highly improved
adding future tests and supporting new forks is easier

Future work

This is a big improvement, but of course we can always still improve. Anyways I want to move ethportal-peertest to the testing folder, and probably the test folder into it as well. What do you guys think?

I also want to move utp-testing into the testing folder

…e duplication

njgheorghita · 2025-01-08T17:48:06Z

testing/ef-tests/README.md

+## Run [ethereum/consensus-spec-tests](https://github.com/ethereum/consensus-spec-tests)
+
+
+Run Tests this will automatically download test data


IIUC, this will also run the tests? This kind of sounds like it will only download the test data...

njgheorghita · 2025-01-09T14:49:48Z

testing/ef-tests/README.md

+Clean test files
+```bash
+make clean
+```


In general, I think this README could use a fair amount more useful information....

outline of the crate structure

what kinds of datatypes are being tested?

how to add tests in future forks & make sure that they're tested

any other info you think would be useful to someone who just looks at this crate, and doesn't want to go digging through the code to understand what's going on

Thank you for the feedback I will update the readme 🫡

KolbyML · 2025-01-09T15:49:45Z

@njgheorghita ready for another look, I tried to address the feedback, let me know if there is anything else you think would need improvement

ogenev

I think it will be better if we have a root Makefile, instead of always going inside testing/ef-test if we want to download the test data.

ogenev · 2025-01-10T09:58:00Z

testing/ef-tests/README.md

+
+In the future we may also test `ethereum/execution-test-specs`
+
+## how to add tests in future forks & make sure that they're tested


nit:

Suggested change

## how to add tests in future forks & make sure that they're tested

## How to add tests in future forks & make sure that they're tested

ogenev · 2025-01-10T09:59:19Z

testing/ef-tests/README.md

@@ -0,0 +1,46 @@
+## Run [ethereum/consensus-spec-tests](https://github.com/ethereum/consensus-spec-tests)
+
+`ethereum/consensus-spec-tests` test data is important for testing our types, but the test data is very large which makes it not feasible to host it in our repository, this has lead to us under testing new types when new forks ship, and adding thousands of lines of test data and copy pasting almost identical tests with a few variables renamed.


Suggested change

`ethereum/consensus-spec-tests` test data is important for testing our types, but the test data is very large which makes it not feasible to host it in our repository, this has lead to us under testing new types when new forks ship, and adding thousands of lines of test data and copy pasting almost identical tests with a few variables renamed.

`ethereum/consensus-spec-tests` test data is important for testing our types, but the test data is very large which makes it not feasible to host it in our repository, this has led to us under testing new types when new forks ship, and adding thousands of lines of test data and copy pasting almost identical tests with a few variables renamed.

ogenev · 2025-01-10T09:59:47Z

testing/ef-tests/README.md

+### tests
+All the tests the `ef-tests` crates run will be located in the `tests` folder
+
+## what kinds of data types are being tested?


nit:

Suggested change

## what kinds of data types are being tested?

## What kinds of data types are being tested?

ogenev · 2025-01-10T10:31:55Z

testing/ef-tests/Makefile

I'm more in favor of having a Makefile in the root dir. I would like, for example, to run a make test from the root to do all data downloads and all testing for me locally. We can have different commands for more granular testing, something similar to what reth are doing here and here.

KolbyML · 2025-01-10T12:05:11Z

@ogenev ready for another review, I believe I addressed all your concerns, let me know if I missed something or anything

morph-dev · 2025-01-11T18:54:21Z

I didn't take a closer look, but I wanted to ask why use Makefile and downloading test files instead of using submodule (like we do for portal-spec-tests)? Am I missing something?

In general, I'm in favor of this PR.

KolbyML · 2025-01-11T18:56:40Z

I didn't take a closer look, but I wanted to ask why use Makefile and downloading test files instead of using submodule (like we do for portal-spec-tests)? Am I missing something?

In general, I'm in favor of this PR.

Both lighthouse and reth download the test files using makefiles.

The test data is over 500MB's and uses git lfs, so I think using a submodule would make it less friendly to clone our project or run tests.

Doing it this way git cloning is still fast, and the annoying thing of git submodules changing branches is avoided.

ogenev · 2025-01-13T10:55:32Z

Makefile

 ##@ Other

 .PHONY: clean
 clean: ## Perform a `cargo` clean and remove the binary and test vectors directories.
 	cargo clean
 	rm -rf $(BIN_DIR)
-	rm -rf $(EF_TESTS_DIR)
+	rm -rf $(EF_TESTS_DIR)


nit: missed new line in EOF

ogenev · 2025-01-13T11:01:45Z

Makefile

+	@echo "Tests complete."
+
+.PHONY: test
+test: ## Runs workspace tests.


What do you think about having a make command like test-workspace (or test-trin?), where we will run cargo test --workspace -- --nocapture and then having make test to run all tests (ef-tests + test-workspace)?

I wouldn't want make test to run ef-tests as it downloads files

neither lighthouse or reth uses make test to run ef-tests

Lighthouse puts ef-tests behind make test-full, I will do that as then it is more intentional you want to run the expanded test suite, which would involve downloading files.

Both lighthouse and reth use make test for testing the workspace

ok fair enough 👍 .

ogenev

⛵

KolbyML force-pushed the improve-consensus-testing branch 2 times, most recently from 2f5e6a1 to b521e77 Compare January 7, 2025 17:47

KolbyML marked this pull request as ready for review January 7, 2025 17:49

KolbyML requested review from carver, njgheorghita, ogenev and morph-dev January 7, 2025 17:49

KolbyML self-assigned this Jan 7, 2025

refactor: use generics for consensus test specs testing to reduce cod…

d745c63

…e duplication

KolbyML force-pushed the improve-consensus-testing branch from b521e77 to d745c63 Compare January 7, 2025 17:57

KolbyML mentioned this pull request Jan 7, 2025

refactor: move crates into crates folder and remove redundant prefixes #1624

Merged

njgheorghita reviewed Jan 9, 2025

View reviewed changes

KolbyML added 2 commits January 9, 2025 08:33

fix: make readme more clear

1ab67bd

fix: remove unneeded type

4721285

KolbyML requested a review from njgheorghita January 9, 2025 15:49

ogenev reviewed Jan 10, 2025

View reviewed changes

fix: address feedback

ba10b07

ogenev reviewed Jan 13, 2025

View reviewed changes

ogenev approved these changes Jan 13, 2025

View reviewed changes

fix: address PR feedback

32910f1

KolbyML merged commit 4323283 into ethereum:master Jan 13, 2025
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: use generics for consensus test specs testing to reduce code duplication #1621

refactor: use generics for consensus test specs testing to reduce code duplication #1621

KolbyML commented Jan 7, 2025 •

edited

Loading

njgheorghita Jan 8, 2025

njgheorghita Jan 9, 2025

KolbyML Jan 9, 2025

KolbyML commented Jan 9, 2025

ogenev left a comment

ogenev Jan 10, 2025

ogenev Jan 10, 2025

ogenev Jan 10, 2025

ogenev Jan 10, 2025

KolbyML commented Jan 10, 2025

morph-dev commented Jan 11, 2025

KolbyML commented Jan 11, 2025

ogenev Jan 13, 2025 •

edited

Loading

ogenev Jan 13, 2025

KolbyML Jan 13, 2025

ogenev Jan 13, 2025

ogenev left a comment

		## Run [ethereum/consensus-spec-tests](https://github.com/ethereum/consensus-spec-tests)


		Run Tests this will automatically download test data


		In the future we may also test `ethereum/execution-test-specs`

		## how to add tests in future forks & make sure that they're tested

		@@ -0,0 +1,46 @@
		## Run [ethereum/consensus-spec-tests](https://github.com/ethereum/consensus-spec-tests)

		`ethereum/consensus-spec-tests` test data is important for testing our types, but the test data is very large which makes it not feasible to host it in our repository, this has lead to us under testing new types when new forks ship, and adding thousands of lines of test data and copy pasting almost identical tests with a few variables renamed.

	## what kinds of data types are being tested?
	## What kinds of data types are being tested?

refactor: use generics for consensus test specs testing to reduce code duplication #1621

refactor: use generics for consensus test specs testing to reduce code duplication #1621

Conversation

KolbyML commented Jan 7, 2025 • edited Loading

What was wrong?

How was it fixed?

Future work

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KolbyML commented Jan 9, 2025

ogenev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KolbyML commented Jan 10, 2025

morph-dev commented Jan 11, 2025

KolbyML commented Jan 11, 2025

ogenev Jan 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ogenev left a comment

Choose a reason for hiding this comment

KolbyML commented Jan 7, 2025 •

edited

Loading

ogenev Jan 13, 2025 •

edited

Loading