fix(consume): consume_direct.sh files contained invalid commands because value after --filter flag was not enclosed in parenthesis (issue reported by flcl42)

[felix314159]

🗒️ Description

Title says it all, simple fix. Context: #1843 , more specifically this comment by flcl

What is fixed?

Before this PR, if you run sth like
uv run consume direct --bin=nethtest --input=stable@latest --traces --dump-dir=evm-tmp -n 8 -k push0
and then try to run a consume_direct.sh file like
./push0_contracts/tests-shanghai-eip3855_push0-test_push0.py::test_push0_contracts[fork_Shanghai-blockchain_test_from_state_test-gas_cost]/consume_direct.sh
you will see sth like:

Unhandled exception: System.Text.RegularExpressions.RegexParseException: Invalid pattern '^(tests/shanghai/eip3855_push0/test_push0.py::test_push0_contracts[fork_Shanghai-blockchain_test_from_state_test-gas_cost])' at offset 82. [x-y] range in reverse order.
   at System.Text.RegularExpressions.RegexParser.ScanCharClass(Boolean caseInsensitive, Boolean scanOnly)
   at System.Text.RegularExpressions.RegexParser.ScanRegex()
   at System.Text.RegularExpressions.RegexParser.Parse(String pattern, RegexOptions options, CultureInfo culture)
   at System.Text.RegularExpressions.Regex..ctor(String pattern, CultureInfo culture)
   at System.Text.RegularExpressions.RegexCache.GetOrAdd(String pattern)
   at System.Text.RegularExpressions.Regex.Match(String input, String pattern)
   at Nethermind.Test.Runner.BlockchainTestsRunner.RunTestsAsync() in /home/user/Documents/nethermind/src/Nethermind/Nethermind.Test.Runner/BlockchainTestsRunner.cs:line 35
   at Nethermind.Test.Runner.Program.RunBlockTest(String path, Func`2 testRunnerBuilder) in /home/user/Documents/nethermind/src/Nethermind/Nethermind.Test.Runner/Program.cs:line 115
   at Nethermind.Test.Runner.Program.Run(ParseResult parseResult, CancellationToken cancellationToken) in /home/user/Documents/nethermind/src/Nethermind/Nethermind.Test.Runner/Program.cs:line 90
   at System.CommandLine.Invocation.InvocationPipeline.InvokeAsync(ParseResult parseResult, CancellationToken cancellationToken)

This PR fixes this, after applying it the command works as expected (e.g.):

tests/shanghai/eip3855_push0/test_push0.py::test_push0_contracts[fork_Shanghai-blockchain_test_from_state_test-gas_cost] PASS

Both Nethtest and geth's evm were leading to faulty sh files and this PR fixes both.

🔗 Related Issues or PRs

N/A.

✅ Checklist

• All: Ran fast tox checks to avoid unnecessary CI fails, see also Code Standards and Enabling Pre-commit Checks:

  uvx --with=tox-uv tox -e lint,typecheck,spellcheck,markdownlint

• All: PR title adheres to the repo standard - it will be used as the squash commit message and should start type(scope):.
• All: Considered adding an entry to CHANGELOG.md.
• All: Considered updating the online docs in the ./docs/ directory.
• All: Set appropriate labels for the changes (only maintainers can apply labels).
• Tests: Ran mkdocs serve locally and verified the auto-generated docs for new tests in the Test Case Reference are correctly formatted.
• Tests: For PRs implementing a missed test case, update the post-mortem document to add an entry the list.
• Ported Tests: All converted JSON/YML tests from ethereum/tests or tests/static have been assigned @ported_from marker.

[marioevz]

Suggestion on a change to the approach, thanks!

[marioevz]

Suggested change

	# ensure that the --filter flag value is wrapped in parentheses
	# ensure that the --filter flag value is wrapped in double-quotes

[marioevz]

Afaik, all parameters that contain spaces need to be enclosed in double-quotes, not only the ones after filter, and not only for the nethermind commands.

Should we look into pre-processing the command parameter before it's passed to this function?

[marioevz]

A quick gpt-query yields that we should use shlex.quote on every parameter to handle this automatically:

import shlex

args = ["my command", "--option", "value with spaces", "file$(rm -rf /)"]

command = " ".join(shlex.quote(arg) for arg in args)
print(command)

Output:

'my command' --option 'value with spaces' 'file$(rm -rf /)'

And shlex is included in the default libraries so no new package dependencies.

[felix314159]

I have not heard of shlex before, so I think it will be harder to read the code if someone sees shlex.quote() because chances are the reader is not familar with that function. But the current solution is just a few lines of trivial code. I also saw in shlex docs that that function seems to be incompatible with windows, I know that windows support itsn't a prio but why make potential future support harder for no reason.

We could add pre-processing to command but we need separate logic for fixing the sh command produced by geth's evm anyway, so its simpler to have separate fixes for nethtest and geth and then decide to not add support for more execution clients. For geth's evm specifically we create faulty .sh files for blockchain tests, I will add another commit to fix it too. All problems are just results from our decision to allow weird chars like :, [ and ] in filenames which makes escaping of names ugly and error-prone.

[felix314159]

Working on this takes me longer than expected because I can't get logging to work for some reason. Edit: found temporary workaround

[marioevz]

from our decision to allow weird chars like :, [ and ] in filenames

Just for clarification, they are part of the test names, not file names, this is a pytest intrinsic issue.

I also saw in shlex docs that that function seems to be incompatible with windows

This will end up in a .sh file anyway, which needs bash to run. If required it we could build a windows script in the future but I feel like that's out of scope for this PR.

I think it will be harder to read the code if someone sees shlex.quote() because chances are the reader is not familar with that function.

That's true, I haven't heard of it before but (a) this is deep into the weeds of the code anyway, I would be more concerned if this was part of a test, and (b) it's really easy to look it up.

[felix314159]

Yeah good point with the .sh lol.. I would not be against shlex if I didn't already build a solution. Can re-do this PR and start from scratch (and use shlex) if you want

[felix314159]

I have now figured out what we must change to also fix geth's evm command in the sh files. There actually are 2 issues with blockchain tests:

• value after --run flag has to be put in quotations
• last part of the command needs to have \ escapes added before brackets [ and ]

So e.g. currently in the .sh file you would see sth like:

#!/bin/bash

/home/user/Documents/go-ethereum/build/bin/evm --verbosity 100 blocktest --trace --run tests/shanghai/eip3855_push0/test_push0\.py::test_push0_contracts\[fork_Shanghai\-blockchain_test_from_state_test\-before_jumpdest\] evm-tmp/push0_contracts/tests-shanghai-eip3855_push0-test_push0.py::test_push0_contracts[fork_Shanghai-blockchain_test_from_state_test-before_jumpdest]/fixtures.json

when actually the command should be

/home/user/Documents/go-ethereum/build/bin/evm --verbosity 100 blocktest --trace --run 'tests/shanghai/eip3855_push0/test_push0\.py::test_push0_contracts\[fork_Shanghai\-blockchain_test_from_state_test\-before_jumpdest\]' evm-tmp/push0_contracts/tests-shanghai-eip3855_push0-test_push0.py::test_push0_contracts\[fork_Shanghai-blockchain_test_from_state_test-before_jumpdest\]/fixtures.json

[felix314159]

Can be merged as is, I tested both state test and blockchain test sh files created via both nethtest and geth's evm and it all works now

[felix314159]

Is making use of shlex a requirement to get this merged? The current solution works well and has been tested, I don't think this is a part of the codebase we will touch often