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

[Dynamic Protocol State] Extend tests for EpochStateMachine #5681

Merged
merged 5 commits into from
May 7, 2024

Conversation

durkmurder
Copy link
Member

Context

Added a test which verifies that epoch transition indeed happens in edge cases described in #5631.

Copy link
Member

@AlexHentschel AlexHentschel left a comment

Choose a reason for hiding this comment

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

Thanks for the clean and well-documented test. Added a few minor suggestions for improving the tests specificity and documentation.

Comment on lines 547 to 552
s.epochStateDB.On("StoreTx", mocks.Anything, mocks.Anything).Run(func(args mocks.Arguments) {
updatedState := args[1].(*flow.ProtocolStateEntry)
require.Equal(s.T(), flow.EpochPhaseStaking, updatedState.EpochPhase())
}).Return(storeTxDeferredUpdate.Execute, nil).Once()

s.mutator.On("SetEpochStateID", mocks.Anything).Return().Once()
Copy link
Member

@AlexHentschel AlexHentschel Apr 29, 2024

Choose a reason for hiding this comment

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

I feel that only looking the epoch phase is a relatively coarse check, that could succeed in a variety of ways despite the Epoch State machine doing something wrong. I may be overlooking something, but I feel it should be relatively straight forward to verify that we produce the expected epoch state here:

expectedEpochState := &flow.ProtocolStateEntry{
  PreviousEpoch:                   s.parentEpochState.CurrentEpoch.Copy(),
  CurrentEpoch:                    *s.parentEpochState.NextEpoch.Copy(),
  NextEpoch:                       nil,
  InvalidEpochTransitionAttempted: true,
}

and with that, we should be able to do

Suggested change
s.epochStateDB.On("StoreTx", mocks.Anything, mocks.Anything).Run(func(args mocks.Arguments) {
updatedState := args[1].(*flow.ProtocolStateEntry)
require.Equal(s.T(), flow.EpochPhaseStaking, updatedState.EpochPhase())
}).Return(storeTxDeferredUpdate.Execute, nil).Once()
s.mutator.On("SetEpochStateID", mocks.Anything).Return().Once()
s.epochStateDB.On("StoreTx", expectedEpochState.ID(), expectedEpochState).Return(storeTxDeferredUpdate.Execute, nil).Once()
s.mutator.On("SetEpochStateID", expectedEpochState.ID()).Return().Once()

Hope that makes sense. 😅

state/protocol/protocol_state/epochs/statemachine_test.go Outdated Show resolved Hide resolved
state/protocol/protocol_state/epochs/statemachine_test.go Outdated Show resolved Hide resolved
durkmurder and others added 2 commits May 7, 2024 21:17
Co-authored-by: Alexander Hentschel <alex.hentschel@flowfoundation.org>
@durkmurder durkmurder changed the base branch from feature/protocol-state-kvstore to master May 7, 2024 18:21
@durkmurder durkmurder enabled auto-merge May 7, 2024 18:24
@codecov-commenter
Copy link

codecov-commenter commented May 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 55.51%. Comparing base (24ed447) to head (82127d8).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5681      +/-   ##
==========================================
- Coverage   55.52%   55.51%   -0.02%     
==========================================
  Files        1132     1132              
  Lines       89474    89474              
==========================================
- Hits        49680    49668      -12     
- Misses      35018    35033      +15     
+ Partials     4776     4773       -3     
Flag Coverage Δ
unittests 55.51% <ø> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@durkmurder durkmurder added this pull request to the merge queue May 7, 2024
Merged via the queue into master with commit 65ca341 May 7, 2024
55 checks passed
@durkmurder durkmurder deleted the yurii/EFM-epoch-transition-test branch May 7, 2024 19:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants