Skip to content

Conversation

@LeftHandCold
Copy link
Contributor

@LeftHandCold LeftHandCold commented Mar 27, 2025

User description

What type of PR is this?

  • API-change
  • BUG
  • Improvement
  • Documentation
  • Feature
  • Test and CI
  • Code Refactoring

Which issue(s) this PR fixes:

issue #21606

What this PR does / why we need it:

Fix TestPitrMeta


PR Type

Bug fix, Tests


Description

  • Fixed a bug in the TestPitrMeta test to address CI failures.

  • Removed a call to ForceCheckpoint in the test logic.

  • Ensured proper garbage collection by enabling GC in the test.


Changes walkthrough 📝

Relevant files
Bug fix
db_test.go
Adjusted `TestPitrMeta` test logic to fix CI issues           

pkg/vm/engine/tae/db/test/db_test.go

  • Removed the ForceCheckpoint call in the TestPitrMeta test.
  • Enabled garbage collection by invoking EnableGC on the cleaner.
  • Ensured test checks for pending LSN count and proper cleanup.
  • +0/-1     

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • @qodo-code-review
    Copy link

    Qodo Merge was enabled for this repository. To continue using it, please link your Git account with your Qodo account here.

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ✅

    21606 - PR Code Verified

    Compliant requirements:

    • Fix the CI failure in TestPitrMeta test

    Requires further human verification:

    • Verify that the fix works on both 2.1-dev and main branches

    ⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Test Stability

    The fix removes ForceCheckpoint but relies on EnableGC. Verify this approach is sufficient to resolve the CI failures consistently across different environments.

    db.DiskCleaner.GetCleaner().EnableGC()

    @qodo-code-review
    Copy link

    qodo-code-review bot commented Mar 27, 2025

    Qodo Merge was enabled for this repository. To continue using it, please link your Git account with your Qodo account here.

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Fail test instead of returning

    The test has an early return if pending LSN count is not zero, which will skip
    important assertions and cleanup. This can lead to test flakiness and resource
    leaks. Consider failing the test with a proper error message instead of silently
    returning.

    pkg/vm/engine/tae/db/test/db_test.go [7388-7393]

     testutils.WaitExpect(10000, func() bool {
         return db.Runtime.Scheduler.GetPenddingLSNCnt() == 0
     })
     if db.Runtime.Scheduler.GetPenddingLSNCnt() != 0 {
    -    return
    +    t.Fatalf("Pending LSN count is not zero: %d", db.Runtime.Scheduler.GetPenddingLSNCnt())
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    __

    Why: The suggestion improves test reliability by replacing a silent early return with a proper test failure and error message. This change ensures that test failures are explicitly reported rather than silently skipped, which helps with debugging and prevents potential resource leaks from unfinished tests.

    Medium
    • Update

    @matrix-meow matrix-meow added the size/XS Denotes a PR that changes [1, 9] lines label Mar 27, 2025
    @mergify mergify bot added the kind/test-ci label Mar 27, 2025
    @mergify mergify bot merged commit d3465f5 into matrixorigin:2.1-dev Apr 1, 2025
    18 checks passed
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Labels

    kind/test-ci Review effort 1/5 size/XS Denotes a PR that changes [1, 9] lines

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    3 participants