Attempt to include special code only available for cli_examples #5519

crodas · 2024-01-26T06:29:49Z

Description

The cli_examples are converted also to unit tests. These unit tests will compile a binary and execute it. The main goal is to enforce documentation correctness through the unit tests.

Sometimes a special case should be hard-coded (especially mocking user input) that should only be available for the binaries to be executed from the unit tests but should be excluded otherwise.

This PR adds the ability to include code through option_env! macro since the CLI_TEST should only be defined in for the binary to be executed from the unit tests.

This PR is a pre-requisite to solve FuelLabs/forc-wallet#157.

This PR also fixes #5482

This PR will include more examples as part of #5444.

Checklist

I have linked to any relevant issues.
I have commented my code, particularly in hard-to-understand areas.
I have updated the documentation where relevant (API docs, the reference, and the Sway book).
I have added tests that prove my fix is effective or that my feature works.
I have added (or requested a maintainer to add) the necessary Breaking* or New Feature labels where relevant.
I have done my best to ensure that my PR adheres to the Fuel Labs Code Review Standards.
I have requested a review from the relevant team or maintainers.

crodas · 2024-02-01T15:56:30Z

@FuelLabs/tooling any idea how to generate a source map so I can test forc addr2line ?

sdankel · 2024-02-02T19:55:19Z

forc/tests/Forc.toml

+authors = ["Fuel Labs <[email protected]>"]
+entry = "main.sw"
+license = "Apache-2.0"
+name = "tests"


This sample project should be under forc/tests/<project_name>

It should also have a more meaningful name, since all the projects under tests are used for testing.

The binary file should probably not be checked in.

I'll move those files as part of the setup {} and teardown {}

@sdankel I removed all the fixtures from the source code, instead they are all generated inside setup {} and destroyed inside teardown {} or automatically since every test has its own folder space in /tmp,.

That said, tests now run much faster and run naturally in parallel instead of sequentially. /cc @FuelLabs/tooling

forc-util/src/cli.rs

kayagokalp

Looking good, I haven't tested yet but left nitpicks and couple of questions to understand this better.

I see that there is test_project2 under forc-doc. Is that still necessary. Maybe we can rename that to be more meaningful

kayagokalp · 2024-02-05T19:59:15Z

forc-plugins/forc-client/src/cmd/deploy.rs

@@ -30,24 +33,28 @@ pub struct Command {
    pub node: NodeTarget,
    /// Optional 256-bit hexadecimal literal(s) to redeploy contracts.
    ///
-    /// For a single contract, use `--salt <SALT>`, eg.: forc deploy --salt 0x0000000000000000000000000000000000000000000000000000000000000001
+    /// For a single contract, use `--salt <SALT>`, eg.: forc deploy --salt
+    /// 0x0000000000000000000000000000000000000000000000000000000000000001
    ///


Is there a reason we are adding these newlines? Is this how rustfmt formats these 🤔

If these are how these should be formatted I wonder if we can add a check for our doc comments formatting, in a later/seperate pr of course

It is done by rustfmt automatically. I'll see to revert it.

Fixed in 86d96f3

forc-util/src/cli.rs

kayagokalp · 2024-02-05T20:04:47Z

forc-plugins/forc-client/src/util/tx.rs

+    if option_env!("CLI_TEST").is_some() {
+        return Ok(true);
+    }


Not required at all but might make sense to create a macro that expands to this, as this appears a lot. Would bring most of these checks to single line from 3-4 lines.

execute_if_cli_test!(return Ok(true)))

Also I am a little out of the loop so this might be already discussed. What was the reason we are doing these with if and not cfgs?

I don't know if cfg can be used with ENV variables at compile time. Adding features is quite complicated as it needs changes in the Cargo.toml

execute_if_cli_test! { return Ok(true); }

I like macros that can take a group of statements instead of a single statement. Thoughts @JoshuaBatty ? I can make it happen very quickly and that would abstract the usage of the ENV and it can be changed into the future to something like cfg

Fixed in bce67d1

Arent' these only relevant while we are running cargo test? what would be stopping us from using something like cfg(test) instead of a separate env variable CLI_TESTS. Do we want to be able to run tests without cli tests or something?

The following code piece would only fail to compile if you execute cargo t and build correctly if you execute cargo b

