Repository navigation
[fix](be) Use ConfigValidatorTest fixture consistently - #69009
Merged
suxiaogang223 merged 1 commit intoOct 13, 2026
Merged
suxiaogang223 merged 1 commit into
suxiaogang223 merged 1 commit into
Conversation
### What problem does this PR solve? Issue Number: None Related PR: apache#67228 Problem Summary: PR apache#67228 added a TEST case to the existing ConfigValidatorTest suite, whose other test uses TEST_F. GoogleTest requires every test in one suite to use the same fixture declaration and rejects the mixed TEST/TEST_F suite during registration. Use TEST_F for the added case as well. ### Release note None ### Check List (For Author) - Test: No need to test (one-line GoogleTest fixture declaration correction) - Behavior changed: No - Does this need documentation: No
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Member
Author
|
run buildall |
Member
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review complete for head f4d1a16. No actionable findings or existing blocking inline comments were found.
The one-line change makes LookupConnectionCacheBlockPoolSize use the same ConfigValidatorTest fixture as Validator, correcting the mixed TEST/TEST_F fixture identity. This follows the exact GoogleTest 1.12.1 macro definitions and fixture consistency/lifecycle implementation.
Critical checkpoint conclusions:
- Goal and evidence: the two tests now use the same fixture type; the existing lookup-validator assertions can execute with that fixture. This conclusion is based on static code inspection, not an executed test result.
- Scope: the single macro replacement is small, clear, and focused, reusing the adjacent fixture.
- Concurrency/locking: no new thread or lock is introduced. The standard runner invokes one test binary; fixture hooks execute per test. Existing configuration mutation is serialized and the getter reads its atomic snapshot.
- Lifecycle/static initialization: setup and teardown operate on a directory dedicated to this suite. Missing-directory deletion succeeds, creation makes parents, and each test has independent setup. The filesystem singleton is initialized lazily. No introduced directory collision, ordering/filter dependency, ownership cycle, or static initialization issue was found. The lookup test restores its saved configuration value.
- Configuration: no item is added. Existing mutable configuration validation and update publication match the test's expectations.
- Compatibility: no production function symbol, storage format, rolling-upgrade behavior, or shipped glibc ABI input changes.
- Thrift/protobuf: no definitions change; field-ID compatibility checks are not applicable.
- Parallel paths: both ConfigValidatorTest declarations now use TEST_F; no additional counterpart requiring this correction was found.
- Conditions/error handling: no conditional checks are added. Setup reports filesystem failures through assertions; the unchanged test checks successful and rejected configuration updates.
- Coverage: the existing unit test checks valid endpoints 1 and 32, invalid values 0/-1/33, preservation after rejected updates, and restoration. The file is already included in the BE test target. Additional feature or end-to-end coverage is not needed for this declaration correction.
- Test results: no result/golden files change; existing expectations agree with the validator. Builds and tests were not run, as required by this static-review contract.
- Observability: existing GoogleTest assertions report failures; no new production logs or metrics are needed.
- Transactions/persistence: no transaction, EditLog, failover, or persistent configuration behavior changes.
- Data writes/crashes: no product data-write or crash-atomicity path changes. Fixture filesystem work is limited to its test directory; setup failure precedes test-body mutation, and ordinary EXPECT failures do not bypass final restoration.
- FE-BE propagation: no transmitted variable or send path changes.
- Performance/memory: a few local directory operations per unit test introduce no material performance or memory-accounting concern; production hot paths are unaffected.
- Other checks: no headers/includes, PCH, template instantiations, unity-skip lists, gate tables, or dependencies change. No other substantiated regression was found.
- User focus: no additional focus was supplied; the entire changed-file scope was reviewed.
The main risk scan, full-review pass, separate fixture-lifecycle risk scan, and recovery verification converged in round 1 with NO_NEW_VALUABLE_FINDINGS. All suspicious points have concrete dispositions, and the final changed-file/unresolved-candidate sweep is complete. Current head/base and fresh GitHub reviews/comments were checked immediately before submission. There are no new inline findings or existing P0/P1 IDs to carry forward.
Contributor
TPC-H: Total hot run time: 29014 ms |
Contributor
TPC-DS: Total hot run time: 152109 ms |
Contributor
ClickBench: Total hot run time: 24.52 s |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: None
Related PR: #67228
Problem Summary:
PR #67228 added
LookupConnectionCacheBlockPoolSizewithTESTto the existingConfigValidatorTestsuite, while the suite already usesTEST_F. GoogleTest requires all tests in a suite to use the same fixture declaration and fails during test registration:Change the new case to
TEST_Fso it uses the existing fixture consistently.Release note
None
Check List (For Author)