feat: rework test-harness for integration tests

[lima-limon-inc]

Tackles #981

This PR aims to remove some functionality from the compiler's testing crate and rely on miden-protocol's testing crate.
This includes the following changes:

• Removed the create_note_from_package function. This now relies on miden-protocol's NoteBuilder (which got Package support added into it in this PR).
• Removed the account_component_from_package function which was replaced with AccountoComponent::from_package.
• Now, the compile_rust_package method calls miden build instead of the compiler's library.
• Removed build_existing_basic_wallet_account_builder function. MockChainBuilder::add_existing_account_from_components which was also added in PR
• Removed NoteCreationConfig since it was superseded by NoteBuilder.

[lima-limon-inc]

@greenhat @bitwalker The PR is not yet finished, but what do you think about this approach?

Any comments / recommendations are more than welcome.

Thanks in advance!

[greenhat]

Good job!

The plan is to have something along these lines:

#[miden_test(
    chain(name = "builder"),
    faucet(name = "faucet", max_supply = 1_000_000_000),
    account(name = "bob"),
    account(name = "alice"),
)]
pub fn test_basic_wallet_p2id() {
    (...)
}

The only problem with attribute macro DSLs I see is discoverability by the user. To this day I have to jump and search the docs when I'm using clap. OTOH, it's powerful and we can design it however we want. Let's try to implement the example above and see how it goes.

[lima-limon-inc]

Good job!

Thank you very much! 😊

The only problem with attribute macro DSLs I see is discoverability by the user. To this day I have to jump and search the docs when I'm using clap.

Completely agree, clap can definitely feel like a maze at times. I'll keep discoverability in mind for this PR.

The first thing that comes to mind is implementing a "help" command in the macro itself. Something like:

#[miden_test(help)]
pub fn test_basic_wallet_p2id() {
    (...)
}

Which would display all of the supported attributes.

Whilst I haven't seen this idea of a "help" command in macros, it's pretty standard in CLI tools; so maybe the it isn't too far out there.

I'll tackle it and see how it goes. Do let me know if you have any thoughts!

[lima-limon-inc]

#[miden_test(help)]
pub fn test_basic_wallet_p2id() {
    (...)
}

Which would display all of the supported attributes.

Whilst I haven't seen this idea of a "help" command in macros, it's pretty standard in CLI tools; so maybe the it isn't too far out there.

I'll tackle it and see how it goes. Do let me know if you have any thoughts!

I have just pushed the code that supports #[miden_test(help)]. When used, it will produce the following output:

--> tests/integration-network/src/mockchain/basic_wallet.rs:20:1
 |
 | #[miden_test(help)]
 | ^^^^^^^^^^^^^^^^^^^
 |
 = help: message:
   <documentation string>

Where <documentation string> is the documentation for every supported attribute (and its corresponding recognized fields). The list of supported struct is none other than all the enum variants present in RecognizedAttrsBuilder.

A nifty detail is that it uses each struct's doc-comments to generate the help message. Which means that it remains consistent and up to date with the generated cargo-doc webpage.

Additionally, I also added support for #[miden_test(help(attribute))] which only displays <attribute>'s help message.

The actual displayed help message is still a bit rough around the edges, but all the "logic" to collect and generate the message is all there.

I'll update the PR as soon as that is done.

Just wanted to update the PR to show how things are shaping up 😊. Any comment/suggestion is more than welcome!

Cheers!

[greenhat]

I have just pushed the code that supports #[miden_test(help)]. When used, it will produce the following output:

--> tests/integration-network/src/mockchain/basic_wallet.rs:20:1
 |
 | #[miden_test(help)]
 | ^^^^^^^^^^^^^^^^^^^
 |
 = help: message:
   <documentation string>

Where <documentation string> is the documentation for every supported attribute (and its corresponding recognized fields). The list of supported struct is none other than all the enum variants present in RecognizedAttrsBuilder.

A nifty detail is that it uses each struct's doc-comments to generate the help message. Which means that it remains consistent and up to date with the generated cargo-doc webpage.

Nice! I have not thought about calling a macro to get help.

[lima-limon-inc]

@bitwalker @greenhat I believe the PR is ready for review. Do note that it mainly focuses on refactoring the present test harness infrastructure itself to make it compatible with integration tests.
The main changes are the introduction of the #[miden_test] attribute instead of the previous approach where the "magic" miden-testing structures were generated from passed in arguments.
This new macros currently support:

• account
• chain
• faucet
• package
• help: this is a special attribute which will display all the available documentation. This is generated from the doc comments from all the various structs and its fields.

The actual expansion of the present tests could probably be done in a separate PR since this already is quite extensive.

