Port `blob_to_kzg_commitment` tests to pytest by leolara · Pull Request #4541 · ethereum/consensus-specs

leolara · 2025-08-28T05:39:48Z

In this PR:

I port one of the 7 formats introduced yesterday to the framework we are using reducing potentially the number of repeated code (considering the boilerplate in kzg_4844_legacy.py that is implemented elsewhere).
Enable these tests to run in pytest so allow to future use of the pytest plugins from STEEL.
~~Dramatically reduce the start time of generated tests, but refactoring a test that was being executed always at collection phase.~~ This has been merged now in another PR

Note: this PR is based on the new caching system PR, to keep the non-multithread caching that was already implemented in kzg_4844_legacy in the refactored version. #4440

jtraglia · 2025-08-28T21:02:47Z

Hey Leo, yeah the proof of concept looks fine. Templates look nice. Just some random feedback:

These will be run with make test which isn't really what we wanted, but that's fine.
- I expect they will make CI checks considerably slower.
- You can disable these in the CI with the @generator_only decorator.
These are now under the kzg_4844 runner when they should be in kzg.
- See https://github.com/ethereum/consensus-spec-tests/tree/master/tests/general/deneb/kzg
These are no longer in the general directory like before:
- They are now in both the minimal & mainnet, which also means they're executed twice.
- Eg consensus-spec-tests/tests/{minimal,mainnet}/deneb/kzg_4844/blob_to_kzg_commitment/.
- Try running make reftests k=blob_to_kzg_commitment_case_valid_blob.
- I think you'll want to use the @single_phase decorator.
- But I'm not sure how to get it to be in the general directory.

PS: I merged a fix to the legacy tests which prevented other tests from working. See:

Do not run KZG compute challenge tests in the collection phase #4546

leolara · 2025-08-29T13:44:15Z

Hey Leo, yeah the proof of concept looks fine. Templates look nice. Just some random feedback:

These will be run with make test which isn't really what we wanted, but that's fine.

I expect they will make CI checks considerably slower.

You can disable these in the CI with the @generator_only decorator.

Will add a decorator that make the test run only in generation mode

These are now under the kzg_4844 runner when they should be in kzg.

ok

See https://github.com/ethereum/consensus-spec-tests/tree/master/tests/general/deneb/kzg

These are no longer in the general directory like before:

ok

They are now in both the minimal & mainnet, which also means they're executed twice.

Eg consensus-spec-tests/tests/{minimal,mainnet}/deneb/kzg_4844/blob_to_kzg_commitment/.

Try running make reftests k=blob_to_kzg_commitment_case_valid_blob.

I think you'll want to use the @single_phase decorator.

But I'm not sure how to get it to be in the general directory.

Ok, should run only in minimal or only in mainnet?

PS: I merged a fix to the legacy tests which prevented other tests from working. See:

Do not run KZG compute challenge tests in the collection phase #4546

Ok, will include this

jtraglia · 2025-08-29T15:24:28Z

Ok, should run only in minimal or only in mainnet?

It will produce the same values for both, but technically it should exist in the general directory.

By making the test run at test time instead of collection time in SSZ generic container. This was first introduced here: #4541 but that PR has more complications with other things

leolara · 2025-09-11T10:23:01Z

PS: I merged a fix to the legacy tests which prevented other tests from working. See:

#4546

I checked here that kzg_4844.py in master has the same content that here kzg_4844_legacy.py in this branch, so it should be included.

leolara · 2025-09-11T13:03:42Z

@jtraglia please check again

jtraglia · 2025-09-11T14:07:44Z

tests/core/pyspec/eth2spec/test/deneb/kzg/test_blob_to_kzg_commitment.py

+    blob = VALID_BLOBS[index]
+
+    @manifest(preset_name="general", suite_name="kzg-mainnet")
+    @only_generator("randomized test for broad coverage, not point-to-point CI")


Description isn't really accurate or helpful. It's not randomized and what is "point-to-point" CI exactly?

I'd prefer no rational sentence but if it's necessary maybe something like this instead? For both instances of this.

Suggested change

@only_generator("randomized test for broad coverage, not point-to-point CI")

@only_generator("too slow")

@leolara this suggestion applies to multiple lines:

For both instances of this.

jtraglia · 2025-09-11T14:08:06Z

tests/core/pyspec/eth2spec/test/deneb/kzg/test_blob_to_kzg_commitment.py

