-
Notifications
You must be signed in to change notification settings - Fork 54
feat: custom test harness #817
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
base: next
Are you sure you want to change the base?
feat: custom test harness #817
Conversation
292e2be to
22f5f5b
Compare
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
22f5f5b to
70fd1f7
Compare
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
70fd1f7 to
1db9ca0
Compare
|
Regarding It means that /cc @bitwalker |
|
Also, rust-lang/rust-analyzer#16662 test explorer is coming. Which we'd want to support as well. EDIT: linking the custom test harness integration issue - rust-lang/rust-analyzer#21259 |
…-lib Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
1db9ca0 to
5bdf3de
Compare
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
31572ad to
df9650a
Compare
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
…the rest untouched Additionally, renamed the function from load_account and added documentation. Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
c620983 to
4056270
Compare
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
6605f89 to
cc5ea76
Compare
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
bitwalker
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! I have left various notes and requested some changes, but I think this is a great starting point for experimenting further with this approach.
I think with some of the changes I suggested, we can get the test discovery working (per @greenhat's request), simply by ensuring that the test code is always emitted, just gated behind #[cfg(test)]. This allows rust-analyzer to see the tests. Additional work is likely still needed to enable integration with rust-analyzer's test lens (e.g. making the test runnable from your IDE), but the basic discovery should work at least.
| @@ -0,0 +1,23 @@ | |||
| [package] | |||
| name = "miden-test-harness-lib" | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's call this crate just miden-test-harness
| }; | ||
| input_module.content.as_mut().unwrap().1.insert(0, internal_use); | ||
|
|
||
| let module = if is_test() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just emit the module annotated with #[cfg(test)]? That should accomplish the same goal, no? It isn't 100% clear why this would need to be facilitated via feature flags on the macro crate (which when you take into account Cargo's feature flag unification approach, could lead to test modules being emitted even when they aren't expected - not likely, but one downside of using features for this sort of thing).
| use miden_test_harness_lib; | ||
|
|
||
| fn main() { | ||
| let args = miden_test_harness_lib::MidenTestArguments::from_args(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd always qualify paths absolutely, e.g. ::miden_test_harness_lib::MidenTestArguments::from_args(), this ensures that the path resolves from the crate root, ignoring any imports in the current module. Not a super big problem in this case, but still good practice for proc macros like this.
| }; | ||
|
|
||
| // This env var is set by `cargo miden test`. | ||
| let package_path = std::env::var("CREATED_PACKAGE").unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should namespace all env vars that we use, e.g. CARGO_MIDEN_CREATED_PACKAGE (though I think it would probably be better to call it CARGO_MIDEN_TEST_PACKAGE_PATH to make it clearer what defines the variable, and what the value is expected to be.
| } | ||
|
|
||
| #[proc_macro_attribute] | ||
| pub fn miden_test_block( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's name this attribute #[miden_test_suite], since that is effectively what it is used to define.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This crate should depend on miden-test-harness-macros, and re-export its macros from the crate root, e.g.:
pub use miden_test_harness_macros::{miden_test, miden_test_suite};Then, downstream crates need only depend on miden_test_harness
| /// Storage map holding the counter value. | ||
| #[storage(slot(0), description = "counter contract storage map")] | ||
| count_map: StorageMap, | ||
| #[cfg(not(test))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of gating this behind #[cfg(not(test))], it would make more sense to gate this behind #[cfg(target_family = "wasm")] - our test suite will never be built for the Wasm target, and even if it was, then the compilation error you ran into wouldn't be an issue anyway.
| // Do not link against libstd (i.e. anything defined in `std::`) | ||
| #![no_std] | ||
| #![feature(alloc_error_handler)] | ||
| #![cfg_attr(not(test), no_std)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're not actually dependent on std for the code we emit via #[miden_test]/#[miden_test_suite] AFAICT, so do we actually need to make this conditionally compiled?
| pub fn get_count(&self) -> Felt { | ||
| let key = Word::from([felt!(0), felt!(0), felt!(0), felt!(1)]); | ||
| self.count_map.get(&key) | ||
| #[miden_test_block] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe test discovery will work if instead of #[miden_test_suite] stripping out the decorated module entirely, it instead annotated it with #[cfg(test)]. I mentioned in my other comment that this would also remove the need for the test_flag feature in miden_test_harness_macros, but the big win here is test discovery.
| // printed. | ||
| #[miden_test] | ||
| fn bar(bar: Package) { | ||
| std::dbg!(&bar); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can support this even in #[no_std] crates by simply adding #[cfg(test)] extern crate std; somewhere in the module (conventionally, at the top near the #![no_std] attribute itself, but I believe it can go anywhere (I haven't experimented with that though).
greenhat
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work!
The list of showstoppers that we need to solve boils down to:
- Working code completion inside the test functions themselves;
- Running tests via the lens in vscode(any IDE using rust-analyzer);
These can be addressed in the subsequent PRs (create gh issues). My only request for this PR is to not change the examples/counter-contract.
| #[storage(slot(0), description = "counter contract storage map")] | ||
| count_map: StorageMap, | ||
| #[cfg(not(test))] | ||
| mod component { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is a user-facing example (i.e. used in cargo miden example) let's copy it to tests/rust-apps-wasm/rust-sdk/ where our internal test examples live.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd actually prefer we start placing these integration test examples under a new directory, tests/examples. We've had a longstanding TODO to re-organize and cleanup the integration tests directory structure and module organization, and we may as well start doing that with new code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure.
|
|
||
| let Some(CommandOutput::BuildCommandOutput { | ||
| output: BuildOutput::Masm { artifact_path }, | ||
| }) = build.exec(OutputType::Masm)? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We discussed that cargo miden test should be just a dumb cargo test alias, nothing more. Otherwise, the rust-analyzer lens and test discovery would not work. Unless vscode calls cargo miden test when the test is started via lens.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That isn't actually true for discovery at least - the test discovery relies purely on analysis of the macro-expanded code, which with the changes I suggested in my review, will work just like any standard test module.
That said, the rust-analyzer test lens is a slightly different story - I haven't looked at how it is implemented, so I don't know how specific it is about how the test gets run.
I agree with the general point though, which is that ideally we can simply make cargo miden test an alias of cargo test, possibly with additional options to allow for us to provide additional Miden-specific functionality when running tests via cargo miden, but hopefully we won't have to require executing tests using cargo miden test, to make IDE integration trivial.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. The point I wanted to make was that we cannot rely on the cargo miden test to compile the package under test. The test harness should compile the package. We could do it in the main function generated by the miden_test_block macro.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The one tricky detail there is going to be whether we will have enough information available when the test binary is built to be able to invoke cargo miden build. I think we should be good, but not 100% sure. I'm guessing it would be enough to bake in the build target name when generating main? Might have to play with it a bit and see what works and what doesn't.

Note: this PR is mainly intended to be an exploration on what a custom test harness for miden could look like. Comments/feeback more than welcome!
This PR implements a custom test harness for miden rust code. The custom harness itself is comprised of two crates:
miden-test-harness-lib: Library which serves as a small wrapper tolibtest_mimicmiden-test-harness-macros: Crate containing the#[miden_test]and#[miden_test_block]macros; which serve as miden-test specific equivalents for#[test]andmod testsrespectively.#[miden_test]adds every function to aninventorylist. Additionally, if the function receives aPackageas argument, it will load the generated Package into a variable automatically; if no argument is passed, thePackageisn't loaded.The
counter-contractexample was updated to showcase what these tests would look like.Additionally, a new subcommand was added to
cargo midencalledtest, which callscargo miden buildto generate the corresponding.maspPackage and then callscargo testwith the required flags.Limitation/Places to go:
rust-analyzer: rust-analyzer 1.91.0 (f8297e35 2025-10-28). This, I believe is related to greenhat's commentsAccountComponentrust code when runningcargo testin order for the test suite to run, if not,cargowould simply panic. I believe this breakage came from the different targets that are required:cargo buildtargetting wasm, whilstcargo testtargetting the native architecture. Hence, I decided to move theAccountComponentcode into a separate module that was only compiled when not using the test profile. However, I don't believe this setup is ideal/final; only a temporary fix.MockChainBuilderis so versatile and has a lot of customization options, I struggled to automatically generate aMockChainand thus the test ended up having a lot of boilerplate. Maybe defining sane defaults would aid in passing in a pre-builtMockChainand thus skipping this boilerplate.Would love to hear your ideas!