Skip to content

Conversation

@DeVikingMark
Copy link
Contributor

Split the single large test method into 7 focused test methods, each
verifying a specific scenario. This improves test isolation and makes
it easier to identify which scenario fails when tests break.

Added a helper method to reduce code duplication and a constant to
replace magic numbers.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR refactors a monolithic test method into 7 focused, isolated test methods with improved readability and maintainability. The split makes it easier to identify which scenario fails and improves test isolation.

Key changes:

  • Split single large test into 7 scenario-specific tests
  • Added a helper method to reduce code duplication
  • Introduced constant MaxIntervalSeconds to replace magic number
Comments suppressed due to low confidence (1)

src/Nethermind/Nethermind.HealthChecks.Test/ClHealthTrackerTests.cs:110

  • Test method naming is inconsistent with the existing test naming conventions in this project. Other test files in Nethermind.HealthChecks.Test use snake_case (e.g., 'free_disk_check_ensure_free_on_startup_no_wait', 'CheckHealth_returns_expected_results'). Consider renaming these methods to follow the established convention, for example: 'check_cl_alive_initially_returns_true'.
    public async Task CheckClAlive_Initially_ReturnsTrue()
    {
        ManualTimestamper timestamper = new(DateTime.Parse("18:23:00"));
        await using ClHealthRequestsTracker healthTracker = await CreateHealthTracker(timestamper);

        healthTracker.CheckClAlive().Should().BeTrue();
    }

    [Test]
    public async Task CheckClAlive_AfterMaxInterval_ReturnsFalse()
    {
        ManualTimestamper timestamper = new(DateTime.Parse("18:23:00"));
        await using ClHealthRequestsTracker healthTracker = await CreateHealthTracker(timestamper);

        timestamper.Add(TimeSpan.FromSeconds(MaxIntervalSeconds + 1));
        healthTracker.CheckClAlive().Should().BeFalse();
    }

    [Test]
    public async Task CheckClAlive_BeforeMaxInterval_ReturnsTrue()
    {
        ManualTimestamper timestamper = new(DateTime.Parse("18:23:00"));
        await using ClHealthRequestsTracker healthTracker = await CreateHealthTracker(timestamper);

        timestamper.Add(TimeSpan.FromSeconds(MaxIntervalSeconds - 1));
        healthTracker.CheckClAlive().Should().BeTrue();
    }

    [Test]
    public async Task CheckClAlive_AfterForkchoiceUpdated_ResetsTimer()
    {
        ManualTimestamper timestamper = new(DateTime.Parse("18:23:00"));
        await using ClHealthRequestsTracker healthTracker = await CreateHealthTracker(timestamper);

        timestamper.Add(TimeSpan.FromSeconds(MaxIntervalSeconds + 1));
        healthTracker.CheckClAlive().Should().BeFalse();

        healthTracker.OnForkchoiceUpdatedCalled();
        healthTracker.CheckClAlive().Should().BeTrue();
    }

    [Test]
    public async Task CheckClAlive_AfterNewPayload_ResetsTimer()
    {
        ManualTimestamper timestamper = new(DateTime.Parse("18:23:00"));
        await using ClHealthRequestsTracker healthTracker = await CreateHealthTracker(timestamper);

        timestamper.Add(TimeSpan.FromSeconds(MaxIntervalSeconds + 1));
        healthTracker.CheckClAlive().Should().BeFalse();

        healthTracker.OnNewPayloadCalled();
        healthTracker.CheckClAlive().Should().BeTrue();
    }

    [Test]
    public async Task CheckClAlive_WithBothRequests_WorksCorrectly()
    {
        ManualTimestamper timestamper = new(DateTime.Parse("18:23:00"));
        await using ClHealthRequestsTracker healthTracker = await CreateHealthTracker(timestamper);

        healthTracker.OnForkchoiceUpdatedCalled();
        healthTracker.CheckClAlive().Should().BeTrue();

        healthTracker.OnNewPayloadCalled();
        healthTracker.CheckClAlive().Should().BeTrue();

        timestamper.Add(TimeSpan.FromSeconds(MaxIntervalSeconds + 1));
        healthTracker.CheckClAlive().Should().BeFalse();
    }

    [Test]
    public async Task CheckClAlive_AtBoundaryConditions_WorksCorrectly()
    {
        ManualTimestamper timestamper = new(DateTime.Parse("18:23:00"));
        await using ClHealthRequestsTracker healthTracker = await CreateHealthTracker(timestamper);

        timestamper.Add(TimeSpan.FromSeconds(MaxIntervalSeconds - 1));
        healthTracker.OnForkchoiceUpdatedCalled();
        healthTracker.CheckClAlive().Should().BeTrue();

        timestamper.Add(TimeSpan.FromSeconds(2));
        healthTracker.CheckClAlive().Should().BeTrue();
    }

Comment on lines +17 to +25
private static async Task<ClHealthRequestsTracker> CreateHealthTracker(ManualTimestamper timestamper)
{
ManualTimestamper timestamper = new(DateTime.Parse("18:23:00"));
await using ClHealthRequestsTracker healthTracker = new(
return new ClHealthRequestsTracker(
timestamper,
new HealthChecksConfig()
{
MaxIntervalClRequestTime = 300
MaxIntervalClRequestTime = MaxIntervalSeconds
}, LimboLogs.Instance);
}
Copy link

Copilot AI Jan 10, 2026

Choose a reason for hiding this comment

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

The helper method is marked as async and returns Task, but the ClHealthRequestsTracker constructor is synchronous. This adds unnecessary overhead from the async state machine. Change the method to return ClHealthRequestsTracker directly and remove the async keyword. The test methods can then use 'using' instead of 'await using'.

Copilot uses AI. Check for mistakes.
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.

2 participants