Skip to content

Add go-based TokenDuality e2e test#17

Closed
karlb wants to merge 4 commits intocelo1from
karlb/e2e-test
Closed

Add go-based TokenDuality e2e test#17
karlb wants to merge 4 commits intocelo1from
karlb/e2e-test

Conversation

@karlb
Copy link

@karlb karlb commented Sep 25, 2023

Unfortunately the test still has two problems. Please provide feedback on these issues!

Uses external geth command

The geth command is not automatically built when running go test, so it might be outdated or completely missing.

Potential solutions:

  • Run make geth from inside the test. I feel like this is something a test should not do and will surprise users of the test suite who might rely on the last explicitly built geth version for something else they do.
  • Use the test binary. This is something which is done in ./cmd/geth. But without major changes, it will only work if we move the e2e test into that package, as not only private functions are used to run the test binary, but also specific code has to go into the test binary to execute geth from there.

geth not reliably killed after test

Using the CommandContext should send a SIGKILL to geth when the test ends (due to the deferred call to the call's cancel function), but sometimes geth survives and keeps operating like it never received such a signal. I witnessed occasional left over geth processed from other parts of the OP-stack, so maybe the test code is find, but geth is hard to kill in some unusual way?

Potential solutions:

  • Thoroughly check why this is happening and if geth reliably reacts to SIGKILL outsite of the test.
  • At the end of the context (or in another deffered), check if the process still exists and kill it repeatedly until it is gone.

Copy link

@carterqw2 carterqw2 left a comment

Choose a reason for hiding this comment

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

What if we externalize geth management completely and accept the endpoint as an environment variable instead? We can provide a command/script to do that locally, and in CI we might want to implement that differently anyway.

Code: goldTokenBytecode,
Balance: big.NewInt(0),
},
common.HexToAddress("0x42cf1bbc38BaAA3c4898ce8790e21eD2738c6A4a"): {

Choose a reason for hiding this comment

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

Why not reuse DevAddr?

Copy link
Author

Choose a reason for hiding this comment

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

I just missed that occurrence of the address when introducing DevAddr. I changed i just now, but I had to distinguish between the real DevAddr and the 32byte representation of it for the storage fields.

Remaining problems:
* Expects that geth binary has been build
* In some cases, geth keeps running, even though CommandContext sends a
  SIGKILL on cancel.
@karlb
Copy link
Author

karlb commented Sep 25, 2023

What if we externalize geth management completely and accept the endpoint as an environment variable instead? We can provide a command/script to do that locally, and in CI we might want to implement that differently anyway.

That would prevent us from getting a fresh state for each test. But on the bright side, using the same geth across multiple tests makes the tests run faster. We could write the test in a way that it works on unclean state and provide a helper (e.g. a make target) to run the tests as expected with a freshly compiled running geth instance.

How/when to stop geth might still be somewhat tricky.

@karlb karlb closed this in #21 Sep 27, 2023
@karlb karlb deleted the karlb/e2e-test branch April 3, 2024 13:37
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 this pull request may close these issues.

2 participants