[feature](tools) Add fast-fe-ut.sh for fast incremental FE unit test running#65055
[feature](tools) Add fast-fe-ut.sh for fast incremental FE unit test running#65055englefly wants to merge 1 commit into
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
englefly
force-pushed
the
fast-fe-ut
branch
2 times, most recently
from
a5e6eda to
a5abfc5
Compare
|
/review |
|
run buildall |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated review result: requesting changes.
I found two blocking issues in the new fast FE UT helper: the fast path can report success while running stale compiled/generated artifacts, and the new executable script currently fails the repository ShellCheck workflow.
Checkpoint conclusions: the goal is clear and the patch is small, but the implementation does not safely preserve FE unit-test semantics. There is no concurrency or persistence impact. The relevant lifecycle/config issue is that EXTRA_FE_MODULES, upstream reactor modules, resources, and Nereids generate-sources work are not refreshed before direct Surefire execution. The parallel run-fe-ut.sh path still uses Maven test, so this shortcut diverges from the established path. Validation performed: bash -n tools/fast-fe-ut.sh passed, shellcheck tools/fast-fe-ut.sh failed with SC2034, GitHub live patch scope is only tools/fast-fe-ut.sh. Full script execution was not possible in this runner because .worktree_initialized, thirdparty/installed, output/, and fe-core/target/classes are missing.
User focus: no additional user-provided focus points.
Subagent conclusions: OR-1 and TSC-001 were verified and merged into the first inline comment; TSC-002 was verified as the second inline comment. Convergence round 1 ended with both live subagents reporting NO_NEW_VALUABLE_FINDINGS for the same final two-comment set.
There was a problem hiding this comment.
Automated review result: requesting changes.
I found two blocking issues in the new fast FE UT helper: the fast path can report success while running stale compiled/generated artifacts, and the new executable script currently fails the repository ShellCheck workflow.
Checkpoint conclusions: the goal is clear and the patch is small, but the implementation does not safely preserve FE unit-test semantics. There is no concurrency or persistence impact. The relevant lifecycle/config issue is that EXTRA_FE_MODULES, upstream reactor modules, resources, and Nereids generate-sources work are not refreshed before direct Surefire execution. The parallel run-fe-ut.sh path still uses Maven test, so this shortcut diverges from the established path. Validation performed: bash -n tools/fast-fe-ut.sh passed, shellcheck tools/fast-fe-ut.sh failed with SC2034, GitHub live patch scope is only tools/fast-fe-ut.sh. Full script execution was not possible in this runner because .worktree_initialized, thirdparty/installed, output/, and fe-core/target/classes are missing.
User focus: no additional user-provided focus points.
Subagent conclusions: OR-1 and TSC-001 were verified and merged into the first inline comment; TSC-002 was verified as the second inline comment. Convergence round 1 found no new valuable findings; the first submission used diff positions that landed one line low, so those comments were deleted. Convergence round 2 ended with both live subagents reporting NO_NEW_VALUABLE_FINDINGS for the repaired exact-line anchor set.
|
/review |
There was a problem hiding this comment.
Reviewed PR 65055 at head 9a374fd.
Requesting changes for one new issue: the fast FE unit-test runner invokes Surefire without disabling the FE Maven build cache, so it can restore cached test results instead of executing the requested tests.
Critical checkpoints:
- Goal/test proof: The PR adds a focused fast FE UT helper.
bash -n tools/fast-fe-ut.shandshellcheck tools/fast-fe-ut.shpass, but the test-execution contract is weakened by the accepted cache issue. - Scope: The change is limited to one shell helper and mostly follows
run-fe-ut.shpatterns. - Concurrency/lifecycle/config: No runtime concurrency or Doris service lifecycle is involved. The skipped Maven lifecycle/generated-artifact concern is already covered by the existing line-222 thread and was not resubmitted. The remaining config issue is Maven build-cache propagation for the direct Surefire path.
- Compatibility/parallel paths:
EXTRA_FE_MODULESnow falls back torun-fe-ut.sh; the direct fast path still needs to matchrun-fe-ut.shby disabling Maven build cache for normal test runs. - Tests/style: Shell syntax and ShellCheck were checked locally. I did not run FE unit tests in this review runner.
- Data/transactions/security/observability: Not applicable for this shell test helper.
Subagent conclusions: optimizer-rewrite reported only OPT-001, which is a duplicate of existing thread 3502844597 and produced no new inline comment. tests-session-config reported TESTS-CONFIG-001, which was verified and accepted as the inline comment below. Convergence round 1 ended with both live subagents replying NO_NEW_VALUABLE_FINDINGS for the same final ledger/comment set.
User focus: No additional user-provided review focus was supplied.
…running fast-fe-ut.sh skips Maven's compile lifecycle by using fast-compile-fe.sh for incremental compilation, then runs tests directly via mvn surefire:test. If fast compilation fails, it falls back to run-fe-ut.sh. Usage: ./fast-fe-ut.sh # Run all UT ./fast-fe-ut.sh org.apache.doris.common.CidrTest # Run specific test Feature: - Positional test spec (same as run-fe-ut.sh --run) - Incremental compile (main + test) via fast-compile-fe.sh - Runs tests via mvn surefire:test (no Maven compile phase) - Falls back to run-fe-ut.sh --run <spec> on compilation failure Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
/review |
There was a problem hiding this comment.
Reviewed PR 65055 at head 9ee434f.
Requesting changes for one new issue: the new helper can misparse the established run-fe-ut.sh --run <test> invocation shape and drop the requested test class.
Critical checkpoint conclusions:
- Goal/test proof: The PR adds a focused fast FE unit-test helper.
bash -n tools/fast-fe-ut.shandshellcheck tools/fast-fe-ut.shpass, but argument handling can run the wrong requested test selector. Full helper execution was not run because this checkout lacks.worktree_initialized,thirdparty/installed,output/, andfe-core/target/classes. - Scope: The change is limited to one shell helper, and the accepted issue is in that helper's argument/fallback path.
- Concurrency, persistence, data correctness, observability, and security: Not applicable for this shell test runner.
- Lifecycle/config/parallel paths: The stale generated-artifact/lifecycle concern remains duplicate-covered by existing thread
discussion_r3502844597; the current head resolves the previous unused-variable ShellCheck issue and the Maven build-cache issue. - Compatibility: The established
run-fe-ut.sh --run <test>shape is documented in the parallel runner, but this new shortcut treats--runitself as the test selector and ignores the real test class. - Tests/style: Shell syntax and ShellCheck were checked locally. No additional user-provided focus points were supplied.
Subagent conclusions: optimizer-rewrite found no new optimizer/rewrite candidate and treated the Nereids generated-pattern limitation as duplicate of discussion_r3502844597. tests-session-config proposed TSC-1, which was verified and accepted as the inline comment below. Convergence round 1 ended with both live subagents replying NO_NEW_VALUABLE_FINDINGS for the same final one-comment set.
|
|
||
| # ─── Argument parsing ───────────────────────────────────────────────────────── | ||
|
|
||
| TEST_SPEC="" |
There was a problem hiding this comment.
This parser still accepts any first positional argument as the Surefire test selector and silently ignores the rest. That makes the familiar run-fe-ut.sh --run <class> shape dangerous here: tools/fast-fe-ut.sh --run org.apache.doris.common.CidrTest sets TEST_SPEC to --run, drops the actual class name, and eventually invokes Surefire with -Dtest=--run; if the fast compile path falls back, it also builds run-fe-ut.sh --run --run. Since this helper is explicitly a shortcut for FE UTs and forwards to run-fe-ut.sh on fallback, please either reuse the existing run-fe-ut.sh option parsing or reject option-style / extra arguments with usage instead of running a different test selection than the user requested.
2 participants
What problem does this PR solve?
fast-fe-ut.sh skips Maven's compile lifecycle by using fast-compile-fe.sh for incremental compilation, then runs fe-core tests directly via mvn surefire:test. If fast compilation fails, it falls back to run-fe-ut.sh for a full build.
Feature:
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)