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

Simplify contract runtime tests #6921

Closed
matklad opened this issue May 31, 2022 · 2 comments · Fixed by #7012
Closed

Simplify contract runtime tests #6921

matklad opened this issue May 31, 2022 · 2 comments · Fixed by #7012
Assignees
Labels
A-testing Area: Unit testing / integration testing C-housekeeping Category: Refactoring, cleanups, code quality T-contract-runtime Team: issues relevant to the contract runtime team

Comments

@matklad
Copy link
Contributor

matklad commented May 31, 2022

Our average test looks like this:

fn simple_contract() -> Vec
 {
 wat::parse_str(
 r#"
 (module
 (type (;0![](/images/icons/emoticons/wink.png) (func))
 (func (;0![](/images/icons/emoticons/wink.png) (type 0))
 (export "hello" (func 0))
 )"#,
 )
 .unwrap()
 }

#[test]
 fn test_simple_contract() {
 with_vm_variants(|vm_kind: VMKind| 

 { gas_and_error_match( make_simple_contract_call_vm(&simple_contract(), "hello", vm_kind), Some(43032213), None, ); } 

);
 }
 

I find this OK, but I am not entirely satisfied with it. I think we can benefit long-term from spending some cycles on cleaning up this. Specific things I don't like here:

  • our WAT is quite dirty, there's no need for (;0![](/images/icons/emoticons/wink.png) comments, we should use $name and (func (export)) shortcuts
  • I don't like that we use random hello as an entry point. We should use main, and make it conventional (so that we don't have to specify it)
  • explicit with_vm_variants and lambda are a boilerplate we repeat in every test
  • gas_and_error_match requires a bit of typing, is prone to chunrn as it depends on the shape of the API (eg, changing errors will lead to a lot of changes), and requires VmError: Eq
  • make_simple_contract_call_vm familiy of functions is inconventient when you want to tweak some aspect of the code
  • there's a lot of wat::parse-str calls repeated

Ideally, I'd love the tests to be less API-denpendent and more data-driven, and to look like this:

#[test]
 fn test_simple_contract() {
 test_builder()
 .wat(
 r#"
 (module
 (func (export "main")))
 "#,
 )
 .expect(expect![[r#"
 VMOutcome: balance 4 storage_usage 12 return data None burnt gas 42815463 used gas 42815463
 "#]]);
 }
@matklad matklad added C-housekeeping Category: Refactoring, cleanups, code quality A-testing Area: Unit testing / integration testing T-contract-runtime Team: issues relevant to the contract runtime team labels May 31, 2022
@matklad
Copy link
Contributor Author

matklad commented May 31, 2022

@matklad
Copy link
Contributor Author

matklad commented Jun 1, 2022

Good point from jakob: with unified infra, we can have a loop over vm_kinds and protocol versions, making sure that old protocol versions preseve behavior.

near-bulldozer bot pushed a commit that referenced this issue Jun 9, 2022
…6922)

RFC solution for #6921

Vision:

* remove `make_simple_call` family of functions completely in favor of unified `test_builde()`
* use snapshot/expect testing rather than relying on `Error: Eq`. I'd love to use `insta` as that's what we are using elsewhere, but I don't think insta can do what I want here (packaging "snapshot" as a value), so I plugged my `expect_test`. 
* pull the `with_vm_kinds` loop inside
@matklad matklad self-assigned this Jun 10, 2022
nikurt pushed a commit that referenced this issue Jun 13, 2022
…6922)

RFC solution for #6921

Vision:

* remove `make_simple_call` family of functions completely in favor of unified `test_builde()`
* use snapshot/expect testing rather than relying on `Error: Eq`. I'd love to use `insta` as that's what we are using elsewhere, but I don't think insta can do what I want here (packaging "snapshot" as a value), so I plugged my `expect_test`. 
* pull the `with_vm_kinds` loop inside
near-bulldozer bot pushed a commit that referenced this issue Jun 20, 2022
* reduce syntactic fluff and use idiomatic wat
* name the entry point consistently (`main`)
* reduce gas allocated for testing "out-of-gas" conditions, to speed up
  slow tests
* add a new test for re-exporting import
* move feature-gating tests from rs-contract to a simpler `wat`

closes #6921
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testing Area: Unit testing / integration testing C-housekeeping Category: Refactoring, cleanups, code quality T-contract-runtime Team: issues relevant to the contract runtime team
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant