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 macro_assembler.nim and re-enable tests #1447

Closed
kdeme opened this issue Jan 26, 2023 · 1 comment · Fixed by #1516
Closed

Fix macro_assembler.nim and re-enable tests #1447

kdeme opened this issue Jan 26, 2023 · 1 comment · Fixed by #1516

Comments

@kdeme
Copy link
Contributor

kdeme commented Jan 26, 2023

In #1445 3 tests got disabled: test_op_env, test_op_misc and test_txpool.
This is because they would fail on the usage of the Stop op code.

It seems that the macro breaks on an ambiguous identifier for Stop. There is ambiguity between the Stop from the Ops enum and the Stop from the ServerCommand enum in chronos. (The file where the macro is located is already not importing the one of Chronos: https://github.com/status-im/nimbus-eth1/blob/master/tests/macro_assembler.nim#L17. But that is likely not enough and it is probably due to the imports of test where the macro is used).

Some possible fixes:

  • Figure out where the leakage comes from and try to exclude ServerCommand from (I've tried this to some degree but can stop the leaking so far, and it is definitely not from an export).
  • Adjust the macro to deal with the nnkClosedSymChoice and select the right one.
  • Don't work with the untyped opcodes but instead work with strings that are then converted with parseEnum.
@zah
Copy link
Contributor

zah commented Jan 27, 2023

Please note that the third option does not require changing the syntax in any way. It's merely a suggestion to immediately turn the identifier into a string within the macro and then to use parseEnum on that string instead of using any of the more complicated getType machinery.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants