Skip to content

Disallow non-default expectedExecutionsPerDeployment in evmasm tests when only a creation object is present #16023

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 1 commit into
base: subassemblies-in-evm-assembly-tests
Choose a base branch
from

Conversation

cameel
Copy link
Member

@cameel cameel commented Apr 25, 2025

Depends on #16021.

This adds an extra check to prevent confusion like in #16012 (comment). expectedExecutionsPerDeployment only affects runtime assemblies, but the top-level assembly is a creation one so if you have only one, it will seem like the setting is not working. This PR makes the test case throw an exception in that situation to make it obvious. It is not likely to be intentional.

I did not include it in #16021, because I'm not yet 100% convinced we want this. There are some downsides:

  • It would be better off as a warning, but soltest does not have support for that. I can only throw.
    • I guess I could just print to stdout...
  • This error means that the JSON import cannot be tested in full generality. This corner case will trigger an error while --import-asm-json would just accept it.
  • It does not detect when the setting is specified in the test, only when it has a non-default value.

Still, these seem minor enough that we're probably better off with this check than without it.

@cameel cameel added testing 🔨 has dependencies The PR depends on other PRs that must be merged first labels Apr 25, 2025
@cameel cameel self-assigned this Apr 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has dependencies The PR depends on other PRs that must be merged first testing 🔨
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant