Adding Event Index Sequence checking to UTs#3386
Conversation
Tonix517
requested review from
janezpodhostnik,
m4ksio,
pattyshack and
ramtinms
as code owners
| for _, event := range events { | ||
| require.Equal(t, expectedEventIndex, event.EventIndex) | ||
| if testutil.IsServiceEvent(event, chainID) { | ||
| expectedEventIndex += 2 |
There was a problem hiding this comment.
In our code at https://github.com/onflow/flow-go/blob/master/fvm/environment/event_emitter.go#L154-L155, for service events we do double counting (original behavior), so the event after one service event will have index as +2.
However if AppendServiceEvent() fails and AppendEvent() is skipped, then if EmitEvent() is called again, we can continue to have unique index values. If this use case can happen, we may want to keep the existing extra counting for service events.
There was a problem hiding this comment.
This looks like a bug. Can you open an issue for it?
FVM Benchstat comparisonThis branch with compared with the base branch onflow:master commit bba471c The command Collapsed results for better readability
|
|||||||||||||||||||||||||||||||||||||||||||
Codecov Report
@@ Coverage Diff @@
## master #3386 +/- ##
==========================================
- Coverage 55.45% 48.64% -6.82%
==========================================
Files 746 163 -583
Lines 67984 14297 -53687
==========================================
- Hits 37703 6955 -30748
+ Misses 27222 6907 -20315
+ Partials 3059 435 -2624
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
| for _, event := range events { | ||
| require.Equal(t, expectedEventIndex, event.EventIndex) | ||
| if testutil.IsServiceEvent(event, chainID) { | ||
| expectedEventIndex += 2 |
There was a problem hiding this comment.
This looks like a bug. Can you open an issue for it?
fvm/fvm_fuzz_test.go
Outdated
| } | ||
| } | ||
|
|
||
| func ensureEventsIndexSeq(t *testing.T, events []flow.Event, chainID flow.ChainID) { |
There was a problem hiding this comment.
you're duplicating these two functions. maybe add it to a shared testutil
pattyshack
left a comment
pattyshack
left a comment
There was a problem hiding this comment.
see my duplicated code comment, but otherwise i don't have issues with this
Tonix517
force-pushed
the
tonyz/add_ut_event_index
branch
from
d2b46d5 to
2388999
Compare
Tonix517
force-pushed
the
tonyz/add_ut_event_index
branch
from
2388999 to
f21e39a
Compare
|
bors merge |
|
will merge it first and treat #3393 as a separate fix in future PR |
4 participants
Adding new checking in UT cases to make sure numbers in event index sequence are unique and increasing. As a follow-up to #3371 (review)
changes to FVM and execution computer UT are split into separate commits.