Skip to content

fix(test): Use synctest to deflake timer-based tests.#21957

Open
Fletch153 wants to merge 5 commits intodevelopfrom
fix/CORE-2382-timing-assertions
Open

fix(test): Use synctest to deflake timer-based tests.#21957
Fletch153 wants to merge 5 commits intodevelopfrom
fix/CORE-2382-timing-assertions

Conversation

@Fletch153
Copy link
Copy Markdown
Collaborator

@Fletch153 Fletch153 commented Apr 10, 2026

Summary

Use synctest in TestPlugin_GetLatencies and TestScheduledExecutionStrategy_LocalDON.

Root Cause

The race detector adds 5-10x overhead to memory operations. Tests with absolute timing assertions and only 100ms margins fail intermittently under -race or CI CPU pressure.

Test plan

  • TestScheduledExecutionStrategy_LocalDON passes with -count=50 -race locally
  • CI passes

Fixes: CORE-2382

The race detector adds 5-10x overhead to memory operations. Tests with
tight absolute timing margins (100ms windows) fail intermittently under
-race or CI CPU pressure.

- promwrapper: ceiling 700ms → 2000ms (600ms sleep + race overhead)
- transmission: widen upper bounds by 200ms (100ms windows → 300ms)

Fixes: CORE-2382
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 10, 2026

✅ No conflicts with other open PRs targeting develop

@trunk-io
Copy link
Copy Markdown

trunk-io bot commented Apr 10, 2026

Static BadgeStatic BadgeStatic BadgeStatic Badge

View Full Report ↗︎Docs

@Fletch153
Copy link
Copy Markdown
Collaborator Author

/rerun

@Fletch153
Copy link
Copy Markdown
Collaborator Author

Closing — widening timing margins is a band-aid. The test still uses absolute timing assertions that are inherently flaky under load. A proper fix would either remove the upper-bound assertions or test relative ordering instead of absolute windows.

@Fletch153 Fletch153 closed this Apr 10, 2026
@pavel-raykov pavel-raykov reopened this Apr 11, 2026
@pavel-raykov pavel-raykov marked this pull request as draft April 11, 2026 20:23
@github-actions
Copy link
Copy Markdown
Contributor

I see you updated files related to core. Please run make gocs in the root directory to add a changeset as well as in the text include at least one of the following tags:

  • #added For any new functionality added.
  • #breaking_change For any functionality that requires manual action for the node to boot.
  • #bugfix For bug fixes.
  • #changed For any change to the existing functionality.
  • #db_update For any feature that introduces updates to database schema.
  • #deprecation_notice For any upcoming deprecation functionality.
  • #internal For changesets that need to be excluded from the final changelog.
  • #nops For any feature that is NOP facing and needs to be in the official Release Notes for the release.
  • #removed For any functionality/config that is removed.
  • #updated For any functionality that is updated.
  • #wip For any change that is not ready yet and external communication about it should be held off till it is feature complete.

@pavel-raykov pavel-raykov changed the title fix(test): widen timing assertion margins for race detector overhead fix(test): Use synctest to deflake timer-based tests. Apr 12, 2026
@pavel-raykov pavel-raykov marked this pull request as ready for review April 12, 2026 07:33
Copilot AI review requested due to automatic review settings April 12, 2026 07:33
pavel-raykov
pavel-raykov previously approved these changes Apr 12, 2026
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

Risk Rating: LOW — primarily test-only changes; one small production change (mutable cache defaults) that can affect behavior if unintentionally modified.

This PR deflakes timer-based tests by switching them to Go’s testing/synctest so timing assertions are deterministic under -race and CI load.

Changes:

  • Use testing/synctest in TestPlugin_GetLatencies to avoid real-time sleeps and tighten duration assertions.
  • Use testing/synctest in TestScheduledExecutionStrategy_LocalDON to stabilize timer-based scheduling assertions.
  • Make promwrapper cache cleanup defaults overridable from tests (by changing cache defaults from const to var and overriding defaultCleanupInterval in the test).

Scrupulous human review areas:

  • core/services/ocr2/plugins/promwrapper/plugin.go: introduction of mutable package-level defaults (defaultExpiration, defaultCleanupInterval) and how/where they can be mutated.
  • core/services/ocr2/plugins/promwrapper/plugin_test.go: global override of defaultCleanupInterval and ensuring it’s restored and properly scoped to the synctest run.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
core/services/ocr2/plugins/promwrapper/plugin.go Makes cache default timings mutable to allow tests to disable go-cache GC under synctest.
core/services/ocr2/plugins/promwrapper/plugin_test.go Wraps latency test in synctest and disables cache GC to avoid synctest incompatibility.
core/capabilities/transmission/local_target_capability_test.go Uses synctest for deterministic schedule/timer assertions.
Comments suppressed due to low confidence (1)

core/services/ocr2/plugins/promwrapper/plugin.go:26

  • Only defaultCleanupInterval needs to be overridable for synctest; turning defaultExpiration into a mutable package-level variable is unnecessary and increases the surface for accidental mutation. Consider keeping defaultExpiration as a const and defining defaultCleanupInterval as a separate var (or exposing it via a constructor/option for tests).
// var instead of const to make it overridable in tests
var (
	// defaultExpiration is the default expiration time for cache items.
	defaultExpiration = 30 * time.Minute

	// defaultCleanupInterval is the default interval for cache cleanup.
	defaultCleanupInterval = 5 * time.Minute
)

@cl-sonarqube-production
Copy link
Copy Markdown

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.

3 participants