Skip to content

test: stabilize signal resolved test#4773

Open
hongyunyan wants to merge 1 commit intopingcap:masterfrom
hongyunyan:eventservice-stabilize-signal-resolved-test
Open

test: stabilize signal resolved test#4773
hongyunyan wants to merge 1 commit intopingcap:masterfrom
hongyunyan:eventservice-stabilize-signal-resolved-test

Conversation

@hongyunyan
Copy link
Copy Markdown
Collaborator

@hongyunyan hongyunyan commented Apr 9, 2026

What problem does this PR solve?

The table trigger signal resolved test is brittle because it validates two behaviors that are outside the scenario described by the test name:

  • it depends on the dispatcher still being unhandshaked and therefore expects a HandshakeEvent
  • it uses wall clock time to bypass the resolved-ts rate limiter

That makes the test sensitive to benign changes in handshake timing or rate-limit related setup, even though the behavior under test is only the signal resolved path when there is no forward range and the dispatcher is not in the syncpoint commit stage.

Issue Number: close #4772

What is changed and how it works?

This PR narrows the test scope to the behavior it is supposed to verify.

  • mark the dispatcher as already handshaked so the test does not depend on handshake ordering
  • set lastSentResolvedTsTime to zero time so the rate-limit bypass is deterministic instead of wall-clock based
  • assert only the emitted resolved event and the related watermark state

Check List

Tests

  • Unit test

Questions

Will it cause performance regression or break compatibility?

No.

Do you need to update user documentation, design documentation or monitoring documentation?

No.

Release note

None

Summary by CodeRabbit

  • Tests
    • Enhanced event broker signal resolution test to run with deterministic dispatcher state and improved assertion validation.

@ti-chi-bot ti-chi-bot bot added do-not-merge/needs-triage-completed release-note-none Denotes a PR that doesn't merit a release note. labels Apr 9, 2026
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Apr 9, 2026

📝 Walkthrough

Walkthrough

The test TestProcessTableTriggerDispatcherSendsSignalResolvedWhenNoForwardRangeAndNotInCommitStage is refactored to use deterministic state instead of wall-clock timing. The dispatcher's sequence and last-sent-resolved timestamp are pre-initialized, and assertions now expect a single ResolvedEvent instead of a handshake followed by the event.

Changes

Cohort / File(s) Summary
Test Stabilization
pkg/eventservice/event_broker_test.go
Removed wall-clock timing dependency by pre-setting dispatcher.seq = 1 and dispatcher.lastSentResolvedTsTime = time.Time{}. Simplified assertions from expecting two messages (handshake + resolved event) to one resolved event only.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Possibly related PRs

  • PR #4460: Modifies broker/dispatcher behavior around when resolved-ts signals are sent, which relates to the resolved timestamp rate limiting logic being tested.

Suggested labels

lgtm, approved, size/XS

Suggested reviewers

  • wk989898
  • 3AceShowHand
  • lidezhu

Poem

🐰 A test once danced with time's cruel hand,
Now stands firm on deterministic land,
No more the clock's capricious sway,
Just zero-time and seq in play,
One signal resolved, clear and true! 📨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description check ✅ Passed The description follows the template with all required sections: problem statement, explanation of changes, test type, and answers to compatibility/documentation questions.
Linked Issues check ✅ Passed The code changes directly address all requirements from issue #4772: dispatcher is marked handshaked, lastSentResolvedTsTime uses zero time deterministically, and assertions focus only on the resolved event and watermark state.
Out of Scope Changes check ✅ Passed All changes are directly scoped to test stabilization as described in the PR objectives and linked issue #4772; no extraneous modifications are present.
Title check ✅ Passed The title accurately reflects the main change: stabilizing a brittle unit test by removing non-deterministic time dependencies and handshake ordering dependencies.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@ti-chi-bot ti-chi-bot bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Apr 9, 2026
@hongyunyan hongyunyan changed the title eventservice: stabilize signal resolved test test: stabilize signal resolved test Apr 9, 2026
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request refactors a test in event_broker_test.go to decouple it from handshake logic and improve determinism by using a zero-value timestamp for rate limiting. The review feedback suggests using the setHandshaked() method instead of directly modifying the internal seq field to improve encapsulation and maintain consistency with other tests in the codebase.

dispatcher.lastSentResolvedTsTime.Store(time.Now().Add(-defaultSendResolvedTsInterval - time.Second))
// Keep the test focused on the signal resolved path instead of coupling it with
// the separate handshake behavior.
dispatcher.seq.Store(1)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Consider using the setHandshaked() method instead of directly storing a value in the unexported seq field. This is more idiomatic, ensures consistency with many other tests in this file (e.g., lines 172, 321, 400, 700), and provides better encapsulation of the dispatcher's handshake state.

Suggested change
dispatcher.seq.Store(1)
dispatcher.setHandshaked()

@ti-chi-bot
Copy link
Copy Markdown

ti-chi-bot bot commented Apr 9, 2026

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: asddongmen

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ti-chi-bot ti-chi-bot bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Apr 9, 2026
@ti-chi-bot
Copy link
Copy Markdown

ti-chi-bot bot commented Apr 9, 2026

[LGTM Timeline notifier]

Timeline:

  • 2026-04-09 05:54:39.851484546 +0000 UTC m=+1022085.056844593: ☑️ agreed by asddongmen.

@ti-chi-bot ti-chi-bot bot added the approved label Apr 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved do-not-merge/needs-triage-completed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. release-note-none Denotes a PR that doesn't merit a release note. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

eventservice: stabilize signal resolved table trigger test

2 participants