fn test_me() { #[cfg(test)] { a = 10; } }

We can maybe make if_cli_tests { x } macro to expand into:

#[cfg(test)] { x }

Ohh I see if we have other unit tests touching those functions things might get complicated, you may want to test the rest of the function but cli tests insertion would return early etc. A final thought is to make this into a separate flag:

fn test_me() { #[cfg(cli_test)] { a = 10; } }

If you do not want to setup features in cargo tomls, you can set thorough env variables:

RUSTFLAGS="--cfg cli_test" cargo test

RUSTFLAGS is a great thing, I did not know about it, I'll give it a go.

Although I like RUSTFLAGS and play with cfg, can we revisit that? I think the PR is ready as it is @kayagokalp .

forc-util/src/cli.rs

forc-plugins/forc-client/mint.json

sdankel · 2024-02-08T19:20:08Z

forc-plugins/forc-client/src/cmd/deploy.rs

-    [ Deploy a single contract from a different path => deploy "bc09bfa7a11a04ce42b0a5abf04fd437387ee49bf4561d575177e2946468b408 --path ../tests/" => r#".*Error making HTTP request.*"# ]
-    [ Deploy to a custom network => deploy "--node-url https://beta-5.fuel.network/graphql" => ".*Refused to create a new wallet.*" ]
+    [ Deploy a single contract from a different path => deploy "bc09bfa7a11a04ce42b0a5abf04fd437387ee49bf4561d575177e2946468b408 --path {path}" => r#".*Error making HTTP request.*"# ]
+    [ Deploy to a custom network => deploy "--node-url https://beta-5.fuel.network/graphql" ]


Should there be an error for this one?

It works for some reason, it used to fail with "account has no funds"

sdankel · 2024-02-08T19:29:20Z

forc-plugins/forc-client/src/op/submit.rs

 use fuel_core_client::client::{types::TransactionStatus, FuelClient};
 use fuel_crypto::fuel_types::canonical::Deserialize;

 /// A command for submitting transactions to a Fuel network.
 pub async fn submit(cmd: cmd::Submit) -> anyhow::Result<()> {
    let tx = read_tx(&cmd.tx_path)?;
    let node_url = get_node_url(&cmd.network.node, &None)?;
+    if_cli_test! {


I think you can do this without an env variable (link)

Suggested change

if_cli_test! {

#[cfg(test)]

{

No, not really. Because this bit of code is executed from a binary that is unaware of the testing context.

Even if you pass cargo run -f test? That should make it aware of the feature.

@sdankel -f does not work with cargo run. I had a few attempts with RUSTFLAGS. I will follow up with a ticket. Ideally, this should work with cfg(test). I'll keep trying to get it done, but if possible in a follow-up PR.

sdankel · 2024-02-08T19:32:32Z

forc/src/cli/mod.rs

@@ -28,6 +28,32 @@ mod commands;
 mod plugin;
 pub mod shared;

+/// Creates a new project and install their dependencies
+#[cfg(any(feature = "test", test))]


Isn't this the same as:

Suggested change

#[cfg(any(feature = "test", test))]

#[cfg(test)]

No, cfg(test) is a built-in thing, we also have a feature "test" whenever the test only function should be included.

The setup is a codeblock that will be called before any test. This was needed in order to easily test things like `forc new`

After a test runs the teardown {} section allows to reset any state for other tests

Updated the macro so every instance have their own CWD so the binaries can be called in parallel

Co-authored-by: Kaya Gökalp <[email protected]>

## Description Add macro to test argument parsing rather than testing the external command through building and spawning a separated process. This is an improved version of #5519 ## Checklist - [x] I have linked to any relevant issues. - [x] I have commented my code, particularly in hard-to-understand areas. - [x] I have updated the documentation where relevant (API docs, the reference, and the Sway book). - [x] I have added tests that prove my fix is effective or that my feature works. - [x] I have added (or requested a maintainer to add) the necessary `Breaking*` or `New Feature` labels where relevant. - [x] I have done my best to ensure that my PR adheres to [the Fuel Labs Code Review Standards](https://github.com/FuelLabs/rfcs/blob/master/text/code-standards/external-contributors.md). - [x] I have requested a review from the relevant team or maintainers.

crodas self-assigned this Jan 26, 2024

crodas force-pushed the more_cli_examples branch 5 times, most recently from 768b36a to c856b15 Compare February 1, 2024 11:35

crodas marked this pull request as ready for review February 1, 2024 15:56

crodas force-pushed the more_cli_examples branch from 95d8e31 to ee24693 Compare February 1, 2024 16:23

crodas enabled auto-merge (squash) February 1, 2024 16:25

crodas requested a review from a team February 1, 2024 21:11

sdankel reviewed Feb 2, 2024

View reviewed changes

crodas force-pushed the more_cli_examples branch from 62a7151 to 4b1f987 Compare February 3, 2024 19:10

crodas requested review from sdankel and a team February 3, 2024 19:12

crodas force-pushed the more_cli_examples branch from 4b1f987 to 43320d7 Compare February 3, 2024 19:13

JoshuaBatty reviewed Feb 4, 2024

View reviewed changes

forc-util/src/cli.rs Outdated Show resolved Hide resolved

JoshuaBatty requested a review from a team February 4, 2024 23:13

crodas force-pushed the more_cli_examples branch from 43320d7 to 60ee2f5 Compare February 5, 2024 13:46

crodas requested a review from JoshuaBatty February 5, 2024 13:47

kayagokalp reviewed Feb 5, 2024

View reviewed changes

crodas force-pushed the more_cli_examples branch from c9e694a to bce67d1 Compare February 5, 2024 22:27

crodas requested a review from kayagokalp February 5, 2024 22:28

JoshuaBatty requested changes Feb 6, 2024

View reviewed changes

forc-util/src/cli.rs Outdated Show resolved Hide resolved

forc-util/src/cli.rs Outdated Show resolved Hide resolved

forc-util/src/cli.rs Outdated Show resolved Hide resolved

JoshuaBatty added the forc label Feb 6, 2024

crodas force-pushed the more_cli_examples branch from a163023 to c6d0943 Compare February 7, 2024 20:45

crodas requested review from JoshuaBatty and a team February 7, 2024 20:49

JoshuaBatty previously approved these changes Feb 8, 2024

View reviewed changes

sdankel reviewed Feb 8, 2024

View reviewed changes

crodas and others added 21 commits February 8, 2024 19:56

Default return values for functions that requires user input

9a4480a

Adding more examples

f9760e1

Add setup handler

da9dc0a

The setup is a codeblock that will be called before any test. This was needed in order to easily test things like `forc new`

Add tests for forc init

1260fd5

Add examples for forc predicate

bad8817

Add more examples for forc new and forc init

e685901

Add tests for forc template

4f231a0

Added support for teardown

9b47959

After a test runs the teardown {} section allows to reset any state for other tests

Improved contract_id with setup{} and teardown {}

78d68b0

Remove assets and use setup {} and teardown {} instead

df6f5d2

Compile once and reuse compiled binaries

00ceebb

Enable parallelism into cli tests

5de5035

Updated the macro so every instance have their own CWD so the binaries can be called in parallel

Update forc-plugins cli_examples macro usage

4581992

Add doc-commetn for macro

b2256b3

Update forc-util/src/cli.rs

8e776fd

Co-authored-by: Kaya Gökalp <[email protected]>

Revert comments

48c2dee

Define if_cli_test! macro

fd3f061

Fixed merging issues

488503a

Remove teardown {}

6754333

Fixed typos

6c092a1

Rename mint.json

54dd5dc

crodas dismissed JoshuaBatty’s stale review via 54dd5dc February 8, 2024 23:01

crodas force-pushed the more_cli_examples branch from 6d65175 to 54dd5dc Compare February 8, 2024 23:01

crodas requested review from sdankel and JoshuaBatty February 8, 2024 23:01

crodas disabled auto-merge February 9, 2024 13:05

crodas marked this pull request as draft February 9, 2024 13:05

crodas mentioned this pull request Feb 9, 2024

Improve cli_examples macro #5589

Merged

7 tasks

crodas closed this Feb 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Attempt to include special code only available for cli_examples #5519

Attempt to include special code only available for cli_examples #5519

crodas commented Jan 26, 2024 •

edited

Loading

crodas commented Feb 1, 2024

sdankel Feb 2, 2024

crodas Feb 2, 2024

crodas Feb 3, 2024

kayagokalp left a comment

kayagokalp Feb 5, 2024

kayagokalp Feb 5, 2024 •

edited

Loading

crodas Feb 5, 2024

crodas Feb 5, 2024

kayagokalp Feb 5, 2024

kayagokalp Feb 5, 2024

crodas Feb 5, 2024

crodas Feb 5, 2024

kayagokalp Feb 5, 2024 •

edited

Loading

kayagokalp Feb 5, 2024

kayagokalp Feb 5, 2024

crodas Feb 5, 2024

crodas Feb 5, 2024

kayagokalp Feb 5, 2024

sdankel Feb 8, 2024

crodas Feb 8, 2024

sdankel Feb 8, 2024

crodas Feb 8, 2024

sdankel Feb 8, 2024

crodas Feb 8, 2024

sdankel Feb 8, 2024

crodas Feb 8, 2024

Attempt to include special code only available for cli_examples #5519

Attempt to include special code only available for cli_examples #5519

Conversation

crodas commented Jan 26, 2024 • edited Loading

Description

Checklist

crodas commented Feb 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kayagokalp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kayagokalp Feb 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kayagokalp Feb 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crodas commented Jan 26, 2024 •

edited

Loading

kayagokalp Feb 5, 2024 •

edited

Loading

kayagokalp Feb 5, 2024 •

edited

Loading