Skip to content
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

Test that events that are emitted during block finalization are included in the blocks #1798

Merged
merged 23 commits into from
Dec 6, 2019

Conversation

mrsmkl
Copy link
Contributor

@mrsmkl mrsmkl commented Nov 20, 2019

Description

Just a simple test to check if these events are actually included in the blocks.

Tested

Other changes

Also added some new events that are emitted when paying epoch rewards.

Related issues

Backwards compatibility

@codecov
Copy link

codecov bot commented Nov 20, 2019

Codecov Report

Merging #1798 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1798   +/-   ##
=======================================
  Coverage   74.42%   74.42%           
=======================================
  Files         281      281           
  Lines        7824     7824           
  Branches      975      975           
=======================================
  Hits         5823     5823           
  Misses       1884     1884           
  Partials      117      117
Flag Coverage Δ
#mobile 74.42% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0090203...cd1b946. Read the comment docs.

@asaj asaj self-assigned this Nov 20, 2019
@@ -687,6 +687,15 @@ describe('governance tests', () => {
}
}
})

it('should have emitted transfer events when paying epoch rewards', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we test that the correct events are emitted, and also that the events are only emitted during epoch blocks?

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, adding more checks.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, didn't realize that we don't actually emit the main events that we'd want to record.

More specifically, can we check that the following events are emitted in finalizing the last block of the epoch, and that no events are emitted in finalizing any other block?

TargetVotingYieldUpdated (needs to be added to EpochRewards.updateTargetVotingYield)
ValidatorEpochPaymentDistributed (needs to be added to Validators.distributeEpochPayementsFromSigner)
EpochRewardsDistributedToVoters (needs to be added to Election.distributeEpochRewards)

And can you please add unit tests that check for these events as well?

Thanks!

@asaj asaj assigned mrsmkl and unassigned asaj Nov 20, 2019
@mrsmkl mrsmkl assigned asaj and unassigned mrsmkl Nov 25, 2019
@asaj asaj assigned mrsmkl and unassigned asaj Nov 26, 2019
@mrsmkl mrsmkl requested a review from m-chrzan as a code owner December 2, 2019 09:15
@mrsmkl mrsmkl assigned asaj and unassigned mrsmkl Dec 2, 2019
Copy link
Contributor

@asaj asaj left a comment

Choose a reason for hiding this comment

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

Love it!

@asaj asaj added the automerge Have PR merge automatically when checks pass label Dec 2, 2019
Copy link
Contributor

@diminator diminator left a comment

Choose a reason for hiding this comment

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

LGTM

@celo-ci-bot-user celo-ci-bot-user merged commit 272c4f0 into celo-org:master Dec 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Have PR merge automatically when checks pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants