Skip to content

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.


### 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.

5 participants