-
Notifications
You must be signed in to change notification settings - Fork 14
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
420 change github workflow to run test with feature flags #488
base: main
Are you sure you want to change the base?
420 change github workflow to run test with feature flags #488
Conversation
53ce42c
to
7b0a48d
Compare
.github/workflows/test-code.yml
Outdated
with: | ||
toolchain: nightly-2024-05-30 | ||
command: test | ||
args: --all --features runtime-benchmarks try-runtime |
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.
Don't we need to wrap it in quotation marks?
args: --all --features runtime-benchmarks try-runtime | |
args: --all --features "runtime-benchmarks try-runtime" |
Also, can't we remove the other test in ll. 42-46 then?
bfd9e9d
to
b5f3ef2
Compare
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.
Similar to @ebma's question, to me the intention of #420 is not entirely clear: is the task to 1) just filter out all test for --features runtime-benchmarks
or is it to 2) make these tests work also for --features runtime-benchmarks
.
The issue says:
This allows for changes that break benchmarks to pass the pipeline and therefore be merged into the main branch
This almost sounds to me like it means option 2.
Cargo.toml
Outdated
# probable patch for nightly |
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.
Is this not required anymore? In that case: can we remove it (instead of commenting it out)?
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.
My intention when I wrote the ticket was that whatever cargo command the pipeline runs (which was just plain cargo test
when I wrote the ticket) it would be ran using the runtime-benchmarks
feature so that the pipeline would catch any benchmarks related errors. So this is more like option 2 in my opinion.
This reverts commit e03041e.
…fault weight" This reverts commit 79012a1.
``` error[E0046]: not all trait items implemented, missing: `try_successful_origin` --> /Users/b.carlayap/.cargo/git/checkouts/cumulus-59522f43471fa161/f603a61/parachains/runtimes/assets/common/src/foreign_creators.rs:28:1 | 28 | / impl< 29 | | IsForeign: ContainsPair<MultiLocation, MultiLocation>, 30 | | AccountOf: Convert<MultiLocation, AccountId>, 31 | | AccountId: Clone, ... | 36 | | RuntimeOrigin::PalletsOrigin: 37 | | From<XcmOrigin> + TryInto<XcmOrigin, Error = RuntimeOrigin::PalletsOrigin>, | |___________________________________________________________________________________^ missing `try_successful_origin` in implementation ``` just yet
…ob/30039330371#step:5:1949
…ime` move the benchmark to another workflow
update CI to 1 file only
9ef613f
to
d295174
Compare
Let's wait for #485 to be merged before we merge this one to avoid conflicts with the workflow definitions. |
closes #420
Add another step to run a test but with features
runtime-benchmarks
andtry-runtime
.This also removes
nightly