Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix check invalid binds #16958

Merged
merged 1 commit into from
Jun 18, 2024
Merged

fix check invalid binds #16958

merged 1 commit into from
Jun 18, 2024

Conversation

iamlinjunhong
Copy link
Contributor

@iamlinjunhong iamlinjunhong commented Jun 18, 2024

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 #https://github.com/matrixorigin/MO-Cloud/issues/3490 #https://github.com/matrixorigin/MO-Cloud/issues/3530
#https://github.com/matrixorigin/MO-Cloud/issues/3521 #https://github.com/matrixorigin/MO-Cloud/issues/3512

What this PR does / why we need it:

fix check invalid binds


PR Type

Bug fix, Tests


Description

  • Added a nil check before calling isRetryError in the checkInvalidBinds method to prevent potential errors.
  • Updated TestRetryValidateService to include a nil check before calling isRetryError.
  • Added a new test TestValidateService to validate service responses and ensure correct behavior.

Changes walkthrough 📝

Relevant files
Bug fix
lock_table_allocator.go
Add nil check before retry error validation                           

pkg/lockservice/lock_table_allocator.go

  • Added a nil check before calling isRetryError in checkInvalidBinds
    method.
  • +1/-1     
    Tests
    rpc_test.go
    Update and add tests for service validation                           

    pkg/lockservice/rpc_test.go

  • Updated TestRetryValidateService to include a nil check before calling
    isRetryError.
  • Added a new test TestValidateService to validate service responses.
  • +29/-6   

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Copy link

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review [1-5] 2
    🧪 Relevant tests Yes
    🔒 Security concerns No
    ⚡ Key issues to review None

    @matrix-meow matrix-meow added the size/S Denotes a PR that changes [10,99] lines label Jun 18, 2024
    @mergify mergify bot requested a review from sukki37 June 18, 2024 03:16
    Copy link

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add timeout context to service validation calls in tests to prevent hanging

    Add a timeout context to the validateService calls in TestValidateService to ensure the
    tests do not hang indefinitely if the service does not respond.

    pkg/lockservice/rpc_test.go [291-297]

    -valid, err := validateService(time.Millisecond*100, "UNKNOWN", c)
    +ctx, cancel := context.WithTimeout(context.Background(), time.Millisecond*100)
    +defer cancel()
    +valid, err := validateService(ctx, "UNKNOWN", c)
     require.False(t, err != nil && isRetryError(err))
     require.True(t, !valid)
     
    -valid, err = validateService(time.Millisecond*100, "s1", c)
    +ctx, cancel = context.WithTimeout(context.Background(), time.Millisecond*100)
    +defer cancel()
    +valid, err = validateService(ctx, "s1", c)
     require.False(t, err != nil && isRetryError(err))
     require.False(t, !valid)
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Adding a timeout context is crucial to prevent tests from hanging, which is a significant improvement in test reliability and performance.

    8
    Add a test case to handle unexpected errors in validateService

    Add a test case to handle the scenario where validateService returns an unexpected error
    that is not a retry error.

    pkg/lockservice/rpc_test.go [291-293]

     valid, err := validateService(time.Millisecond*100, "UNKNOWN", c)
    +if err != nil && !isRetryError(err) {
    +    t.Fatalf("unexpected error: %v", err)
    +}
     require.False(t, err != nil && isRetryError(err))
     require.True(t, !valid)
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Adding a test case for unexpected errors is a good practice to ensure the robustness of the service validation logic, especially in edge cases not covered by current tests.

    7
    Best practice
    Use assert instead of require for non-critical assertions to allow the test suite to continue running

    Use assert instead of require for non-critical test assertions to allow the test suite to
    continue running even if one assertion fails.

    pkg/lockservice/rpc_test.go [292-297]

    -require.False(t, err != nil && isRetryError(err))
    -require.True(t, !valid)
    +assert.False(t, err != nil && isRetryError(err))
    +assert.True(t, !valid)
     
    -require.False(t, err != nil && isRetryError(err))
    -require.False(t, !valid)
    +assert.False(t, err != nil && isRetryError(err))
    +assert.False(t, !valid)
     
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: Changing from require to assert allows the test suite to continue even if these assertions fail, which can be useful for identifying multiple failing conditions in a single test run.

    6

    @mergify mergify bot merged commit 820b417 into matrixorigin:1.2-dev Jun 18, 2024
    18 checks passed
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    Bug fix Review effort [1-5]: 2 size/S Denotes a PR that changes [10,99] lines Tests
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    4 participants