Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Only assert events in assert_* functions #13244

Closed
wants to merge 1 commit into from
Closed

Only assert events in assert_* functions #13244

wants to merge 1 commit into from

Conversation

ggwpez
Copy link
Member

@ggwpez ggwpez commented Jan 26, 2023

Turns out we call this function in some Polkadot benchmarks and it makes them panic. Putting this check only in the explicit functions makes more sense anyway IMO and fixes that.

Failing CI example https://gitlab.parity.io/parity/mirrors/polkadot/-/jobs/2320642

@ggwpez
Only assert in assert_ test functions
696e9a5 
Turns out we call this function in some Polkadot benchmarks and it
makes them panic. Putting this check only in the explicit functions
makes more sense anyway IMO and fixes that.

Signed-off-by: Oliver Tale-Yazdi <[email protected]>
@ggwpez ggwpez added A0-please_review Pull request needs code review. A2-insubstantial Pull request requires no code review (e.g., a sub-repository hash update). B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Jan 26, 2023
@ggwpez ggwpez requested a review from muharem January 26, 2023 15:13
@ggwpez ggwpez changed the title Only assert in assert_ test functions Only assert events in assert_* functions Jan 26, 2023
bkchr
bkchr suggested changes Jan 26, 2023
View reviewed changes
Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

The benchmark is failing here: https://github.com/paritytech/polkadot/blob/38c820b260284e615e7184d4f38696b063cceb8a/xcm/pallet-xcm-benchmarks/src/generic/benchmarking.rs#L104

Which is not used?

And the general solution should be to fix the benchmark to switch to block 1 instead of changing this code here.

@ggwpez
Copy link
Member Author

ggwpez commented Jan 26, 2023

The benchmark is failing here:

Yea, actually the benchmark test is failing.

And the general solution should be to fix the benchmark to switch to block 1 instead of changing this code here.

IMO it is a bit unexpected when System::events() panics instead of returning nothing, but I can also change the bench.

@ggwpez
Copy link
Member Author

ggwpez commented Jan 26, 2023

Okay here paritytech/polkadot#6635 @bkchr

@ggwpez ggwpez closed this Jan 26, 2023
@bkchr bkchr deleted the oty-fix-event-assert branch January 26, 2023 19:41
@ggwpez
Copy link
Member Author

ggwpez commented Feb 14, 2023

@conatus-est @marebranza please look into work chat to prevent your account from being removed from the org.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bkchr bkchr bkchr requested changes

@kianenigma kianenigma kianenigma approved these changes

@muharem muharem Awaiting requested review from muharem

Assignees
No one assigned
Labels
A0-please_review Pull request needs code review. A2-insubstantial Pull request requires no code review (e.g., a sub-repository hash update). B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ggwpez @kianenigma @bkchr