+    @spec_cache(["blob_to_kzg_commitment"])
+    def the_test(spec):
+        commitment = spec.blob_to_kzg_commitment(blob)
+        # assert exception is not thrown with valid blob


Let's remove the useless comment.

jtraglia · 2025-09-11T14:08:16Z

tests/core/pyspec/eth2spec/test/deneb/kzg/test_blob_to_kzg_commitment.py

+        except Exception:
+            pass
+
+        # exception is thrown


Let's remove the useless comment.

It is not useless, it is a justtification to assert False

I mean to assert commitment is None

jtraglia · 2025-09-11T15:40:42Z

tests/infra/spec_cache.py

Please remove the new spec caching stuff. This is irrelevant to this PR & I'm not ready to approve that stuff yet.

But, you added non-multithreading caching to this?

You mean like the cached_blob_to_kzg_commitment wrapper function? Sure but that was way simpler. Not looking to start an argument, just stating that I'm not ready to review/merge that part yet. This PR should focus on converting the tests to pytest, not adding new features. Please remove it.

This PR doesn't need to include any caching. It doesn't matter to me if don't add back the cached_blob_to_kzg_commitment function. That's not really important.

Sorry, I don't understand.

I am wrong stipulating that this is porting a test that in the orginal was caching spec.blob_to_kzg_commitment with cached_blob_to_kzg_commitment and now it is caching it with @spec_cache(["blob_to_kzg_commitment"])

Yes, so this PR is making a rather large additional change; it's doing more than just porting some test to pytest. @leolara I don't understand why you're pushing back so hard on this. I'm just asking for you to split the spec_cache part out of this PR. I will not approve this PR until then.

Co-authored-by: Justin Traglia <95511699+jtraglia@users.noreply.github.com>

leolara · 2025-09-19T10:36:25Z

The command diff -rq consensus-spec-tests/tests consensus-spec-tests/tests-master/ returned nothing, which means that there are no differences.

leolara · 2025-09-19T12:04:55Z

@jtraglia please check again, I used diff -rq to compare, meld has many dependencies.

jtraglia

LGTM, thanks @leolara

I can confirm these produce the same reference tests:

leolara requested a review from ralexstokes August 28, 2025 05:43

leolara marked this pull request as draft September 11, 2025 09:14

leolara marked this pull request as ready for review September 11, 2025 10:46

leolara changed the title ~~Several refactorings to testing~~ Port KZG 4844 blob_to_kzg_commitment tests to pytest Sep 11, 2025

jtraglia reviewed Sep 11, 2025

View reviewed changes

leolara and others added 7 commits September 19, 2025 09:13

Several refactorings to testing

6430930

Merge important fix to legacy tests

c4d7888

Fix vectors directories

1bc62a4

Only generator enabled in KZG tests

c600e07

lint

58e4af3

Apply suggestion from @jtraglia

5ca697f

Co-authored-by: Justin Traglia <95511699+jtraglia@users.noreply.github.com>

lint

53b3d50

leolara force-pushed the leolara/several-refactoring-testing branch from 5363c1e to 53b3d50 Compare September 19, 2025 09:26

Remove caching

5843208

Remove comment

56d6120

jtraglia approved these changes Sep 19, 2025

View reviewed changes

jtraglia changed the title ~~Port KZG 4844 blob_to_kzg_commitment tests to pytest~~ Port blob_to_kzg_commitment tests to pytest Sep 19, 2025

jtraglia added the testing CI, actions, tests, testing infra label Sep 19, 2025

jtraglia merged commit 1a1cc25 into ethereum:master Sep 19, 2025
15 checks passed

danceratopz mentioned this pull request Jan 15, 2026

Add Leo from STEEL, ethereum/consensus-specs protocolguild/documentation#471

Merged

	@only_generator("randomized test for broad coverage, not point-to-point CI")
	@only_generator("too slow")

Comments

Conversation

leolara commented Aug 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jtraglia commented Aug 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

leolara commented Aug 29, 2025

Uh oh!

jtraglia commented Aug 29, 2025

Uh oh!

leolara commented Sep 11, 2025

Uh oh!

leolara commented Sep 11, 2025

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!

This comment was marked as spam.

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!

leolara commented Sep 19, 2025

Uh oh!

leolara commented Sep 19, 2025

Uh oh!

jtraglia left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

leolara commented Aug 28, 2025 •

edited

Loading

jtraglia commented Aug 28, 2025 •

edited

Loading