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.

Sign up for GitHub

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

Jump to bottom

RFC: introduce unified testing infrastructure for contract runtime #6922

Merged
merged 6 commits into from
Jun 9, 2022
Merged

RFC: introduce unified testing infrastructure for contract runtime #6922

merged 6 commits into from
Jun 9, 2022

Conversation

matklad
Copy link
Contributor

@matklad matklad commented May 31, 2022

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 force-pushed the m/expect-test branch 4 times, most recently from d7101fd to 17f7c61 Compare June 7, 2022 19:29
@matklad matklad marked this pull request as ready for review June 7, 2022 19:29
@matklad matklad requested a review from a team as a code owner June 7, 2022 19:29
@matklad matklad requested a review from akhi3030 June 7, 2022 19:29
@matklad
Copy link
Contributor Author

matklad commented Jun 7, 2022

This is worthy of review now I think! I want to add some comments to the test_builder tommorow though.

jakmeier
jakmeier reviewed Jun 8, 2022
View reviewed changes
Copy link
Contributor

@jakmeier jakmeier left a comment

Choose a reason for hiding this comment

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

Awesome stuff! Thanks for tackling this, I like the new approach so much better.

I also like how the exact gas change due to protocol_feature_fix_contract_loading_cost is now checked in some tests that it previously wasn't. But see my comments in the code, I think as soon as we stabilise the feature, we lose some test coverage that I explicitly tried to have when I wrote tests for that feature.

runtime/near-vm-runner/src/tests/test_builder.rs Outdated Show resolved Hide resolved
runtime/near-vm-runner/src/tests/test_builder.rs Show resolved Hide resolved
runtime/near-vm-runner/src/tests/runtime_errors.rs
test_builder().wasm(&bad_import_global("env")).method("hello").opaque_error().expect(expect![
[r#"
VMOutcome: balance 4 storage_usage 12 return data None burnt gas 46500213 used gas 46500213
Err: ...
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we need something to specify different errors for different VMs. Or maybe we specifically don't care about what the error instance?
I'm just thinking that in the old version of the test, it was obvious from the code that there is a difference and AND if any of the errors changed, we would see it in the test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I debated this. We could introduce multi-expect functionality here, the same way we do for protocl features.

However, I don't think we actually care. It's a property of our systems that errors are opaque. So we don't loose a lot if we erase everything except the fact that the error occured. Rather than supporting results between vm being different, I decided to double down on expressing that they should be sufficiently the same.

There are a couple of cases where the results are different and we care about that, that is hacked around by skipping two of the three runtimes (see test_nan_sign).

runtime/near-vm-runner/src/tests/runtime_errors.rs Outdated Show resolved Hide resolved
runtime/near-vm-runner/src/tests/runtime_errors.rs Show resolved Hide resolved
runtime/near-vm-runner/src/tests/runtime_errors.rs Show resolved Hide resolved
@matklad matklad requested review from nagisa and Ekleog June 8, 2022 15:20
@matklad
Copy link
Contributor Author

matklad commented Jun 8, 2022

Notable thing this doesn't do: the with_vm_kinds is still used by some tests. For example, the cache tests probe for something which isn't obsevable via outcomes.

jakmeier
jakmeier approved these changes Jun 8, 2022
View reviewed changes
Copy link
Contributor

@jakmeier jakmeier left a comment

Choose a reason for hiding this comment

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

I'd probably leave other reviewers time before merging but LGTM!

@Ekleog
Copy link
Contributor

Ekleog commented Jun 9, 2022

As @jakmeier already did it I have not reviewed all changes in-depth, but taking a look at a few of the resulting changes in tests, it looks much cleaner!

@nagisa
Copy link
Collaborator

nagisa commented Jun 9, 2022

I also looked at it briefly before, and I think interface wise this is good™. I haven't looked at the implementation details too deeply, but I'm willing to bet that its all reasonably good.

@matklad
test: switch vm-runner to data-driven snapshot tests
9543871 
Replaces imperative tests with declarative tests. The primary motivation
here is better maintainability -- now that all tests go through the same
test infrastructure which isolates internal API, it's possible to update
tests en masse.

Additionally, we gained ability to run the same test across different
protocol versions, which should allow us to track not only the latest
protocol, but all historical versions as well.
@matklad
clarify the tricky bit
2dd90cb
@matklad
more readable test
c162cd7
@matklad
clarify assertion
f7791ae
@matklad
allow dead code
c0c6933
@matklad
fix newly added tests
2efc488
@near-bulldozer near-bulldozer bot merged commit 25c852c into near:master Jun 9, 2022
nikurt pushed a commit that referenced this pull request Jun 13, 2022
@matklad @nikurt
RFC: introduce unified testing infrastructure for contract runtime (#…
f361bd4 
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jakmeier jakmeier jakmeier approved these changes

@akhi3030 akhi3030 Awaiting requested review from akhi3030 akhi3030 is a code owner automatically assigned from near/nearcore-codeowners

@nagisa nagisa Awaiting requested review from nagisa

@Ekleog Ekleog Awaiting requested review from Ekleog

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@matklad @Ekleog @nagisa @jakmeier