Skip to content

fix: test to cover sequencing reactor#89

Open
beer-1 wants to merge 1 commit into
minitia/v0.38.xfrom
fix/test
Open

fix: test to cover sequencing reactor#89
beer-1 wants to merge 1 commit into
minitia/v0.38.xfrom
fix/test

Conversation

@beer-1
Copy link
Copy Markdown
Member

@beer-1 beer-1 commented May 11, 2026


PR checklist

  • Tests written/updated
  • Changelog entry added in .changelog (we use unclog to manage our changelog)
  • Updated relevant documentation (docs/ or spec/) and code comments

Summary by CodeRabbit

  • Bug Fixes

    • Improved validator-set handling and proposer selection logic with better nil-safety checks.
    • Enhanced block-validation error handling for commit signatures and invalid proposers.
  • Tests

    • Replaced randomized test fixtures with deterministic validator role-based configurations (sequencer/attestor).
    • Updated block-validation and block-time rule coverage.
    • Added comprehensive voting-power safety tests.

@beer-1 beer-1 changed the title fix test to cover sequencing reactor fix: test to cover sequencing reactor May 11, 2026
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 11, 2026

Walkthrough

This PR refactors validator test construction and validation logic to use role-based voting powers (sequencer vs. attestor) instead of hardcoded constants and random values. Helper functions deterministically assign powers based on validator role, and proposer resolution logic adds nil-safety guards.

Changes

Role-Based Validator Powers and Test Determinism

Layer / File(s) Summary
Proposer Resolution & Safety
types/validator_set.go, types/validation.go
GetProposer() now panics if proposer remains nil after resolution; shouldBatchVerify() resolves proposer with fallback to findProposer() and exits early if nil to avoid unsafe dereferences.
Test Helper Functions
state/helpers_test.go
New testValidatorPower(index) maps index 0 to SequencerVotingPower, others to AttestorVotingPower; new getSequencer() helper locates the sequencer validator; makeState() and genValSet() now use role-based powers instead of fixed constants.
Validator Set Test Fixtures
types/validator_set_test.go
Randomized validators replaced with deterministic testValidator(power) and sequencerValidatorSet(numValidators) helpers; proposer-selection tests removed; new TestValidatorSet_TotalVotingPowerSafe added to cover boundary and overflow cases.
State Execution Tests
state/execution_test.go
Evidence and vote power now derive from current validator VotingPower and TotalVotingPower fields instead of hardcoded constants.
State Rollback Tests
state/rollback_test.go
Validator set initialization switched from RandValidatorSet(5, 10) to genValSet(5); added mock expectation for DeleteBlocksFromHeight().
State Storage Tests
state/store_test.go
Deterministic validator creation via ed25519.GenPrivKey().PubKey() and types.NewValidator(pubKey, types.SequencerVotingPower); replaced randomized construction and hardcoded power 100.
State Validator History Tests
state/state_test.go
TestOneValidatorChangesSaveLoad renamed to TestSequencerValidatorChangesSaveLoad and refactored to track sequencer address changes across heights; proposer-priority assertions adjusted to preserve proposer identity across queried heights.
Block Validation Tests
state/validation_test.go
Test failure expectations shifted: wrongSigsCommit and "unknown validator flagged as commit" now succeed in makeBlock() and fail in ValidateBlock(); block-time test updated to permit times after last block without median matching; added BlockTimeTolerance(30*time.Second) test for far-future block rejection.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 Our validators now know their role,
No more constants, hard and cold—
Sequencer, attestor, each with their might,
Tests deterministic, no more randomized flight!
With proposers guarded and powers true,
The validation chain runs clear and new.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Title check ⚠️ Warning The PR title is vague and does not accurately reflect the main changes, which involve updating validator power constants across multiple test files rather than covering a sequencing reactor. Update the title to be more specific about the main change, such as 'refactor: use dynamic validator power constants in tests' or 'test: align validator voting power with role-based constants'.
Docstring Coverage ⚠️ Warning Docstring coverage is 7.89% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/test

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (2)
state/validation_test.go (2)

454-459: ⚡ Quick win

Make the subtest’s precondition explicit.

The subtest name says “after last block time,” but block.Time is not set explicitly. On Line 458 this currently relies on makeBlock behavior. Set it directly to avoid brittle intent drift.

Suggested patch
 t.Run("block time after last block time passes without tolerance", func(t *testing.T) {
 	height := int64(3)
 	block, err := makeBlock(state, height, lastCommit)
 	require.NoError(t, err)
+	block.Time = state.LastBlockTime.Add(time.Second)
 	err = blockExec.ValidateBlock(state, block)
 	require.NoError(t, err)
 })
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@state/validation_test.go` around lines 454 - 459, The subtest relies on
makeBlock to set block.Time implicitly; make the precondition explicit by
assigning block.Time to a value strictly after the state's last block time
(e.g., state.LastBlockTime plus the validation tolerance and a small delta)
before calling blockExec.ValidateBlock; update the test around makeBlock and
ValidateBlock to set block.Time directly so the subtest name "after last block
time passes without tolerance" cannot drift.

200-201: ⚡ Quick win

Tighten these assertions to the intended invalid-commit failure modes.

At Line 201 and Line 530, require.Error(t, err) is too broad; unrelated failures would still pass. Please assert scenario-specific error type/message for each case so regressions stay diagnosable.

Also applies to: 527-530

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@state/validation_test.go` around lines 200 - 201, The test currently calls
blockExec.ValidateBlock(state, block) and only uses require.Error(t, err), which
is too broad; replace those generic assertions at the two failing spots with
scenario-specific checks: assert the error matches the expected sentinel or
message for the invalid-commit case (e.g., use require.True(t, errors.Is(err,
expectedErr)) if there is a sentinel error like ErrInvalidCommit, or
require.ErrorContains(t, err, "invalid commit") / require.EqualError(t, err,
"expected error string") to match the specific failure message), and import
"errors" if using errors.Is; apply this change to the invocation of
blockExec.ValidateBlock and the surrounding assertions so unrelated failures no
longer pass silently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@state/validation_test.go`:
- Around line 454-459: The subtest relies on makeBlock to set block.Time
implicitly; make the precondition explicit by assigning block.Time to a value
strictly after the state's last block time (e.g., state.LastBlockTime plus the
validation tolerance and a small delta) before calling blockExec.ValidateBlock;
update the test around makeBlock and ValidateBlock to set block.Time directly so
the subtest name "after last block time passes without tolerance" cannot drift.
- Around line 200-201: The test currently calls blockExec.ValidateBlock(state,
block) and only uses require.Error(t, err), which is too broad; replace those
generic assertions at the two failing spots with scenario-specific checks:
assert the error matches the expected sentinel or message for the invalid-commit
case (e.g., use require.True(t, errors.Is(err, expectedErr)) if there is a
sentinel error like ErrInvalidCommit, or require.ErrorContains(t, err, "invalid
commit") / require.EqualError(t, err, "expected error string") to match the
specific failure message), and import "errors" if using errors.Is; apply this
change to the invocation of blockExec.ValidateBlock and the surrounding
assertions so unrelated failures no longer pass silently.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: f27a6c26-9eca-4f26-b759-e87d1f1c2dac

📥 Commits

Reviewing files that changed from the base of the PR and between 38ce4ee and 2217343.

📒 Files selected for processing (9)
  • state/execution_test.go
  • state/helpers_test.go
  • state/rollback_test.go
  • state/state_test.go
  • state/store_test.go
  • state/validation_test.go
  • types/validation.go
  • types/validator_set.go
  • types/validator_set_test.go

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.

1 participant