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

EVM test tool #85

Merged
merged 8 commits into from
Jul 15, 2019
Merged

EVM test tool #85

merged 8 commits into from
Jul 15, 2019

Conversation

chfast
Copy link
Member

@chfast chfast commented Jul 3, 2019

This adds EVM testing tool for EVMC-compatible VM implementations.
It is build out of generic evmone execution tests.

E.g.

evm-test ./aleth-interpreter.so

Do not confuse with evmone-unittests which contains these test + internal unit tests and has evmone linked "statically".

The name evmone-test was also considered.

TODO:

  • Extend EVMC C++ API
  • Add entry in README

@codecov-io
Copy link

codecov-io commented Jul 3, 2019

Codecov Report

Merging #85 into master will decrease coverage by 3.32%.
The diff coverage is 4.22%.

@@            Coverage Diff             @@
##           master      #85      +/-   ##
==========================================
- Coverage   93.78%   90.45%   -3.33%     
==========================================
  Files          16       18       +2     
  Lines        1722     1792      +70     
  Branches      168      182      +14     
==========================================
+ Hits         1615     1621       +6     
- Misses         90      154      +64     
  Partials       17       17

@axic
Copy link
Member

axic commented Jul 3, 2019

Any way moving this back to evmc?

@chfast
Copy link
Member Author

chfast commented Jul 3, 2019

Any way moving this back to evmc?

That would be very inconvenient for developing evmone as you don't want to have unit tests in other project.

@axic
Copy link
Member

axic commented Jul 3, 2019

The name is misleading then, should be evmone-test.

@chfast
Copy link
Member Author

chfast commented Jul 3, 2019

The name is misleading then, should be evmone-test.

It tests EVMs.

@axic
Copy link
Member

axic commented Jul 3, 2019

So it is not evmone specific then?

@chfast
Copy link
Member Author

chfast commented Jul 3, 2019

So it is not evmone specific then?

Description updated.

@axic
Copy link
Member

axic commented Jul 3, 2019

So then my question is reasonable: any possibility moving this out of evmone at some point to aid other VMs using evmc?

@chfast
Copy link
Member Author

chfast commented Jul 3, 2019

So then my question is reasonable: any possibility moving this out of evmone at some point to aid other VMs using evmc?

The answer is still no. The evm-test tool contributes to the EVMC ecosystem, but it would be very inconvenient to have these tests in some other repo. Also, it might be bad to extend EVMC with "testing" feature.

I'm going to get the reference to this tool from ethereum/testing.

@axic
Copy link
Member

axic commented Jul 3, 2019

So it is basically a "state test over evmc" tool. Still, it would make sense to have it outside of evmone to encourage other EVMC compatible EVMs to use it. Not saying it has to be in evmc, since it may not belong there.

@chfast
Copy link
Member Author

chfast commented Jul 4, 2019

So it is basically a "state test over evmc" tool. Still, it would make sense to have it outside of evmone to encourage other EVMC compatible EVMs to use it. Not saying it has to be in evmc, since it may not belong there.

Yes it make sense for other projects but not for this project. And is not worth the trouble and my time at this point.

@@ -43,6 +43,18 @@ bin/evmone-unittests
bin/evmone-bench
```

### Tools

#### evm-test
Copy link
Member

Choose a reason for hiding this comment

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

At the bare minimum extending this section on build instruction if it is possible to only build this without evmone to be used by other projects would be useful.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is not possible right now. I need another round of CMake changes to re-tune build options.

{"MODULE"}};
cli.set_preprocessor(testing::InitGoogleTest);

if (const auto error_code = cli.parse(argc, argv, std::cout, std::cerr); error_code <= 0)
Copy link
Member

Choose a reason for hiding this comment

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

cool, C++17


if (ec != EVMC_LOADER_SUCCESS)
{
if (const auto error = evmc_last_error_msg())
Copy link
Member

Choose a reason for hiding this comment

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

This could be just

const auto error = evmc_last_error_msg();
std::cerr << "EVMC loading error: " << (error ? error : ec) << "\n";

Copy link
Member Author

Choose a reason for hiding this comment

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

The error is a char*.

Copy link
Member

Choose a reason for hiding this comment

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

Ah I see

#include <vector>

/// The loaded EVMC module.
static evmc::vm evmc_module;
Copy link
Member

Choose a reason for hiding this comment

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

It would be less confusing if this and get_evm below were in a separate cpp file, then it would be more clear that it's another implementation for get_evm of vm_loader.hpp
(some setter I guess would be required then, too)

Copy link
Member Author

Choose a reason for hiding this comment

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

It was planned like that, but it gets tricky because evmc_module is set from main().

@@ -0,0 +1,179 @@
// evmone: Fast Ethereum Virtual Machine implementation
Copy link
Member

Choose a reason for hiding this comment

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

It would probably be better if this file were in a separate directory, not unittests?

Copy link
Member Author

Choose a reason for hiding this comment

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

There are more changes to dir structure needed, so I left it for another time.

Preferably the evm-test should be in tools/, not in test/ but because the evm-unitests is shared between evm-test and evmone-unittests it might be overkill.

Maybe just get rid of test/ dir entirely...

@chfast chfast merged commit 38744a5 into master Jul 15, 2019
@chfast chfast deleted the test_tool branch July 15, 2019 13:40
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.

4 participants