Skip to content

feat(static tests) : do not compile test fillers source code while loading the models #1439

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

winsvega
Copy link
Contributor

@winsvega winsvega commented Apr 11, 2025

@marioevz please don't merge there are bullet points to complete:

  • When reading Filler.json into pydantic do not compile test source right away. this speeds up test generation
  • Support lllc container docker image, so no need to have lllc compiled locally. can import from docker hub: winsvega/lllc:latest
  • After all tests generated or on exception fail stop, need to call stop_lllc_containers() function

- [x] Make sure exceptions are working correctly after mapper refactoring (too many things to fix) #1440

the test generation is supported with lllc from docker hub.
https://hub.docker.com/r/winsvega/lllc

it works fast using docker exec. all we need is to pull a docker image in our CI

@winsvega winsvega force-pushed the fill_static_tests branch from 07588f0 to 65e4eee Compare April 11, 2025 09:19
@winsvega winsvega force-pushed the fill_static_tests branch from 65e4eee to 8a295d3 Compare April 11, 2025 13:34
@winsvega winsvega requested a review from marioevz April 11, 2025 13:37
@winsvega winsvega changed the title do not compile test fillers source code while loading the models feat(static tests) : do not compile test fillers source code while loading the models Apr 11, 2025
@spencer-tb spencer-tb added type:feat type: Feature scope:fill Scope: fill command labels Apr 14, 2025
 Each legacy test filler has only 1 test per file if it's a !state test!
 So no need to create directory Add11/add11.json it can be plain add11.json
@winsvega
Copy link
Contributor Author

restored this logic
a092ecb

@winsvega
Copy link
Contributor Author

winsvega commented Apr 14, 2025

@marioevz

execution-spec-tests/src/ethereum_test_specs/static_state/common/docker.py
def stop_lllc_containers():

needs to be called after filling of the fixtures has finished. or in case of exception or keyboard interrupt and we are exiting.
where this part of logic is defined in pytest plugin of static test filler?

Copy link
Member

@marioevz marioevz left a comment

Choose a reason for hiding this comment

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

Looks great, just a few comments, let me know what you think about the docker alternative.

if raw_index != -1:
compiled_code = self.code_raw[raw_index + len(raw_marker) :]

# Prase :yul
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# Prase :yul
# Parse :yul

if not isinstance(self.code_raw, str):
raise ValueError(f"parse_code(code: str) code is not string: {self.code_raw}")
if len(self.code_raw) == 0:
return bytes.fromhex("")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return bytes.fromhex("")
return b""

Tiny improvement.

Comment on lines +152 to +153
# using lllc
# result = subprocess.run(["lllc", tmp_path], capture_output=True, text=True)
Copy link
Member

Choose a reason for hiding this comment

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

I think it should be ok to just require this to be installed already.

At the moment, we prepare releases and fill tests using out external runners, and all of our current external runners point to our server that has already lllc installed, so it should just work.

Downside would be that we would be unable to add unit tests for state static tests that contain lllc, but I think that should be ok.

yul_marker = ":yul"
yul_index = self.code_raw.find(yul_marker)

# Prase :raw
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# Prase :raw
# Parse :raw

optimize=options[1] if len(options) >= 2 else None,
)[2:]

# Prase :abi
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# Prase :abi
# Parse :abi

elif self.code_raw.lstrip().startswith("0x"):
compiled_code = self.code_raw[2:].lower()

# Prase lllc code
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# Prase lllc code
# Parse lllc code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope:fill Scope: fill command type:feat type: Feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants