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

Fix electra sanity testgen for blocks #3939

Merged
merged 2 commits into from
Sep 24, 2024

Conversation

jtraglia
Copy link
Member

@jtraglia jtraglia commented Sep 23, 2024

@tersec pointed out on Discord that tests still passed without the new process_ops calls.

If I leave off from [modified process_operations lines], all the EF consensus spec alpha.6 tests Nimbus runs pass. Are there tests which are supposed to detect this?

This is because we forgot to add from .test_blocks import * to __init__.py here:

from .test_deposit_transition import * # noqa: F401 F403

I've added this import and a comment to remind devs to update the __init__.py file in the future.

@hwwhww
Copy link
Contributor

hwwhww commented Sep 24, 2024

Thank you!

test_deposit_transition.py file is an extension of test_blocky.py. I think it was a nice categorization that helped the readability of these sanity block tests.

The sanity/block/__init__.py change was a hack to allow the test_deposit_transition file to reuse the sanity/block test format without editing the test format documents. If we use ../electra/sanity/deposit_transition/pyspec_tests/deposit_transition__* as the new file path, clients may need to add the new path manually.

Perhaps we can maintain the folder structure used in this PR and apply some adjustments in the sanity/main.py so that clients won't see the path change? 🤔

@jtraglia
Copy link
Member Author

The sanity/block/__init__.py change was a hack to allow the test_deposit_transition file to reuse the sanity/block test format without editing the test format documents. If we use ../electra/sanity/deposit_transition/pyspec_tests/deposit_transition__* as the new file path, clients may need to add the new path manually.

Oh I see. Hmm yeah that makes sense.

Perhaps we can maintain the folder structure used in this PR and apply some adjustments in the sanity/main.py so that clients won't see the path change? 🤔

Yes, let me try this. We could also just add the import to __init__.py.

@jtraglia jtraglia force-pushed the fix-electra-sanity-testgen branch from eaeb096 to 0edaccd Compare September 24, 2024 16:17
Copy link
Contributor

@hwwhww hwwhww left a comment

Choose a reason for hiding this comment

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

LGTM!

Co-authored-by: Hsiao-Wei Wang <[email protected]>
@jtraglia jtraglia merged commit dcdcf25 into ethereum:dev Sep 24, 2024
26 checks passed
@jtraglia jtraglia deleted the fix-electra-sanity-testgen branch September 24, 2024 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants