rust-ethereum / evm

Pure Rust implementation of Ethereum Virtual Machine
Apache License 2.0
1.16k stars 355 forks source link

Feat: add rust tests for jsontests #259

Closed mrLSD closed 9 months ago

mrLSD commented 9 months ago

Description

Currently, tests are represented via jsontests crate. It's a good tool 💯, to use as a CLI standalone application.

➡️ This PR also added possibilities to use Rust specific way to run tests with: cargo test.

➡️ The main motivation is to run tests through Rust, to choose which tests to run, and to be able to add tests later. And mostly go through cargo test flow. However, this does not negate all the capabilities of the CLI application. For this purpose, refactoring was done, and utility functions were placed in a separate module, which will allow them to be used both in the application and in Rust tests.

➡️ Also added comments to the code where it makes sense.

➡️ Each single test runs tests from a separate evm jsontests directory.

➡️ The CI has been refactored (however, in this particular case - I'm not sure what is more representative from the CI point of view - running as an application or as cargo test).

mrLSD commented 9 months ago

Why though? What are the advantage of using the testing framework instead of making it a CLI command?

Extended description. Actually, PR does not remove CLI, just extends it. So it's easy to use any of the solutions.

This also makes it more difficult to debug -- previously one just need to add the --debug flag to whatever command they are running.

I think, for debugging CLI is pretty good. But to just check, that all test passed with multi thread in rust specific way, I believe it is not a bad solution.

mrLSD commented 9 months ago

@sorpaas For me there is a difference between running tests through cargo and CLI. Running tests through cargo allows me to approach testing as a development-driven test process. This allows for live debugging through the code and simplifies testing. Implementation via CLI is convenient as an independent tool for running JSON tests and debugging information, but is completely unsuitable for development testing.

This PR propose just extend with rust-specific test, and do not remove CLI capabilities.

mrLSD commented 9 months ago

@sorpaas Also have a question. Do you plan to pass other Eth Fork? Especially Cancun? Currently only Berlin pass succesfully.

sorpaas commented 9 months ago

I see what you mean. However, I still have some concerns about the design. Currently, the added tests look like this:

#[test]
fn st_args_zero_one_balance() {
    const JSON_FILENAME: &str = "res/ethtests/GeneralStateTests/stArgsZeroOneBalance/";
    let tests_status = run::run_single(JSON_FILENAME, false).unwrap();
    tests_status.print_total();
}

This is rather non-gradual -- each unit test case is a whole folder. Optimally, we would want a #[test] attribute for each individual test.

We can do this with proc-macro, or just simply generated Rust files.

What are you actual workflows? If you are using some IDEs which allows you to click-and-run some tests individually, then generated Rust files, while a bit messy, might be a better idea than proc-macro (because the IDE will likely not support that).

sorpaas commented 9 months ago

Do you plan to pass other Eth Fork? Especially Cancun? Currently only Berlin pass succesfully.

Yes definitely. :)

mrLSD commented 9 months ago

This is rather non-gradual -- each unit test case is a whole folder. Optimally, we would want a #[test] attribute for each individual test.

It just runs class of tests.

AFAIK it's about 129 files. It's not a big deal to add it. But one of the pitfalls is that it can be changed, as it's an external repo. But it's also not a problem.

@sorpaas One of the questions - do you approve of that approach and tp spend time on that? My point is to have rust-specific way for testing, and it does not affect CLI.

sorpaas commented 9 months ago

AFAIK it's about 129 files. It's not a big deal to add it. But one of the pitfalls is that it can be changed, as it's an external repo. But it's also not a problem.

We can generate the file. It does not need to be hand-written. I think I have an idea and it'll also simplify testing against Geth.

One of the questions - do you approve of that approach and tp spend time on that? My point is to have rust-specific way for testing, and it does not affect CLI.

Yeah, but we need a more gradual list of test cases.

What I would suggest is that you revert back the CI to use CLI test command. Then I can merge this and we can spend our time working on better libtest integration.

mrLSD commented 9 months ago

@sorpaas In separate PR, if it's aligned with your vision of jsontests unit tests, I can create tests with structure:

jsontests/tests/general_state/
  st_args_zero_one_balance.rs - 46 tests
  st_code_copy_test.rs - 2 tests
  st_example.rs - 12 tests
  st_self_balance.rs - 6 tests
  st_sload_test.rs - 1 tests
  vm_tets/
    vm_arithmetic_testv - 19 tests
    vm_bitwise_logic_operation.rs - 11 tests
    vm_io_and_flow_operations.rs - 15 tests
    vm_log_test.rs - 5 tests 
    vm_tests.rs - 64 tests

with func test body like:

  #[test]
  fn testname() {
      run_single("testfile.json", debug-feature).unwrap();
  }

or proc-macro, something like:

  extern crate proc_macro;
  use proc_macro::TokenStream;
  use quote::quote;
  use syn::{parse_macro_input, LitStr};

#[proc_macro]
pub fn generate_tests(input: TokenStream) -> TokenStream {
  let input = parse_macro_input!(input as LitStr);
  let test_name = input.value();
  let test_file = format!("{}.json", test_name);

  let expanded = quote! {
  #[test]
  fn #test_name() {
     run_single(#test_file, debug-feature).unwrap();
  }
  };

  TokenStream::from(expanded)
}

Or leave it AS IS, for future solutions. ))

sorpaas commented 9 months ago

@mrLSD I like the proc macro thing because it'll be less code. But I'd first want to confirm -- does it work for you? You mentioned that one of the reasons you want to integrate into Rust's test framework is that this can work with your IDE. Does this still work with proc macro? From what I know, they only check the #[test] attribute of function and then execute that, but if we do proc macro then the IDE will not be able to figure that out.

mrLSD commented 9 months ago

@sorpaas Yes, you're absolutely right. IDE proc-macro is not recognized as a test. However, the fact is that there really won't be much test code. The only thing is that the code will look repetitive, as soon as the name of the test and the JSON file changes. However, this will allow you to work with tests in a granular manner.

If this approach suits you, I can take on this implementation.