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

Use an abstract VM trait for integration tests #1265

Closed
wants to merge 21 commits into from

Conversation

alexytsu
Copy link
Contributor

@alexytsu alexytsu commented Mar 30, 2023

WIP: Expect invocations not working anymore. Will need a way to abstract over expectations

#1236

Tracking branch of end-state. This will be broken up and reviewed in chunks:
#1285
#1288
#1289

@alexytsu alexytsu force-pushed the alex/1236-abstract-vm-trait branch from b6fe311 to aec4406 Compare March 30, 2023 06:10
@codecov-commenter
Copy link

codecov-commenter commented Mar 30, 2023

Codecov Report

Merging #1265 (bf1e378) into alex/1236-abstract-vm-trait-easy-tests (15e29d3) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@                           Coverage Diff                            @@
##           alex/1236-abstract-vm-trait-easy-tests    #1265    +/-   ##
========================================================================
  Coverage                                   90.64%   90.64%            
========================================================================
  Files                                         133      133            
  Lines                                       27019    26908   -111     
========================================================================
- Hits                                        24492    24392   -100     
+ Misses                                       2527     2516    -11     
Impacted Files Coverage Δ
test_vm/src/lib.rs 80.98% <100.00%> (-1.43%) ⬇️
test_vm/src/util.rs 99.92% <100.00%> (+0.97%) ⬆️

... and 4 files with indirect coverage changes

@alexytsu alexytsu force-pushed the alex/1236-abstract-vm-trait branch 2 times, most recently from af4231d to 1653d6a Compare April 3, 2023 06:36
Copy link
Contributor Author

@alexytsu alexytsu left a comment

Choose a reason for hiding this comment

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

@anorth this is a WIP but the entire util functionality has been refactored to work with the abstract VM.

I'm partway through refactoring the tests themselves to rely solely on the abstracted trait rather than methods on TestVM but am running into considerable friction around ExpectInvocation logic which seems to be quite central to some of the tests.

@alexytsu alexytsu requested a review from anorth April 3, 2023 06:52
@alexytsu alexytsu force-pushed the alex/1236-abstract-vm-trait branch from 1653d6a to b65e7f5 Compare April 5, 2023 06:44
@alexytsu alexytsu changed the base branch from master to alex/1236-refactored-utils April 5, 2023 06:45
Base automatically changed from alex/1236-refactored-utils to master April 6, 2023 02:57
@alexytsu alexytsu force-pushed the alex/1236-abstract-vm-trait branch 2 times, most recently from 0732bfe to 11d9ae0 Compare April 13, 2023 00:28
@alexytsu alexytsu marked this pull request as ready for review April 13, 2023 23:19
Copy link
Member

@anorth anorth left a comment

Choose a reason for hiding this comment

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

I reviewed about half of this, enough to generate a few requested changes.

It would be ok not to convert all the tests at once. E.g. where there are some new trait methods required for state hacking, we can just mark those tests for follow-up (and file an issue to finish them off), and get them in a new PR. In fact I'd perhaps prefer that this PR only addressed a few of the smaller tests while we settle on the patterns, making it easier to review, land, and then build upon.

I reckon that things like v.assert_state_invariants should be able to be extracted as helper functions, rather than methods. Then we can call them regardless of the VM in use. I think we should start off assuming that we do want those kind of checks to run all the time, though I could be convinced they're not wanted when using the real VM for some reason.

@alexytsu alexytsu force-pushed the alex/1236-abstract-vm-trait branch from 81a72c2 to 851f492 Compare April 21, 2023 04:22
@alexytsu alexytsu changed the base branch from master to alex/1236-abstract-vm-trait-easy-tests April 21, 2023 04:23
@alexytsu alexytsu force-pushed the alex/1236-abstract-vm-trait branch from 851f492 to bf1e378 Compare April 24, 2023 04:31
Copy link
Member

@anorth anorth left a comment

Choose a reason for hiding this comment

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

Thanks. We need to think about how to make this structure more ergonomic. Since the #[test] methods are mostly uniform boilerplate, perhaps we'll be able to make a macro #[vmtest] or similar that automates that little wrapper function. But we also need to think about moving the test bodies out to be importable elsewhere.

Base automatically changed from alex/1236-abstract-vm-trait-easy-tests to master May 1, 2023 01:47
@alexytsu alexytsu mentioned this pull request May 8, 2023
4 tasks
@alexytsu
Copy link
Contributor Author

alexytsu commented May 8, 2023

Outstanding items are tracked in #1294

@alexytsu alexytsu closed this May 8, 2023
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.

3 participants