Skip to content

Conversation

@lance6716
Copy link
Contributor

@lance6716 lance6716 commented Feb 9, 2026

What problem does this PR solve?

Issue Number: close #10229

What is changed and how does it work?

call runtime.GC, no need to wait the indeterministic GC behaviour

Check List

Tests

  • No code

Code changes

Side effects

  • Possible performance regression
  • Increased code complexity
  • Breaking backward compatibility

Related changes

Release note

None.

Summary by CodeRabbit

  • Tests
    • Modified test conditions to ensure garbage collection timing validation in memory tuner testing.

Signed-off-by: lance6716 <lance6716@gmail.com>
Copilot AI review requested due to automatic review settings February 9, 2026 05:49
@ti-chi-bot
Copy link
Contributor

ti-chi-bot bot commented Feb 9, 2026

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign huachaohuang for approval. For more information see the Code Review Process.
Please ensure that each of them provides their approval before proceeding.

The full list of commands accepted by this bot can be found 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 do-not-merge/needs-linked-issue release-note-none Denotes a PR that doesn't merit a release note. dco-signoff: yes Indicates the PR's author has signed the dco. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 9, 2026
@coderabbitai
Copy link

coderabbitai bot commented Feb 9, 2026

📝 Walkthrough

Walkthrough

This PR adds a runtime.GC() call in the TestGlobalMemoryTuner test's waiting condition check to force garbage collection before validating the reset condition, addressing timing-related test flakiness from data race conditions.

Changes

Cohort / File(s) Summary
Test Timing Fix
pkg/gctuner/memory_limit_tuner_test.go
Added explicit runtime.GC() call in the waiting condition check to ensure garbage collection occurs before asserting reset behavior, preventing race condition-induced test failures.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~2 minutes

Poem

🐰 A tiny tweak with GC's call,
Makes flaky tests stand proud and tall,
No races now, just clean-swept floors,
The garbage collector opens doors! ✨

🚥 Pre-merge checks | ❌ 5
❌ Failed checks (1 warning, 4 inconclusive)
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.
Title check ❓ Inconclusive The title is vague and does not clearly convey what specific change was made. 'call runtime.GC to reduce flaky' is missing context and doesn't follow the repository's PR title format (pkg: what's changed). Revise the title to follow the format 'pkg/gctuner: fix flaky test by adding runtime.GC call' or similar to be more descriptive and follow repository conventions.
Linked Issues check ❓ Inconclusive The PR closes #10229, but the linked issue #10029 appears to address a data race that could cause panics, which is unrelated to the test flakiness fix described in this PR's objectives. Clarify the relationship between the test flakiness fix and the data race issue, or verify that the correct issue number is referenced.
Out of Scope Changes check ❓ Inconclusive The change (adding runtime.GC call to a test) appears to be within scope for reducing test flakiness, though the connection to the linked data race issue is unclear. Verify that fixing test flakiness is the intended scope and properly document how this relates to the linked issue objectives.
Description check ❓ Inconclusive The PR description provides an issue reference and brief explanation, but lacks detail on test coverage, impact assessment, and commit message documentation. Clarify which tests were run to verify the fix, explain why runtime.GC() directly addresses the data race, and provide a more detailed commit message explaining the root cause and solution.

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

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

@lance6716
Copy link
Contributor Author

/cc @HunDunDM @okJiang

@ti-chi-bot ti-chi-bot bot requested review from HunDunDM and okJiang February 9, 2026 05:50
Copy link

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 aims to reduce flakiness in the gctuner memory limit tuner tests by explicitly triggering Go GC rather than relying on nondeterministic runtime GC timing.

Changes:

  • Add an explicit runtime.GC() call inside a require.Eventually loop in TestGlobalMemoryTuner to make the test less timing-dependent.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 98 to +99
require.Eventually(t, func() bool {
runtime.GC()
Copy link

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

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

Calling runtime.GC() on every require.Eventually poll can trigger up to ~50 full GCs (5s / 100ms), which can noticeably slow CI and add noise to this test. Consider invoking GC at most once (e.g., before entering the Eventually) or gating it so it only runs once after the tuner indicates the next GC would be memory-limit related (e.g., after nextGCTriggeredByMemoryLimit becomes true).

Suggested change
require.Eventually(t, func() bool {
runtime.GC()
runtime.GC()
require.Eventually(t, func() bool {

Copilot uses AI. Check for mistakes.
@ti-chi-bot
Copy link
Contributor

ti-chi-bot bot commented Feb 9, 2026

@lance6716: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-unit-test-next-gen-3 8c84ac7 link true /test pull-unit-test-next-gen-3

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dco-signoff: yes Indicates the PR's author has signed the dco. release-note-none Denotes a PR that doesn't merit a release note. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

TestGlobalMemoryTuner is flaky

1 participant