Skip to content

Conversation

@gab8i
Copy link
Contributor

@gab8i gab8i commented Feb 5, 2024

The following PR introduces the usage of xtask for scripting tests and handling CI. It includes minimal required components for enabling sovereign rollup testing. Additional features mentioned in new issues are linked within the code and should be addressed as follow up PRs

p.s.
#[rustfmt::skip] is just my preference for avoiding multiple lines in the "cmd!" macro. If you don't agree with it or if it goes against the standard, I can remove it easily

Copy link
Contributor Author

gab8i commented Feb 5, 2024

Current dependencies on/for this PR:

This stack of pull requests is managed by Graphite.

@gab8i gab8i force-pushed the gab_xtask_ci branch 2 times, most recently from e45ba67 to fabd6a6 Compare February 6, 2024 12:44
sleep 30 && \
echo \"4000 tokens should be minted\" && \
curl -X POST -H \"Content-Type: application/json\" -d '{\"jsonrpc\":\"2.0\",\"method\":\"bank_supplyOf\",\"params\":[\"sov1zdwj8thgev2u3yyrrlekmvtsz4av4tp3m7dm5mx5peejnesga27svq9m72\"],\"id\":1}' http://127.0.0.1:12345",
).run()?;
Copy link
Contributor

Choose a reason for hiding this comment

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

At this point I'd rather have a .sh file.

Copy link
Contributor

@rphmeier rphmeier Feb 7, 2024

Choose a reason for hiding this comment

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

yeah, or at least a separate file which is include_str!d into the source

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was only a quick experiment to see if what I was thinking was possible. It is, so I will find a cleaner way to do it 👍🏻 I was thinking of not using sh at all and just running the tasks with cmd!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What I've implemented in the new commit is just an experiment. I have never written a "shell" script in Rust, so it was worth a try.

If you don't like it, it would be very simple to just execute the .sh file

@gab8i gab8i force-pushed the gab_xtask_ci branch 2 times, most recently from 4004996 to be57e77 Compare February 8, 2024 13:34

#[derive(clap::Args, Debug, Clone)]
pub struct SovereignParams {
#[arg(long = "sovereign-quiet", value_name = "quiet", id = "sovereign.quiet")]
Copy link
Contributor

Choose a reason for hiding this comment

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

Why would you need id?

Copy link
Contributor Author

@gab8i gab8i Feb 9, 2024

Choose a reason for hiding this comment

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

without it clap panics at runtime:

Compiling xtask v0.1.0 (/home/gabriele/Documents/thrum/blobs/xtask)
    Finished dev [unoptimized] target(s) in 9.89s
     Running `target/debug/xtask test --build-skip`
thread 'main' panicked at /home/gabriele/.cargo/registry/src/index.crates.io-6f17d22bba15001f/clap_builder-4.4.17/src/builder/debug_asserts.rs:86:13:
Command test: Argument names must be unique, but 'quiet' is in use by more than one argument or group
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

this is just a work around though, I didn't want to spend a lot of time fighting against clap::derive so I decided to go for the easiest solution for now

any other solution is extremely welcome

Copy link
Contributor Author

@gab8i gab8i Feb 9, 2024

Choose a reason for hiding this comment

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

Another solution could be to set a prefix for all the fields in test::Params (if there is a way to do that in clap)

Copy link
Contributor

Choose a reason for hiding this comment

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

it doesn't seem like each Params struct should have its own quiet param and there should rather be a single quiet flag in the BuildParams

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For sure, this would simplify a lot of things, but I find this kind of granularity necessary. The reason I added them is that now all the stdout is printed in the same terminal. This means zombienet, shim, and sovereign things are all displayed one on top of the other, and this makes debugging extremely cumbersome (i.e., manually redirecting stdout and grepping things). Having the ability to filter out which program to print simplifies things.

Another approach could be, instead of quiet, adding a flag to decide whether or not to redirect stdout to files, like zombienet does automatically with zombienet/collator01.log (or keep both approaches)

Copy link
Contributor

Choose a reason for hiding this comment

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

I, tbh, feel that the appropriate solution here would be to act as zombienet (and it's predecessor polkadot-launch) and redirect the output of the children to some predefined files. In stdout you would just write a heads up where the user can find the logs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I like it! I will implement it right now!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rphmeier, if it's okay with you, for me you can merge it now and I will address the redirect into log files in another PR. I tried implementing it today, but I encountered some strange behavior

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, that seems fine. You can already start building on top of this branch using Graphite (actually, that's the point)

This was referenced Feb 9, 2024
@gab8i gab8i force-pushed the gab_xtask_ci branch 3 times, most recently from 6542078 to 814255e Compare February 9, 2024 15:07
@gab8i gab8i marked this pull request as ready for review February 9, 2024 15:07
@pepyakin pepyakin merged commit 34b1c87 into main Feb 13, 2024
@pepyakin pepyakin deleted the gab_xtask_ci branch February 13, 2024 09:55
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