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

Feat: add rust tests for jsontests #259

Merged
merged 2 commits into from
Dec 7, 2023

Conversation

mrLSD
Copy link
Contributor

@mrLSD mrLSD commented Dec 6, 2023

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 mrLSD changed the title Feat> Feat: add rust tests for jsontest Dec 6, 2023
@mrLSD mrLSD changed the title Feat: add rust tests for jsontest Feat: add rust tests for jsontests Dec 6, 2023
Copy link
Member

@sorpaas sorpaas left a comment

Choose a reason for hiding this comment

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

Why though? What are the advantage of using the testing framework instead of making it a CLI command? This also makes it more difficult to debug -- previously one just need to add the --debug flag to whatever command they are running.

Plus the testing framework by default run things multi-threaded, which makes the output unreadable.

@mrLSD
Copy link
Contributor Author

mrLSD commented Dec 6, 2023

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
Copy link
Contributor Author

mrLSD commented Dec 6, 2023

@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
Copy link
Contributor Author

mrLSD commented Dec 6, 2023

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

@sorpaas
Copy link
Member

sorpaas commented Dec 7, 2023

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
Copy link
Member

sorpaas commented Dec 7, 2023

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

Yes definitely. :)

@mrLSD
Copy link
Contributor Author

mrLSD commented Dec 7, 2023

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
Copy link
Member

sorpaas commented Dec 7, 2023

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
Copy link
Contributor Author

mrLSD commented Dec 7, 2023

@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 sorpaas merged commit 1e1aac7 into rust-ethereum:master Dec 7, 2023
3 checks passed
@sorpaas
Copy link
Member

sorpaas commented Dec 7, 2023

@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 mrLSD deleted the feat/add-rust-tests branch December 7, 2023 15:01
@mrLSD
Copy link
Contributor Author

mrLSD commented Dec 7, 2023

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

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