search

matrixorigin / matrixone

Hyperconverged cloud-edge native database
https://docs.matrixorigin.cn/en
Apache License 2.0
1.79k stars 277 forks source link

Test for inserting failed ut cases to MOC #20284

Closed Rosyrain closed 6 hours ago

Rosyrain commented 8 hours ago

User description

What type of PR is this?

Which issue(s) this PR fixes:

Test for inserting failed ut cases to MOC issue # https://github.com/matrixorigin/matrixone/issues/19445

What this PR does / why we need it:

Test for inserting failed ut cases to MOC

PR Type

Bug fix, Tests

Description

Changes walkthrough 📝

Relevant files
Tests
config_test.go
Add panic calls for debugging in test functions                   
cmd/mo-service/config_test.go
  • Added panic calls in three test functions for debugging purposes.
  • Inserted comments for testing.
    		•  +7/-0     
    Enhancement
    run_ut.sh
    Improve error handling and test execution in shell script
    optools/run_ut.sh
  • Added set -o pipefail to improve error handling in shell script.
  • Enabled -failfast option in go test command to stop after first
    failure.
    		•  +3/-2     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    codiumai-pr-agent-pro[bot] commented 8 hours ago

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

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    **🎫 Ticket compliance analysis 🔶** **[19445](https://github.com/matrixorigin/matrixone/issues/19445) - Partially compliant** Compliant requirements: - Added set -o pipefail to shell script Non-compliant requirements: - The PR does not directly fix the failing TestDaemonTaskInSqlMock test
    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Test Disruption
    Adding panic calls to test functions will cause tests to fail abruptly and may hide actual issues. These panics should be removed or replaced with proper test assertions. Code Quality
    Empty test comments added without meaningful documentation or explanation of test cases Test Execution
    The -failfast flag will stop test execution after first failure, which may hide other failing tests that need attention
    codiumai-pr-agent-pro[bot] commented 8 hours ago

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

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Remove disruptive panic() calls from test functions to allow proper test execution and failure reporting ___ **Remove panic() calls from test functions as they disrupt normal test execution flow
    and prevent proper test assertions from running. Use t.Fatal() or t.Error() for test
    failures instead.** [cmd/mo-service/config_test.go [75-79]](https://github.com/matrixorigin/matrixone/pull/20284/files#diff-388e064c468fe6c510b58f11c33f3a2323476d93b10513b2cd79bb5fb412981dR75-R79) ```diff assert.Equal(t, 2, len(cfg.getTNServiceConfig().HAKeeper.ClientConfig.ServiceAddresses)) -// test -// test2 -// 3 -panic(1) ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 10 Why: Using panic() in tests is a major issue as it prevents proper test execution and error reporting. The panic calls appear to be debug code that was accidentally committed and should be removed.
    		10
    General
    Remove test execution early termination to ensure all test failures are reported ___ **The -failfast flag will stop test execution on first failure, which may hide other
    test failures and make debugging more difficult. Remove this flag to see all test
    failures.** [optools/run_ut.sh [100]](https://github.com/matrixorigin/matrixone/pull/20284/files#diff-8585259c927720b0c3dfe5cd90e4df60cf6a5fef857db4b85e70241ceba32a4cR100-R100) ```diff -CGO_CFLAGS="-I${BUILD_WKSP}/cgo" CGO_LDFLAGS="-L${BUILD_WKSP}/cgo -lmo" go test -short -v -json -tags matrixone_test -p ${UT_PARALLEL} -timeout "${UT_TIMEOUT}m" -failfast $test_scope | tee $UT_REPORT +CGO_CFLAGS="-I${BUILD_WKSP}/cgo" CGO_LDFLAGS="-L${BUILD_WKSP}/cgo -lmo" go test -short -v -json -tags matrixone_test -p ${UT_PARALLEL} -timeout "${UT_TIMEOUT}m" $test_scope | tee $UT_REPORT ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 8 Why: The -failfast flag prevents discovery of multiple test failures by stopping at the first failure. Removing it would improve test coverage and make debugging easier by revealing all failing tests at once.
    		8

    💡 Need additional feedback ? start a PR chat