Separate tests between spec tests and infra tests AND change framework for test_template_test #4538
Conversation
Makefile
Outdated
| # Run test framework tests. | ||
| # | ||
| # To run a specific test, append k=<test>, eg: | ||
| # make test k=test_verify_kzg_proof | ||
| # To run tests with a specific bls library, append bls=<bls>, eg: | ||
| # make test bls=arkworks | ||
| test_infra: MAYBE_TEST := $(if $(k),-k=$(k)) | ||
| # Disable parallelism which running a specific test. | ||
| # Parallelism makes debugging difficult (print doesn't work). | ||
| test_infra: MAYBE_PARALLEL := $(if $(k),,-n auto) | ||
| test_infra: pyspec | ||
| @mkdir -p $(TEST_REPORT_DIR) | ||
| @$(PYTHON_VENV) -m pytest \ | ||
| $(MAYBE_PARALLEL) \ | ||
| --capture=no \ | ||
| $(MAYBE_TEST) \ | ||
| $(CURDIR)/tests/infra |
There was a problem hiding this comment.
I want to keep these tests bundled together. make test should run all tests.
There was a problem hiding this comment.
Also, after deleting this, can we fix the typo on line 101:
Disable parallelism which running a specific test.
Should be:
Disable parallelism when running a specific test.
There was a problem hiding this comment.
I want to keep these tests bundled together. make test should run all tests.
I think going forward, as more framework tests get added, there would be value in allowing separate execution of these tests:
- It creates a clearer separation of intent that makes it immediately clear to a new developer that these "framework tests" have an entirely different purpose.
- It allows a developer to iterate faster on framework changes that are covered by fw tests (typically fw tests will run faster than the actual tests).
- It allows better reporting, locally, particularly in CI; consensus-specs could have two workflows that would run
make testsandmake test_infra.
For now, until we improve the CI as in 2., we could add this new test_infra target as a dependency to the test target. This already enables 1. and 2. We can improve the CI in a follow-up PR for 3. and potentially remove the test_infra dep from test.
There was a problem hiding this comment.
I understand, but there are better ways of doing this. This solution introduces unnecessary complexity for everyone: (1) we'll need to update documentation telling contributors that they need to run both commands; (2) we need to update the CI checks to run both commands; (3) we'll need to update the release action to run these; (4) we need to update the Makefile help section.
There are two other solutions which I would prefer:
- Ensure infrastructure tests start with
def test_infra_*and usemake test k=test_infra. - Similar to the current style, make the list of files dynamic and use
make test infra-only=true.
There was a problem hiding this comment.
I added a TODO list.
Additionally to what @danceratopz said I don't think the normal spec test developer will not need to run the infra tests, so another advantage of having them separated.
There was a problem hiding this comment.
No that's not what I'm saying. Seriously, let's not add a separate make command for infrastructure tests. Please.
There was a problem hiding this comment.
I pushed ec3edc0 which does it how I think it should be done. Just run make test fw=true.
There was a problem hiding this comment.
After thinking about it some more, I've come to this solution:
make test(tests everything)make test component=fw(runs only framework tests)make test component=pyspec(runs only pyspec tests)
Are we okay with this? Does it do what you want it to?
|
Hey Leo, can we give this PR a better title? Something less ambiguous but still <= 50 chars. |
Makefile
Outdated
| # Run test framework tests. | ||
| # | ||
| # To run a specific test, append k=<test>, eg: | ||
| # make test k=test_verify_kzg_proof | ||
| # To run tests with a specific bls library, append bls=<bls>, eg: | ||
| # make test bls=arkworks | ||
| test_infra: MAYBE_TEST := $(if $(k),-k=$(k)) | ||
| # Disable parallelism which running a specific test. | ||
| # Parallelism makes debugging difficult (print doesn't work). | ||
| test_infra: MAYBE_PARALLEL := $(if $(k),,-n auto) | ||
| test_infra: pyspec | ||
| @mkdir -p $(TEST_REPORT_DIR) | ||
| @$(PYTHON_VENV) -m pytest \ | ||
| $(MAYBE_PARALLEL) \ | ||
| --capture=no \ | ||
| $(MAYBE_TEST) \ | ||
| $(CURDIR)/tests/infra |
There was a problem hiding this comment.
I want to keep these tests bundled together. make test should run all tests.
I think going forward, as more framework tests get added, there would be value in allowing separate execution of these tests:
- It creates a clearer separation of intent that makes it immediately clear to a new developer that these "framework tests" have an entirely different purpose.
- It allows a developer to iterate faster on framework changes that are covered by fw tests (typically fw tests will run faster than the actual tests).
- It allows better reporting, locally, particularly in CI; consensus-specs could have two workflows that would run
make testsandmake test_infra.
For now, until we improve the CI as in 2., we could add this new test_infra target as a dependency to the test target. This already enables 1. and 2. We can improve the CI in a follow-up PR for 3. and potentially remove the test_infra dep from test.
leolara
changed the title
Co-authored-by: danceratopz <[email protected]>
Co-authored-by: danceratopz <[email protected]>
|
If you're okay with my change, we can merge this after the broken test is fixed. Also, please update the title & be sure that it's not crazy long.
|
Co-authored-by: Leo Lara <[email protected]>
These are a set of small improvements suggested by @danceratopz
TODO: