mana-ethereum / mana

Ethereum full node implementation written in Elixir.
Other
274 stars 47 forks source link

Move non-blockchain Ethereum common tests out of blockchain app #293

Open germsvel opened 6 years ago

germsvel commented 6 years ago

Currently we have the common StateTests and TransactionTests inside the blockchain app:

We should probably move those out of the blockchain app since they aren't the blockchain tests.

We also have ethereum common BlockchainTests and VMTests. Those reside within their respective apps:

I'm not sure where we should move the common tests. Clearly, not all of them match our apps 1-1 (e.g. we do not have an apps/transaction. Because of this, it may make sense to have all the ethereum common tests in a single place (even the blockchain and evm ones). If that does not sound like a good idea, we should at the very least move the StateTests and TransactionTests outside of the blockchain app.

hayesgm commented 5 years ago

547 Begins to at least address unifying our common test code framework. Are you suggesting that Transaction.ex and its counterparts belong in a different umbrella-app altogether?

hayesgm commented 5 years ago

Relates to #566

germsvel commented 5 years ago

@hayesgm

Are you suggesting that Transaction.ex and its counterparts belong in a different umbrella-app altogether

No, I hope we don't move the implementations out to separate umbrella apps.

I think this issue was a little confused and didn't have a clear suggestion. It stemmed from the fact that our ethereum common tests are mixed in with our regular tests, and that can seem strange sometimes.

So maybe a better aim of this issue would be to figure out whether or not we want to have all ethereum common tests live in a single place (just the tests not the implementations) or whether we prefer just keeping them as they are. I think it could provide some clarity on what are ethereum common tests and what are mana's own tests.

All this being said, I think after having more knowledge of the code base and if #566 actually happens, I think they could stay where they are. In which case, we could close this issue altogether.