Would love to hear your thoughts!

PS: The CI is currently running but it should pass, I've just pushed a commit updating the documentation.

[bitwalker]

Definitely cleans up the boilerplate!

I do have some reservations about the level of magic involved, but the help attribute does mitigate that for the most part (of course, one has to know to use that in the first place, but it's still better than nothing). I definitely wish that attribute macros supported the same "helper" attributes mechanism that derive macros do, since that would help with discoverability, but 🤷.

I would suggest we provide a higher-level "user guide"-type document as well, maybe in test-harness/README.md, which goes into detail how the different options stack/interact, what the defaults are, and whether some options require others to be provided, and if so, under what conditions. That sort of guidance is the main thing missing IMO, as currently we largely are just relying just on a small set of examples, and the minimal help documentation produced by the help argument (which isn't very discoverable/accessible unless you look at the examples where it is featured).

I'm marking this approved though, and we can merge this once an initial version of that document is added here, just ping me!

[lima-limon-inc]

Definitely cleans up the boilerplate!

I'm glad it helps 😊

I would suggest we provide a higher-level "user guide"-type document as well, maybe in test-harness/README.md, which goes into detail how the different options stack/interact, what the defaults are, and whether some options require others to be provided, and if so, under what conditions. That sort of guidance is the main thing missing IMO, as currently we largely are just relying just on a small set of examples, and the minimal help documentation produced by the help argument (which isn't very discoverable/accessible unless you look at the examples where it is
featured).

I'm marking this approved though, and we can merge this once an initial version of that document is added here, just ping me!

@bitwalker
Noted, I've just pushed b4f6844 which contains a README.md containing said documentation. Was something like this what you had in mind?
Thanks in advance!

[greenhat]

Good job! Very thorough exploration of what's possible to achieve with macros.

I think that the macro parameters approach in general cost/benefit is not looking good.

Let's look at the following macro parameters:

   account(
        name = "alice_account",
        component = "wallet_package",
        seed = 5,
        with_basic_wallet = true
    ),

It is not much more concise than our current ad hoc and unoptimized API:

   let alice_account = builder
        .add_account_from_builder(
            Auth::BasicAuth,
            build_existing_basic_wallet_account_builder(wallet_package.clone(), true, [5_u8; 32]),
            AccountState::Exists,
        );

But it is significantly more expensive for the developers.

I consider the following to be the extra price that the developers would pay for macros on top of Mockchain (which is not going anywhere):

1. Figuring out, understanding and remembering the entirely new for them macro parameters system.
2. No discoverability, no autocomplete, and no parameters help in an IDE.
3. Limited LSP; no refactoring (renaming alice_account or wallet_package would not work), limited "find references" and "goto definition".
4. Deciphering runtime errors encountered in the macros (wrong path, no .masp file, etc.).
5. LLMs are struggling with the unknown macro params system.

With all that in mind I think that we should consider that macros might not be worth pursuing and we might need to change the direction. The alternatives I see:

1. Some hybrid (declarative/imperative) API on top of MockChain. @bitwalker Like the original Scenario but with imperative escape hatches.
2. Improve MockChain API. Make it more concise for default cases.

[lima-limon-inc]

(...)

I consider the following to be the extra price that the developers would pay for macros on top of Mockchain (which is not going anywhere):

1. Figuring out, understanding and remembering the entirely new for them macro parameters system.

2. No discoverability, no autocomplete, and no parameters help in an IDE.

3. Limited LSP; no refactoring (renaming `alice_account` or `wallet_package` would not work), limited "find references" and "goto definition".

4. Deciphering runtime errors encountered in the macros (wrong path, no .masp file, etc.).

5. LLMs are struggling with the unknown macro params system.

With all that in mind I think that we should consider that macros might not be worth pursuing and we might need to change the direction. The alternatives I see:

I see, those are some great points! Thank you very much for the thorough explanation 😊 @greenhat

1. Some hybrid (declarative/imperative) API on top of MockChain. @bitwalker Like the original `Scenario` but with imperative escape hatches.

I have been working on a rework considering your comments and also taking as inspiration the Scenario struct that you mentioned.

Firstly, the Scenario struct is broken into multiple "steps" structs (à la typestate pattern). Every step struct has a next method which turns one struct into the next. These can be chained to form fairly idiomatic code, like so:

    let scenario = ScenarioBuilder::new()
        .next()
        .add_package("../../examples/basic-wallet", "wallet")
        .add_package("../../examples/p2id-note", "p2id-note")
        .add_package("../../examples/basic-wallet-tx-script", "tx-script")
        .next()
        .add_faucet(FaucetAccountConfig::default().with_alias("faucet"))
        (...)

Currently there are 4 of these: ScenarioBuilder (the entry point), ScenarioPackageSetup (used to compile packages and save them under an alias), ScenarioAccountSetup (adds accounts to the mockchain) and Scenario final step where the MockChainBuilder is built.
In case a more detailed/peculiar scenario needs to be tested, every one of these structs contains an escape hatch to the underlyling MockChainBuilder. Thus, if needed, at any point one can access it via get_mock_chain(&mut self) -> &mut MockChainBuilder, and manipulate it as needed. These two mechanisms are not mutually exclusive,since the Scenario structs are wrappers around the actual MockChainBuilder.

Additionally, similar to the previous macro PR, we take a "default if omitted" approach: if a value is not specified, we use a "sane" default. This is achieved with the WalletAccountConfig and FaucetAccountConfig structs.

I have just pushed an initial implementation of this approach, and used it in the test_basic_wallet_p2id test to showcase what it might look like. Here's what it currently looks like: 133c669#diff-4396fed87abb04dd237f9628c6d8d2f9a7a1cd701e57248648f1695552514b6cR22-R33.

What do you think of this new approach? Does it better tackle your points? Or should I change course in some/all aspects? Thanks in advance!

Would love to hear your thoughts! 😊

[greenhat]

Thank you! Good job experimenting with Scenario! I'm not sure about aliases though. Errors in strings will be surfaced only at runtime. Plus, the aliases force the user to name the same thing multiple times.

As we've discussed on the call, we should first explore improving the MockChain/MockChainBuilder before trying to build something on top of it. The low-hanging fruits I see here are:

1. Moving the compile_rust_package to the miden-testing crate but using the (installed via midenup) cargo-miden binary to compile the packages. Because we would rather not introduce the compiler in the crate dependencies there and also we need to support multiple installed toolchains.
2. Integrating an equivalent of build_existing_basic_wallet_account_builder into the MockChainBuilder.
3. Making the note construction easier. See create_note_from_package that might be implemented in the miden-testing crate using a builder pattern.
4. Moving (and refactoring) other generic helpers in tests/integration-network/src/mockchain/helpers.rs. to the miden-testing crate.

[greenhat]

Let's check if miden is available and if not, print an instruction (or link to) on how to install it.

[lima-limon-inc]

Let's check if miden is available and if not, print an instruction (or link to) on how to install it.

Great point, I'll add a validation!

Edit: here's the validation: c1f3086

[lima-limon-inc]

This branch contains the changes made in this PR, only backported to version 0.13 of the miden client.

[lima-limon-inc]

Currently, accessing the miden interface requires further intervention by adding some variables to the system's $PATH. This is not ideal for cases like this I believe.

[lima-limon-inc]

I'll create an issue on midenup to track this: 0xMiden/midenup#165

[lima-limon-inc]

@greenhat I believe the PR is reviewable with a couple of caveats:

• It's using changes from this PR from miden-protocol which hasn't been merged yet (the PR is still in draft). Thus, it's currently pointing at a branch in lambda's fork in order for the code to compile
• Midenup also has a PR that's going to tackle this comment. Additionally, if we want for midenup to handle the compilation of the various example directories, then we probably will want to add a custom channel-manifest in each example directory in order for it to download cargo miden from the compiler's repo directly instead of downloading a specific release from crates.io

I tried to use as much code from miden-protocol as possible and remove code that was specific to the compiler's testing suite. I believe the largest gain came from using the NoteBuilder.

Would love to hear your thoughts.

[greenhat]

Looking good! Nice job! Only one minor issue.

[greenhat]

This function is intended to be used by the users (smart contract developers in their tests). And I believe I saw some discussion of how to implement the package compilation in the miden-testing. Or am I missing something?

@lima-limon-inc For the compiler test suite we should use the previous version of the compile_rust_package that calls CompilerTestBuilder. This should eliminate the need for the CI job above.

[lima-limon-inc]

And I believe I saw some discussion of how to implement the package compilation in the miden-testing. Or am I missing something?

If I'm not mistaken, the only mention of package compilation in miden-testing/miden-protocol was this comment by @PhilippGackstatter in #981 (comment).

PhilippGackstatter said:

I'm not sure what this will look like, but if these are compiler specifics, isn't this better suited to be an extension trait of MockChainBuilder instead of integrating the need for midenup into miden-testing (but maybe I'm imagining the wrong thing)?

Edit: I've adressed your comment in this commit: 4c3d1ea.

As an aside, currently the Cargo.toml file points to a branch I created as a backport. I believe that we can update it as soon as this PR 0xMiden/protocol#2502 gets merged, right?