Fix individual crate compilation issues & add script for running all tests #1444
Conversation
lemunozm
added
I3-annoyance
lemunozm
requested review from
mustermeiszer,
NunoAlexandre,
mikiquantum,
hieronx and
branan
as code owners
| # There is a bug in the frontier repo that adds pallet-ethereum without a try-runtime dependency | ||
| # for this crate which makes our compilation fail with the i_know_what_i_am_doing error. | ||
| # It seem fixed in 0.9.39, and this dependency can be removed from this file safely. | ||
| fp-self-contained = { git = "https://github.com/PureStake/frontier", default-features = false, branch = "moonbeam-polkadot-v0.9.38" } | ||
|
|
There was a problem hiding this comment.
I'm pretty sure this was the issue that happened in #1406 (not tested yet), so it would unlock that PR.
There was a problem hiding this comment.
That could very well be it. But I tried using my fork of frontier fixing all the cargo features and the issue still persisted 🤔
There was a problem hiding this comment.
Have you added "fp-self-contained/try-runtime" too to your try?
There was a problem hiding this comment.
Not sure, but I ran subalfred on the whole repo to make sure I wasn't missing any feature anywhere
There was a problem hiding this comment.
Ah, but before this PR I also run subalfred for all crates, and everything went "well" 😆, and things weren't well. I guess we can not trust 100% in subalfred. Some things are not checked.
| // We need to put the array into the stack (with a vec) to avoid stack overflow | ||
| // here | ||
| let accounts = test_data::system_account::SYSTEM_ACCOUNT.to_vec(); |
There was a problem hiding this comment.
This allows us to remove the stack limit increment in the CI scripts
There was a problem hiding this comment.
I think we should remove this pallet all-together. We will not use it again.
|
It would be great to add this script as a CI job, but it surely exceeds the maximum size available and will take considerably more time than other jobs. Any idea @gpmayorga? My suggestion is to clean artifacts after testing each crate (which will take even more time to run all crates) and create a weekend job for it. |
scripts/tests.sh
Outdated
| go=1 | ||
| fi | ||
|
|
||
| cargo install cargo-workspaces |
There was a problem hiding this comment.
Can we have this as part of a setup script? Having this here means that every time we want to run this (locally) we will have cargo checking for updates and trying to update this crate, which will slow this process even further. wdyt?
There was a problem hiding this comment.
Sure! Make sense. What's the setup script you're referring?
There was a problem hiding this comment.
It doesn't exist atm, but we could create one or adapt the current install-toolchain to be more general-purpose and install tools we need for development
There was a problem hiding this comment.
Ok, I understand. By now, f you agree, I can print a message if the command is not found instead of fetching every time if it exists. I do not want to add things into install_toolchain.sh because it affects all CI job time/space. Only the required job should fetch its required tools, I think.
There was a problem hiding this comment.
We can prepare a good script in another PR, with taplo, subalfred, cargo-workspaces, ...
lemunozm
force-pushed
the
fix-compilation-issues
branch
from
bb34b7e
to
ebd2c97
Compare
|
Thanks Nuno! I think other review is even required 🤣 |
| @@ -222,6 +223,7 @@ impl< | |||
| _currency: <T as pallet_pool_registry::Config>::CurrencyId, | |||
| _max_reserve: <T as pallet_pool_registry::Config>::Balance, | |||
| ) -> DispatchResult { | |||
| #[cfg(feature = "runtime-benchmarks")] | |||
There was a problem hiding this comment.
Are we sure the only code paths for this can be thru the runtime-benchmarks feature? I'm a little concerned about having the mocks behave differently between benches and tests...
There was a problem hiding this comment.
Good point! It's true that it's used in tests even when runtime-benchmarks is not enabled, so in that case, you're not creating a pool. Anyways, it is compiling and passing with and without runtime-benchmarks feature, so I suppose it should behave as expected.
Looking more deeply creating a pool seems to doesn't matter for the testing, but yes for the benchmarks because it adds a lot of weight to the call, so makes sense to only call Although real benchmarks will not use mocks, so actually, I think you could even remove that line 🤔create_pool() on benchmaking.
| // We need to put the array into the stack (with a vec) to avoid stack overflow | ||
| // here | ||
| let accounts = test_data::system_account::SYSTEM_ACCOUNT.to_vec(); |
There was a problem hiding this comment.
I think we should remove this pallet all-together. We will not use it again.
Description
Fixes #1441
Changes and Descriptions
Fixed all compilation issues running all crates isolated 🎉. Now you can run without errors each crate as:
Added script to run all workspace crate tests with all feature combinations.
To run the script for all crates:
To run the script for all crates from the selected one in the workspace list (used mainly to recover the script execution point after a crate failure):