Skip to content

spec tests: enable msg processing root check#2692

Open
nkryuchkov wants to merge 2 commits intostagefrom
fix-spectest-root-check
Open

spec tests: enable msg processing root check#2692
nkryuchkov wants to merge 2 commits intostagefrom
fix-spectest-root-check

Conversation

@nkryuchkov
Copy link
Contributor

@nkryuchkov nkryuchkov commented Feb 18, 2026

We currently don't fail spec tests on root mismatch in MsgProcessingSpecTest

@nkryuchkov nkryuchkov requested review from a team as code owners February 18, 2026 09:40
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Feb 18, 2026

Greptile Summary

This PR upgrades the post-duty runner state root check in MsgProcessingSpecTest.RunAsPartOfMultiTest from a non-failing logger.Error call to a test-failing require.EqualValues assertion. Previously, when the PostDutyRunnerStateRoot didn't match the actual computed root, the mismatch was only logged as an error but the test would pass silently. Now it properly fails the test with a diff of the expected vs actual state.

  • Replaces logger.Error(...) with require.EqualValues(...) to enforce root equality in spec tests
  • Extracts actualPostRoot variable to avoid redundant hex.EncodeToString computation
  • Aligns with the pattern already used in multi_start_new_runner_duty_type.go (line 131)

Confidence Score: 4/5

  • This PR is safe to merge — it strictly tightens test assertions without affecting production code.
  • The change is minimal and well-scoped: it converts a silent log into a proper test failure assertion. It only affects test infrastructure, not production logic. The pattern matches what's already used in multi_start_new_runner_duty_type.go. One minor inconsistency exists (committee_msg_processing_type.go uses t.Errorf for the same check) but is out of scope.
  • No files require special attention. The single changed file is straightforward.

Important Files Changed

Filename Overview
protocol/v2/ssv/spectest/msg_processing_type.go Replaces silent logger.Error with require.EqualValues to make root mismatch a test failure. Minor variable extraction for clarity. Change is correct and consistent with similar patterns elsewhere.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[RunAsPartOfMultiTest] --> B[runPreTesting: process messages]
    B --> C[Assert error codes]
    C --> D[Compare output messages]
    D --> E[Assert beacon broadcasted roots]
    E --> F[Compute postRoot via GetRoot]
    F --> G{PostDutyRunnerStateRoot == actualPostRoot?}
    G -- Yes --> H[Test passes]
    G -- No --> I[dumpState: compute diff]
    I --> J_OLD["BEFORE: logger.Error — test passes silently ❌"]
    I --> J_NEW["AFTER: require.EqualValues — test fails ✅"]
Loading

Last reviewed commit: c886984

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

1 file reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

Comment on lines 222 to 225
if test.PostDutyRunnerStateRoot != actualPostRoot {
diff := dumpState(t, test.Name, test.Runner, test.PostDutyRunnerState)
logger.Error("post runner state not equal", zap.String("state", diff))
require.EqualValues(t, test.PostDutyRunnerStateRoot, actualPostRoot, "post runner state not equal\n%s\n", diff)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Inconsistent root check in sibling file

Similar root mismatch check in committee_msg_processing_type.go:86-88 still uses t.Errorf (which marks the test as failed but does not stop execution), rather than require.EqualValues (which calls t.FailNow()). Consider applying the same upgrade there for consistency across spec tests.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@greptileai now it's consistent with multi_start_new_runner_duty_type.go

@codecov
Copy link

codecov bot commented Feb 18, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 56.2%. Comparing base (f98fa34) to head (0ae92b7).
⚠️ Report is 1 commits behind head on stage.

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@nkryuchkov nkryuchkov requested a review from iurii-ssv February 18, 2026 18:28
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

Comments