Skip to content

tests: adopt synctest in some places - #16321

Merged
npazosmendez merged 1 commit into
mainfrom
njpm/speed-up-tests-synctest
Aug 11, 2026
Merged

tests: adopt synctest in some places#16321
npazosmendez merged 1 commit into
mainfrom
njpm/speed-up-tests-synctest

Conversation

@npazosmendez

@npazosmendez npazosmendez commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

What this PR does

A bunch of tests that play with sleeps, tickers, etc. can be sped up greatly by simply wrapping them with synctest.Test. There are still a bunch of other tests that would benefit from this, but I tried to scope it down to the ones that just needed to be wrapped, to start with.

Review with "Hide whitespace" for saneness

image
Speed up breakdown Ran locally. This will not have the same impact on CI, mostly because tests are parallelized.
Test main this PR
TestMultiDimensionalQueueAlgorithmSlowConsumerEffects 10.49s 0.11s
TestRemoteQuerier_QueryRetryOnFailure 8.00s 0.00s
TestRecordAndReportRuleQueryMetrics 6.00s 0.00s
TestStoreGateway_InitialSyncFailure 5.15s 0.00s
TestRulerSyncQueue_ShouldNotNotifyChannelMoreFrequentlyThanPollFrequency 5.00s 0.00s
TestRemoteQuerier_StatusErrorResponses 4.00s 0.00s
TestLifecycler_CheckReady 2.39s 0.00s
TestReaderPool_ShouldCloseIdleLazyReaders 2.14s 0.13s
TestRuler_NotifySyncRulesAsync_ShouldNotTriggerRulesSyncingOnAllRulersWhenDisabled 1.46s 0.01s
TestHATrackerCheckReplicaShouldFixZeroElectedAtTimestamp 1.11s 0.00s
TestIngesterRestart 0.90s 0.00s
TestAwaitQueryFrontendServiceRunning_ServiceIsNotReadyInitially 0.50s 0.00s
Total 47.1s 0.25s

Which issue(s) this PR fixes or relates to

n/a

Checklist

  • [n/a] Tests updated.
  • [n/a] Documentation added.
  • [n/a] CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]. If changelog entry is not needed, please add the changelog-not-needed label to the PR.
  • [n/a] about-versioning.md updated with experimental features.

@npazosmendez npazosmendez changed the title tests: adopt synctest in some place tests: adopt synctest in some places Aug 6, 2026
@npazosmendez
npazosmendez force-pushed the njpm/speed-up-tests-synctest branch from fead9a4 to 47e11dd Compare August 6, 2026 20:07
@npazosmendez
npazosmendez marked this pull request as ready for review August 6, 2026 20:17
@npazosmendez
npazosmendez requested review from a team as code owners August 6, 2026 20:17
@npazosmendez npazosmendez reopened this Aug 10, 2026
@pitasi

pitasi commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

please rebase this PR on latest main, otherwise it can't merge — it's blocked on required checks that will never report.

context: #16335 changed the unit test CI matrix and the required checks got renamed (test (N, 10)test (N, 5)). CI runs from before that change only report the old names, so the branch needs to pick up the new workflow from main. sorry for the churn.

@narqo narqo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Structurally, the changes make sense 🔥

I think, in the future we can improve individual tests, which may not even need time.Sleep under the synctest. Thank you for working on that

Wrap wait-heavy tests in synctest.Test so their sleeps, tickers and
retry backoffs run on fake time. Test-only change, no logic changes.
@npazosmendez
npazosmendez force-pushed the njpm/speed-up-tests-synctest branch from 47e11dd to d95619f Compare August 11, 2026 11:52
@npazosmendez npazosmendez added the changelog-not-needed PRs that don't need a CHANGELOG.md entry label Aug 11, 2026
@npazosmendez
npazosmendez merged commit 64ea230 into main Aug 11, 2026
92 of 93 checks passed
@npazosmendez
npazosmendez deleted the njpm/speed-up-tests-synctest branch August 11, 2026 12:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog-not-needed PRs that don't need a CHANGELOG.md entry

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants