Skip to content

Remove flaky dial throttler tests#4139

Merged
aaronbuchwald merged 1 commit intomasterfrom
aaronbuchwald/delete-flaky-test-throttler-cancel
Jul 29, 2025
Merged

Remove flaky dial throttler tests#4139
aaronbuchwald merged 1 commit intomasterfrom
aaronbuchwald/delete-flaky-test-throttler-cancel

Conversation

@aaronbuchwald
Copy link
Copy Markdown
Collaborator

@aaronbuchwald aaronbuchwald commented Jul 29, 2025

Resolves #4138

Each of these tests are race conditions. We could modify them, but decided on Slack to remove.

At least the failing cancellation test could be re-written to avoid a race condition:


func TestDialThrottlerCancel(t *testing.T) {
	require := require.New(t)

	// Allows 5 per second
	throttler := NewDialThrottler(5)

	acquiredChan := make(chan error, 1)
	ctx, cancel := context.WithCancel(context.Background())
	cancel()

	// Throttler should return a cancellation error immediately since
	// the context was already cancelled.
	acquiredChan <- throttler.Acquire(ctx)
	require.ErrorIs(<-acquiredChan, context.Canceled)
}

Will leave for PR review if we want to question this and re-write instead.

Copilot AI review requested due to automatic review settings July 29, 2025 17:43
Copilot AI reviewed Jul 29, 2025
View reviewed changes
Copy link
Copy Markdown
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 removes flaky dial throttler tests that contain race conditions as identified in issue #4138. The team decided to remove these problematic tests rather than attempting to fix them, though an example fix for the cancellation test is provided in the PR description for future consideration.

  • Removes three test functions that were causing flaky test failures due to race conditions
  • Eliminates timing-dependent assertions that were unreliable in CI environments
  • Deletes the entire test file since all tests were problematic
Comments suppressed due to low confidence (1)

@aaronbuchwald aaronbuchwald enabled auto-merge July 29, 2025 17:45
@aaronbuchwald aaronbuchwald added this pull request to the merge queue Jul 29, 2025
Merged via the queue into master with commit 6e56650 Jul 29, 2025
30 checks passed
@aaronbuchwald aaronbuchwald deleted the aaronbuchwald/delete-flaky-test-throttler-cancel branch July 29, 2025 18:45
@github-project-automation github-project-automation bot moved this to Done 🎉 in avalanchego Jul 29, 2025
joshua-kim added a commit that referenced this pull request Jul 30, 2025
@joshua-kim
Squashed commit of the following:
8db15d7 
commit 45df9e6
Author: Jonathan Oppenheimer <[email protected]>
Date:   Tue Jul 29 15:44:08 2025 -0400

    chore: Update header year to 2025 (#4140)

commit dca17fd
Author: Stephen Buttolph <[email protected]>
Date:   Tue Jul 29 15:41:14 2025 -0400

    Remove gitignore line that ignores the `database/dbtest` package (#4142)

commit 6e56650
Author: aaronbuchwald <[email protected]>
Date:   Tue Jul 29 14:29:42 2025 -0400

    Remove flaky dial throttler tests (#4139)

commit 577820e
Author: aaronbuchwald <[email protected]>
Date:   Tue Jul 29 13:25:27 2025 -0400

    Add runner input to run c-chain reexecution benchmark on arbitrary target (#4121)

    Signed-off-by: aaronbuchwald <[email protected]>
    Co-authored-by: Copilot <[email protected]>

commit d5cdc50
Author: Stephen Buttolph <[email protected]>
Date:   Tue Jul 29 13:24:13 2025 -0400

    With golangci-lint v2.2.2 using http.NewRequest is discouraged (#4136)

commit d9d6f59
Author: aaronbuchwald <[email protected]>
Date:   Tue Jul 29 11:49:26 2025 -0400

    Remove external-data-json-path from benchmark push step (#4134)

commit 0c62370
Author: aaronbuchwald <[email protected]>
Date:   Mon Jul 28 14:12:02 2025 -0400

    Split action benchmark comparison and push to gh-pages (#4130)

commit b1433fd
Author: Geoff Stuart <[email protected]>
Date:   Fri Jul 25 14:50:16 2025 -0400

    Enable Cubist Signer integration (#3965)

    Signed-off-by: Geoff Stuart <[email protected]>
    Signed-off-by: Joshua Kim <[email protected]>
    Co-authored-by: Richard Pringle <[email protected]>
    Co-authored-by: Joshua Kim <[email protected]>
    Co-authored-by: Stephen Buttolph <[email protected]>

commit 44c79a8
Author: rodrigo <[email protected]>
Date:   Fri Jul 25 10:52:17 2025 -0400

    refactor: remove load 1.0  (#4112)

commit e658cf4
Author: jishudashu <[email protected]>
Date:   Fri Jul 25 22:50:43 2025 +0800

    refactor: use maps.Copy for cleaner map handling (#4119)

    Signed-off-by: jishudashu <[email protected]>

commit 0980edf
Author: aaronbuchwald <[email protected]>
Date:   Thu Jul 24 16:13:30 2025 -0400

    Update codeowners of reexecution changes (#4116)

commit 44b1e6c
Author: Sam Liokumovich <[email protected]>
Date:   Thu Jul 24 16:00:45 2025 -0400

    Simplex QuorumCertificate and BLS aggregator (#4091)

commit 5af69e6
Author: rodrigo <[email protected]>
Date:   Thu Jul 24 13:17:48 2025 -0400

    ci: remove load 1.0 (#4106)

commit 12b75d4
Author: aaronbuchwald <[email protected]>
Date:   Wed Jul 23 13:01:42 2025 -0400

    Add step to push benchmark results to gh-pages (#4103)

    Signed-off-by: aaronbuchwald <[email protected]>

commit c712437
Author: Sam Liokumovich <[email protected]>
Date:   Wed Jul 23 11:33:29 2025 -0400

    Remove Stale References of the toEngine Channel (#4101)

Signed-off-by: Joshua Kim <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@yacovm yacovm yacovm approved these changes

@StephenButtolph StephenButtolph StephenButtolph approved these changes

Assignees

No one assigned

Labels

None yet

Projects

avalanchego
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Flaky TestDialThrottlerCancel

4 participants

@aaronbuchwald @yacovm @StephenButtolph