test(consensus): add tests for getTCEpochInfo() #2045
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
wanwiset25
requested review from
AnilChinchawale,
anunay-xin,
benjamin202410,
gzliudan and
wgr523
There was a problem hiding this comment.
Pull request overview
Adds unit coverage for getTCEpochInfo() in the v2 consensus engine to help catch regressions during refactors, by asserting expected epoch-switch block/round behavior across key timeout rounds.
Changes:
- Added
TestGetTCEpochInfocovering invalid, boundary, and “far future” timeout rounds. - Added a new
GetTCEpochInfoFakertest hook onXDPoS_v2to expose the internalgetTCEpochInfologic to the external test package.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
consensus/tests/engine_v2_tests/timeout_test.go |
Adds a dedicated test validating epoch-switch info returned for various timeout rounds. |
consensus/XDPoS/engines/engine_v2/testing_utils.go |
Adds an exported *Faker helper to call the internal getTCEpochInfo from tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| func TestGetTCEpochInfo(t *testing.T) { | ||
| // First epoch, round 1, switch block 901 | ||
| // Second epoch, round 901, block 1800 |
There was a problem hiding this comment.
The comment describing the second epoch is inaccurate: with SwitchBlock=900 and Epoch=900, the epoch switch happens at round 900 (block 1800), not round 901. Please update the comment to match the assertions below to avoid confusion during future refactors.
| // Second epoch, round 901, block 1800 | |
| // Second epoch, round 900, block 1800 |
| epochInfo, err = engineV2.GetTCEpochInfoFaker(blockchain, types.Round(1)) | ||
| assert.Nil(t, err) | ||
| assert.NotNil(t, epochInfo) | ||
| assert.Equal(t, big.NewInt(901), epochInfo.EpochSwitchBlockInfo.Number) | ||
| assert.Equal(t, types.Round(1), epochInfo.EpochSwitchBlockInfo.Round) |
There was a problem hiding this comment.
These assertions don't stop the test on failure, but the code dereferences epochInfo immediately afterward. If err != nil or epochInfo == nil, the test can panic and mask the real failure. Consider using require assertions or guarding with if !assert.NoError(...) { return } / if !assert.NotNil(...) { return } before accessing fields on epochInfo.
| func (x *XDPoS_v2) GetForensicsFaker() *Forensics { | ||
| return x.ForensicsProcessor | ||
| } | ||
|
|
There was a problem hiding this comment.
This is an exported method added specifically to expose an internal helper for tests. For consistency with the rest of this file (several methods are explicitly marked as test-only), add a short "for test only" / "WARN: testing purpose only" comment so it’s harder to accidentally use in production code paths.
| // WARN: This function is designed for testing purpose only! |
gzliudan
changed the title
3 participants
Proposed changes
Add tests for getTCEpochInfo() to catch bug during refactors.
Types of changes
What types of changes does your code introduce to XDC network?
Put an
✅in the boxes that applyImpacted Components
Which part of the codebase this PR will touch base on,
Put an
✅in the boxes that applyChecklist
Put an
✅in the boxes once you have confirmed below actions (or provide reasons on not doing so) that