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

Warn in tests when no events are being emitted #6818

Closed
wants to merge 2 commits into from

Conversation

shawntabrizi
Copy link
Member

In Substrate, we don't emit events on block zero to prevent genesis construction from creating a ton of events.

See #5463

This nuance occasionally comes up as an issue when people are writing runtime tests, since they would need to know to set the block number to greater than zero in order to start emitting events.

If a user does not set the block number, they may wonder why their tests are failing, for example when trying to inspect the last event being emitted.

This PR adds a simple println statement which is emitted only during tests. Thus, if a test would fail, a user would see this error message, and hopefully understand this behavior and fix it by increasing the block number.

@shawntabrizi shawntabrizi added the A2-insubstantial Pull request requires no code review (e.g., a sub-repository hash update). label Aug 5, 2020
@github-actions github-actions bot added the A0-please_review Pull request needs code review. label Aug 5, 2020
@shawntabrizi shawntabrizi added 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. and removed A0-please_review Pull request needs code review. labels Aug 5, 2020
@athei
Copy link
Member

athei commented Aug 5, 2020

I think it is a great change because I ran into this issue lately. However, I think printing unconditionally could really slow down exactly the test cases where one deliberately set it to 0. Even though the output is captured on success, printing might be even slower than just emitting the event.

I guess we need some "print once" logic here. But that would also miss my the point because tests of the same pallet are executed inside the same process and some static Atomic placed here to implement that logic would not be resetted between tests of the same pallet. So I only offer problems and no solutions ;(

if block_number.is_zero() { return }
if block_number.is_zero() {
// Print a message only in tests warning users about this behavior.
#[cfg(test)]
Copy link
Member

Choose a reason for hiding this comment

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

This does not changes anything outside the crate. The test feature is only enabled for a crate when the crate itself is build for testing. This means, any other crate using frame_system will not print anything.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, is there some attribute that does what I intend?

Copy link
Member

Choose a reason for hiding this comment

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

Nope^^

@shawntabrizi shawntabrizi deleted the shawntabrizi-warn-no-events branch August 5, 2020 09:38
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants