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

Export a registry of all integration tests #1449

Merged
merged 11 commits into from
Oct 20, 2023

Conversation

alexytsu
Copy link
Contributor

@alexytsu alexytsu commented Oct 18, 2023

Uses a attribute macro to decorate tests that should be added to the exported registry.

Tests in the registry have their name derived by the function that was decorated.

The only test body that has changes is placeholder_deploy_test. Other test bodies have not been changed (though that will most likely be required for a few in followup).

@alexytsu alexytsu force-pushed the alexytsu/integration-test-discovery branch from d0110b9 to 97b5f0a Compare October 18, 2023 05:17
Comment on lines 14 to 26
fn assert_placeholder_actor(exp_bal: TokenAmount, v: &dyn VM, addr: Address) {
let act = v.actor(&addr).unwrap();
assert_eq!(EMPTY_ARR_CID, act.state);
assert_eq!(*PLACEHOLDER_ACTOR_CODE_ID, act.code);
assert_eq!(&Type::Placeholder, v.actor_manifest().get(&act.code).unwrap());
assert_eq!(exp_bal, act.balance);
}

#[exported_test]
pub fn placeholder_deploy_test(v: &dyn VM) {
// Create a "fake" eam.
v.set_actor(
&EAM_ACTOR_ADDR,
new_actor(*EAM_ACTOR_CODE_ID, EMPTY_ARR_CID, 0, TokenAmount::zero(), None),
);
Copy link
Contributor Author

@alexytsu alexytsu Oct 18, 2023

Choose a reason for hiding this comment

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

This just changes some fragile assumptions here don't work on the FVM workbench

@alexytsu alexytsu changed the title WIP: Bugfixes and testing for FVM-Workbench exports Export a registry of all integration tests Oct 18, 2023
@alexytsu alexytsu requested a review from anorth October 18, 2023 05:46
@alexytsu alexytsu marked this pull request as ready for review October 18, 2023 05:46
@codecov-commenter
Copy link

codecov-commenter commented Oct 18, 2023

Codecov Report

Merging #1449 (be30e87) into master (b136ad8) will decrease coverage by 0.13%.
Report is 2 commits behind head on master.
The diff coverage is 3.22%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1449      +/-   ##
==========================================
- Coverage   91.11%   90.99%   -0.13%     
==========================================
  Files         146      147       +1     
  Lines       27936    27981      +45     
==========================================
+ Hits        25455    25461       +6     
- Misses       2481     2520      +39     
Files Coverage Δ
integration_tests/src/lib.rs 100.00% <ø> (ø)
integration_tests/macro/src/lib.rs 3.22% <3.22%> (ø)

... and 10 files with indirect coverage changes

@anorth anorth requested a review from Stebalien October 18, 2023 20:41
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.

This is looking very promising. A few design questions arise.

As you note in filecoin-project/fvm-workbench#34, some tests are infeasibly slow to execute in a real VM. Whether or not we can improve some of them, we probably want some annotation ability at the point of export. Can we add that now? Two potential designs:

  • a simple string annotation, into which we can conventionally pack a comma-separated list of tags or similar (flexible, less "safe")
  • an integer to use as a set of flags, and some flag definitions here (perhaps just one for now, "SLOW" or similar)

This registry raises the idea of whether we can generate all the boilerplate entry points in test_vm/tests, both to remove the duplication and to exercise use of the registry here. That also raises a question about what impact removing the direct entry point code might have on development cycles. E.g. is the entry point name discoverable? Can IDEs easily run single tests

integration_tests/macro/src/lib.rs Outdated Show resolved Hide resolved
integration_tests/src/tests/mod.rs Outdated Show resolved Hide resolved
integration_tests/macro/src/lib.rs Outdated Show resolved Hide resolved
let fn_name = &input_fn.sig.ident;

// Generate a unique identifier for the registration function
let register_fn_name = format_ident!("register_{}", fn_name);
Copy link
Member

Choose a reason for hiding this comment

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

Personally, I'd consider mangling this name to avoid conflicts.

integration_tests/src/tests/mod.rs Outdated Show resolved Hide resolved
integration_tests/macro/src/lib.rs Outdated Show resolved Hide resolved
integration_tests/macro/src/lib.rs Show resolved Hide resolved
@alexytsu
Copy link
Contributor Author

@anorth
For marking slow tests, I used an optional u8 that indicates speed. It defaults to 0 and slower tests can be marked with increasing values. We can probably define some thresholds here, once we have a better picture of the execution times on fvm-workbench.

This registry raises the idea of whether we can generate all the boilerplate entry points in test_vm/tests, both to remove the duplication and to exercise use of the registry here. That also raises a question about what impact removing the direct entry point code might have on development cycles. E.g. is the entry point name discoverable? Can IDEs easily run single tests

From my own setup and understanding, the IDEs will probably struggle to have a nice way to run individual tests. It also almagamates all the tests into one compilation target that needs to rebuild if any test is added/modified. I've left the existing entry points for now and left the test registry for external consumption only.

@alexytsu alexytsu requested a review from anorth October 19, 2023 06:16
@Stebalien
Copy link
Member

From my own setup and understanding, the IDEs will probably struggle to have a nice way to run individual tests. It also almagamates all the tests into one compilation target that needs to rebuild if any test is added/modified. I've left the existing entry points for now and left the test registry for external consumption only.

I've already put them all in one target because it makes CI take much less time.

For individual tests... ideally you'd be able to call:

https://doc.rust-lang.org/stable/test/test/fn.parse_opts.html

But that's unstable. However.... it should be possible to pass CLI options to one big test (for filtering). It'll kind of suck, but it'll also kind of work?

@Stebalien
Copy link
Member

Hm. Ok, so, another way is codegen. I.e., test_vm could import integration_tests at both compile time (build dep) and runtime.

Then, in test_vm/build.rs, we'd import integration_tests::TEST_REGISTRY and code-gen a test function for each test listed.

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.

Ok this looks good to land and try out from the workbench. Happy to defer potential codegen of local entry points for a follow-up.

@alexytsu alexytsu added this pull request to the merge queue Oct 20, 2023
Merged via the queue into master with commit 1e50065 Oct 20, 2023
@alexytsu alexytsu deleted the alexytsu/integration-test-discovery branch October 20, 2023 03:52
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.

4 participants