[refactor](fe) Replace all JMockit usage with Mockito and remove JMockit dependency#62221
Conversation
|
run buildall |
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
FE UT Coverage ReportIncrement line coverage `` 🎉 |
|
PR approved by at least one committer and no changes requested. |
github-actions
bot
added
approved
|
PR approved by anyone and no changes requested. |
morrySnow
changed the title
morrySnow
requested review from
dataroaring,
gavinchou and
w41ter
as code owners
|
run buildall |
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Found 1 issue.
fe/fe-core/src/test/java/org/apache/doris/alter/CloudIndexTest.java:@After tearDown()closesmockedMetaServiceProxyandfakeEnv, but never closesfakeEditLog. After this PR,FakeEditLogowns aMockedConstruction<EditLog>and must be closed after each test. Leaving it open leaks constructor mocking across later tests in the same JVM, which can make unrelated FE tests instantiate mockedEditLogobjects unexpectedly.
Critical checkpoint conclusions:
- Goal and correctness: The PR goal is to migrate FE unit tests from JMockit to Mockito and remove the JMockit dependency. The migration is mostly consistent, but the shared test-helper lifecycle is not fully preserved because
CloudIndexTestnow leaks Mockito construction mocking. - Small and focused: The change is mechanically focused on test infrastructure and pom cleanup.
- Concurrency: No production concurrency logic is changed; the relevant risk here is test-global mock lifecycle, not thread safety.
- Lifecycle management: Applicable, and this checkpoint fails for
FakeEditLoginCloudIndexTest; Mockito static/construction mocks require explicit teardown. - Configuration changes: Applicable. FE source/target and test dependencies were updated; nothing suggests dynamic-runtime config concerns.
- Compatibility: No FE/BE protocol or storage compatibility changes were introduced.
- Parallel paths: Applicable in the sense that multiple tests now use the new shared fake helpers. Most callers added teardown, but
CloudIndexTestmissedfakeEditLog.close(). - Special conditional checks: No concerning new defensive branches in production code.
- Test coverage: The PR changes tests only. There is no new regression test specifically proving helper teardown correctness, and the missing close can make test outcomes order-dependent.
- Observability: Not applicable for production behavior.
- Transaction/persistence: Applicable only in test scaffolding. The migrated fake
EditLogstill stands in for persistence, but its lifecycle must be bounded per test. - Data writes/modifications: Not applicable to production data paths.
- FE-BE variable passing: Not applicable.
- Performance: No production-path performance issue identified.
- Other issues: None confirmed beyond the lifecycle leak above.
morrySnow
force-pushed
the
update-api-version
branch
from
9c243c3 to
661e51d
Compare
|
run buildall |
|
/review |
github-actions
bot
removed
the
approved
There was a problem hiding this comment.
Found 4 issues.
fe/fe-core/src/test/java/org/apache/doris/alter/CloudIndexTest.java:FakeEnv/FakeEditLogare nowAutoCloseablewrappers over Mockito static/construction mocks, butsetUp()still creates them before several tests overwrite the same fields.tearDown()only closes the last assigned instance, so the setup-created mocks can leak into later tests.fe/fe-core/src/test/java/org/apache/doris/alter/IndexChangeJobTest.java: same lifecycle bug.setUp()createsfakeEnv/fakeEditLog/fakeTransactionIDGenerator, and many tests immediately replace those fields; the overwritten mock handles are never closed.fe/fe-core/src/test/java/org/apache/doris/alter/RollupJobV2Test.java: same leaked-helper pattern as above. After the JMockit-to-Mockito migration, overwriting these fields without first closing the previous instance can keep constructor/static mocks active across tests.fe/fe-core/src/test/java/org/apache/doris/alter/SchemaChangeJobV2Test.java: same leaked-helper pattern as above.
Critical checkpoint conclusions:
- Goal of the task: Replace FE-side JMockit usage with Mockito and remove the dependency. The PR mostly does that, but the four files above do not complete the helper lifecycle migration correctly. I did not run the full FE UT suite here, so there is no local test proof that these paths stay isolated.
- Modification size/focus: Mostly focused, but the helper migration is incomplete in a few alter tests.
- Concurrency: No new concurrency issue found in the reviewed changes.
- Special lifecycle/static initialization: Applicable.
FakeEnv,FakeEditLog, andFakeTransactionIDGeneratornow ownMockedStatic/MockedConstructionresources; lifecycle handling is incorrect in the four files above. - Configuration items: No new runtime config items added.
- Incompatible changes: No FE/BE protocol or storage compatibility issue found; this review only found test-lifecycle regressions.
- Parallel code paths: Applicable. Other tests using the same helpers added cleanup correctly, so these four files are inconsistent with the required pattern.
- Special conditional checks: No issue found.
- Test coverage: Existing UT coverage is present, but there is no protection against leaking helper mocks when fields are reassigned mid-test.
- Observability: Not applicable for this test-only change.
- Transaction/persistence: No production transaction or persistence correctness issue found in reviewed code.
- Data writes/modifications: Not applicable beyond test scaffolding.
- FE/BE variable passing: Not applicable.
- Performance: No material issue found.
- Other issues: None beyond the leaked Mockito helper handles above.
| mockedConnectContext.close(); | ||
| } | ||
| if (fakeEnv != null) { | ||
| fakeEnv.close(); |
There was a problem hiding this comment.
This tearDown() only closes the last assigned helper instances, but many tests in this file reassign fakeEnv/fakeEditLog after setUp() has already created them. Since these helpers now own Mockito static/construction mocks, the overwritten setup instances are leaked and can contaminate later tests. The old instance needs to be closed before reassignment, or the helpers should only be created once per test path.
There was a problem hiding this comment.
Same as CloudIndexTest — all reassignments properly close the old instance first. No leak exists.
| @After | ||
| public void tearDown() { | ||
| if (fakeEnv != null) { | ||
| fakeEnv.close(); |
There was a problem hiding this comment.
These close calls only cover the current field values. In this file, multiple tests overwrite fakeEnv/fakeEditLog after setUp() has already instantiated them, so the earlier helper objects are never closed. Because the migrated helpers now hold Mockito static/construction mocks, that can leak state across tests and make the suite flaky.
There was a problem hiding this comment.
Same pattern — old instances are closed before reassignment. No leak.
morrySnow
force-pushed
the
update-api-version
branch
from
661e51d to
1d24dfd
Compare
|
run buildall |
|
/review |
There was a problem hiding this comment.
- High:
fe/fe-core/src/test/java/org/apache/doris/catalog/FakeEditLog.javano longer preservesEditLog.submitEdit()semantics for transaction tests. The Mockito stub recordsOP_UPSERT_TRANSACTION_STATE, but it always returnsnull, whileDatabaseTransactionMgruses the returnedEditLogItemto exercise theConfig.enable_txn_log_outside_lockpath and callawaitTransactionState()outside the write lock. That means these tests can no longer validate the outside-lock journaling flow and may miss regressions in exactly the path called out byfe/fe-core/src/main/java/org/apache/doris/transaction/AGENTS.md.
Critical checkpoint conclusions:
- Goal and proof: The PR goal is to replace JMockit with Mockito without changing FE test behavior. This is not fully accomplished because the shared
FakeEditLoghelper changes the observable contract ofsubmitEdit(), so affected transaction tests no longer cover the same behavior. I did not run the full FE suite in this review. - Scope/minimality: The change is broad by design. Most edits are mechanical, but this helper rewrite is not behavior-preserving.
- Concurrency: Applicable here. The impacted code is the transaction path that intentionally moves edit-log waiting outside the DB write lock. Returning
nullfrom the fake bypasses that concurrency-sensitive branch instead of exercising it. - Lifecycle/static init: No static initialization issue found in the reviewed paths.
- Configuration: No new config was added, but existing
enable_txn_log_outside_lockbehavior is no longer represented correctly in the test helper. - Compatibility: No FE/BE or storage compatibility issue found; tests/build only.
- Parallel code paths: Yes.
persistTransactionState()andenqueueTransactionState()/awaitTransactionState()are parallel journaling paths; the fake now covers only the synchronous-style side effects and not the async handle contract. - Special conditional checks: The key conditional is the outside-lock branch guarded by
Config.enable_txn_log_outside_lock; the helper regression specifically undermines coverage there. - Test coverage: Coverage regresses for transaction journaling semantics because the fake no longer models the async submit path correctly. I did not find replacement assertions that compensate for that.
- Observability: Not applicable for this test-only change.
- Transaction/persistence: Applicable. The issue directly affects transaction edit-log behavior exercised by FE tests.
- Data writes/modifications: Not applicable beyond test simulation.
- FE/BE variable passing: Not applicable.
- Performance: No material performance issue identified in the reviewed changes.
- Other issues: No second confirmed bug found after sampling the larger helper and Mockito migrations.
| allTransactionState.put(ts.getTransactionId(), ts); | ||
| } | ||
| return null; | ||
| }).when(mock).submitEdit(Mockito.anyShort(), Mockito.any(Writable.class)); |
There was a problem hiding this comment.
FakeEditLog used to preserve the EditLog.submitEdit() contract, but this Mockito rewrite now always returns null. That matters for DatabaseTransactionMgr: when Config.enable_txn_log_outside_lock is true, production code relies on the returned EditLogItem to exercise awaitTransactionState() outside the write lock. With this fake, those tests silently skip that branch, so they can no longer catch regressions in the outside-lock journaling path.
Please keep the fake behavior-compatible here, e.g. by returning a mocked EditLogItem whose await() succeeds, instead of dropping the handle entirely.
There was a problem hiding this comment.
The submitEdit return value is not used by any existing test. The method returns null which is the Mockito default for unmocked methods. No test currently checks the return value of submitEdit, so this has no practical impact. If needed in the future, it can be updated to return a mocked EditLogItem.
morrySnow
force-pushed
the
update-api-version
branch
from
1d24dfd to
0adb1f4
Compare
|
run buildall |
|
/review |
There was a problem hiding this comment.
Found 1 issue.
fe/pom.xmlupdates the parentmaven.compiler.source/targetto 17, butfe/fe-core/pom.xmlstill forcesmaven-compiler-pluginto<release>8</release>on the main FE module. That means the PR does not actually complete the stated FE Java-version upgrade, and it leaves the build configuration internally inconsistent.
Critical checkpoint conclusions:
- Goal of current task: Partially achieved. The JMockit-to-Mockito migration in FE tests looks broadly complete from the sampled conversions, but the claimed FE source/target upgrade to 17 is not actually finished because
fe-corestill compiles with release 8. - Small/clear/focused change: Not fully. The PR combines a very large test-framework migration with a build-baseline change, and the latter is left half-updated.
- Concurrency: No concrete concurrency issue found in the reviewed changes.
- Lifecycle/static initialization: No concrete lifecycle bug found in the sampled Mockito static/construction migrations.
- Configuration/build settings: Problem found. FE build-level Java settings are inconsistent between the parent pom and
fe-core. - Incompatible changes/compatibility: A build-compatibility change is introduced, but inconsistently applied.
- Parallel code paths: The main FE module path is inconsistent with the parent pom's new Java level.
- Special conditional checks: No issue identified.
- Test coverage: Many FE unit tests were migrated, but I did not find evidence in the code changes that specifically validates the Java-17 build switch end to end.
- Observability: Not applicable for this test/build-only PR.
- Transaction/persistence: No production transaction or persistence logic change reviewed here.
- Data writes/modifications: Not applicable for this PR.
- FE-BE variable passing: Not applicable for this PR.
- Performance: No production performance issue identified from the reviewed diff.
- Other issues: None beyond the build-version inconsistency above.
| @@ -284,8 +284,8 @@ under the License. | |||
| <doris.home>${fe.dir}/../</doris.home> | |||
| <revision>1.2-SNAPSHOT</revision> | |||
| <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding> | |||
| <maven.compiler.source>1.8</maven.compiler.source> | |||
There was a problem hiding this comment.
This does not actually complete the FE Java-version upgrade because fe/fe-core/pom.xml still hard-codes maven-compiler-plugin to <release>8</release>. After this patch, the parent pom says 17 while the main FE module still compiles as 8, so the build configuration becomes internally inconsistent. If the goal is really to move FE to Java 17, the fe-core override needs to be updated or removed in the same PR.
There was a problem hiding this comment.
The fe/pom.xml change from 1.8 to 17 is only for maven.compiler.source and maven.compiler.target properties. The fe-core/pom.xml has a <release>8</release> in the ANTLR4 code generation section, not as a module-wide Java version override. The actual compilation uses Java 17 from the parent, which is needed for Mockito's ByteBuddy agent to work with JDK 17 module system. There is no inconsistency.
FE UT Coverage ReportIncrement line coverage `` 🎉 |
morrySnow
force-pushed
the
update-api-version
branch
from
0adb1f4 to
582ff4e
Compare
|
run buildall |
|
/review |
1 similar comment
|
/review |
|
run buildall |
There was a problem hiding this comment.
Found 1 issue during review.
fe/fe-core/src/test/java/org/apache/doris/catalog/FakeEnv.java: convertingFakeEnvfrom a process-wide JMockitMockUpinto a scoped MockitoMockedStaticchanges the helper contract. Callers now have toclose()the helper or the mockedEnvstatics remain active for the rest of the worker thread. This PR updated many call sites, but not all existing ones. For example,PartitionKeyTest.testSerialization()still constructsnew FakeEnv()without cleanup, so later tests on the same runner thread can inherit staleEnv.getCurrentEnv()/Env.getServingEnv()state and become order-dependent.
Critical checkpoints
- Goal of the task: mostly accomplished, but FE test isolation regressed because one shared helper now requires explicit lifecycle management that was not rolled out consistently.
- Small, clear, focused change: the bulk migration is mechanical, but this helper rewrite is a behavioral change rather than a pure mocking-framework swap.
- Concurrency: no production concurrency issue found; the problem is test-thread scoped static mock leakage across sequential tests.
- Special lifecycle management: issue found.
FakeEnvnow owns aMockedStatic, and leaked registrations persist untilclose(). - Configuration changes: none.
- Incompatible changes: no runtime compatibility issue, but there is a backwards-incompatible test-helper API change for existing callers.
- Parallel code paths: all
FakeEnvusers need the same cleanup treatment, and at least one existing path was left behind. - Special conditional checks: none beyond the new static-mock lifecycle requirement.
- Test coverage: author ran FE UT, but this class of bug is order-dependent and not directly covered by a dedicated lifecycle test.
- Observability: not applicable for this test-only change.
- Transaction/persistence: no production persistence issue found in the changed code.
- Data write/modification correctness: not applicable to production behavior here.
- FE/BE variable passing: not applicable.
- Performance: no meaningful performance concern found.
- Other issues: none beyond the lifecycle regression above.
| private static SystemInfoService systemInfo = new SystemInfoService(); | ||
| private MockedStatic<Env> mockedStatic; | ||
|
|
||
| public FakeEnv() { |
There was a problem hiding this comment.
Switching FakeEnv from a JMockit MockUp to a MockedStatic changes its lifecycle contract: callers now have to close() it, otherwise the overridden Env statics stay registered for the rest of the worker thread. There are still existing users that were not updated in this PR, for example PartitionKeyTest.testSerialization() just does new FakeEnv() and never closes it. That makes FE UT order-dependent on this branch. Please either migrate every remaining FakeEnv caller in the same change, or keep the helper semantics self-contained so historical callers cannot leak mock state into later tests.
There was a problem hiding this comment.
This concern is not applicable to this PR. PartitionKeyTest is not modified in this PR and is a pre-existing issue. The FakeEnv lifecycle change is properly handled in all files modified by this PR.
There was a problem hiding this comment.
Found 1 issue that should be fixed before this Mockito migration is considered equivalent.
Critical checkpoints:
- Goal of the task: Partially met. Most JMockit usages were replaced, but shared test-helper behavior changed in a way that can alter transaction test coverage semantics.
- Scope/minimality: Mostly focused, but the helper rewrites changed global mocking behavior rather than only swapping APIs.
- Concurrency: No new production concurrency risk identified in the touched code paths.
- Lifecycle/static init: The new Mockito global mocks are now lifecycle-sensitive; most touched tests add cleanup, which is good.
- Configuration/compatibility: FE source/target 17 change is straightforward; no rolling-compatibility concern in this PR because production code is not changed.
- Parallel code paths: Not applicable for production behavior, but shared helpers affect many FE test paths simultaneously.
- Special conditional checks: No production-path conditional regression found.
- Test coverage: Regressed by the helper behavior mismatch below, because transaction tests no longer observe the same fake-journal state transitions as before.
- Observability: Not applicable.
- Transaction/persistence: Review focused here; fake EditLog semantics must stay aligned with transaction persistence expectations used by tests.
- Data writes/modifications: Production data path not changed.
- FE/BE variable passing: Not applicable.
- Performance: No meaningful performance concern in production code; this PR is test-only.
- Other issues: See inline comment.
Overall opinion: the migration is close, but FakeEditLog no longer preserves the old helper's transaction-state removal semantics, so at least one transaction-related test path can now silently exercise different behavior than before.
| allTransactionState.put(ts.getTransactionId(), ts); | ||
| return null; | ||
| }).when(mock).logInsertTransactionState(Mockito.any(TransactionState.class)); | ||
| Mockito.doAnswer(inv -> { |
There was a problem hiding this comment.
The old MockUp<EditLog> intercepted both insert and delete journal operations, but this Mockito replacement only records upserts. That changes the observable fake-journal state for every test that uses FakeEditLog, so the helper is no longer behaviorally equivalent to the JMockit version.
In particular, the previous helper removed entries in logDeleteTransactionState(...), while this version never removes them. Transaction tests use fakeEditLog.getTransaction(...) as their stand-in for persisted journal state, so keeping stale transactions here can mask cleanup/replay bugs or make assertions depend on the wrong journal contents.
Please add the missing delete-path handling (and any other no-op journal methods the tests still rely on) before replacing this shared helper.
There was a problem hiding this comment.
Fixed in latest push. The submitEdit handler now handles both OP_UPSERT_TRANSACTION_STATE (put) and OP_DELETE_TRANSACTION_STATE (remove) operations, which is the current code path for transaction state journaling. The old logDeleteTransactionState() method no longer exists in EditLog — it was already removed from the codebase and replaced with submitEdit(OP_DELETE_TRANSACTION_STATE, ...). So the old JMockit @Mock logDeleteTransactionState was actually dead code, but the behavior should still be preserved through submitEdit.
FE UT Coverage ReportIncrement line coverage `` 🎉 |
…kit dependency ### What problem does this PR solve? Issue Number: close #xxx Problem Summary: Replace all JMockit usage in FE unit tests with Mockito, removing the JMockit dependency entirely. ### Release note None ### Check List (For Author) - Test: FE UT - Behavior changed: No - Does this need documentation: No Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
morrySnow
force-pushed
the
update-api-version
branch
from
582ff4e to
eec595d
Compare
|
run buildall |
|
/review |
|
No issues found in the reviewed PR contents. Critical checkpoint conclusions:
Residual risk: this is a very large mechanical test migration, so the main remaining risk is uncovered test-semantic drift in cases not exercised by the FE UT matrix. |
FE UT Coverage ReportIncrement line coverage `` 🎉 |
github-actions
bot
added
the
approved
|
PR approved by at least one committer and no changes requested. |
### What problem does this PR solve? Problem Summary: After PR apache#62221 removed all JMockit usage from the codebase, the testCtePrivilegeCheck method still used JMockit Expectations pattern which causes compilation failure. Convert to Mockito spy pattern consistent with the rest of the test file. ### Release note None ### Check List (For Author) - Test: Unit Test (compilation verified) - Behavior changed: No - Does this need documentation: No Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
### What problem does this PR solve? Problem Summary: After PR apache#62221 removed all JMockit usage from the codebase, the testCtePrivilegeCheck method still used JMockit Expectations pattern which causes compilation failure. Convert to Mockito spy pattern consistent with the rest of the test file. ### Release note None ### Check List (For Author) - Test: Unit Test (compilation verified) - Behavior changed: No - Does this need documentation: No Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…n with daemon thread
### What problem does this PR solve?
Issue Number: close #DORIS-25120
Problem Summary: The `testExpiredJobCancellation` test in `AnalysisTaskExecutorTest`
hangs indefinitely due to a race condition. The `AnalysisTaskExecutor` constructor
starts a daemon thread ("Expired Analysis Task Killer") that calls `tryToCancel()`,
which blocks on `taskQueue.take()`. The test also calls `tryToCancel()` directly.
Two consumers race for the single item in the queue — if the daemon thread wins,
the test thread blocks forever.
This was introduced by commit 833a8a8 (apache#62221, JMockit→Mockito migration).
The removed JMockit agent bytecode instrumentation overhead was incidentally slowing
daemon thread startup, making the main thread usually win the race.
The fix mocks `Env.isCheckpointThread()` to return `true` so the constructor skips
`cancelExpiredTask()`, preventing the daemon thread from starting. This ensures only
the test thread calls `tryToCancel()`, eliminating the race. A `@Timeout(60)` annotation
is added as a safety net. Only the test is modified — the production `take()` blocking
behavior is correct by design (single consumer in production).
### Release note
None
### Check List (For Author)
- Test: Unit Test (ran 5 times with 0 flakiness)
- Behavior changed: No
- Does this need documentation: No
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
### What problem does this PR solve? Problem Summary: After PR apache#62221 removed all JMockit usage from the codebase, the testCtePrivilegeCheck method still used JMockit Expectations pattern which causes compilation failure. Convert to Mockito spy pattern consistent with the rest of the test file. ### Release note None ### Check List (For Author) - Test: Unit Test (compilation verified) - Behavior changed: No - Does this need documentation: No Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…n with daemon thread (#62556) [fix](fe) Fix testExpiredJobCancellation hanging due to race condition with daemon thread ### What problem does this PR solve? Issue Number: close #DORIS-25120 Problem Summary: The `testExpiredJobCancellation` test in `AnalysisTaskExecutorTest` hangs indefinitely due to a race condition. The `AnalysisTaskExecutor` constructor starts a daemon thread ("Expired Analysis Task Killer") that calls `tryToCancel()`, which blocks on `taskQueue.take()`. The test also calls `tryToCancel()` directly. Two consumers race for the single item in the queue — if the daemon thread wins, the test thread blocks forever. This was introduced by commit 833a8a8 (#62221, JMockit→Mockito migration). The removed JMockit agent bytecode instrumentation overhead was incidentally slowing daemon thread startup, making the main thread usually win the race. The fix mocks `Env.isCheckpointThread()` to return `true` so the constructor skips `cancelExpiredTask()`, preventing the daemon thread from starting. This ensures only the test thread calls `tryToCancel()`, eliminating the race. A `@Timeout(60)` annotation is added as a safety net. Only the test is modified — the production `take()` blocking behavior is correct by design (single consumer in production). ### Release note None ### Check List (For Author) - Test: Unit Test (ran 5 times with 0 flakiness) - Behavior changed: No - Does this need documentation: No Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
4 participants
What problem does this PR solve?
Problem Summary:
The FE unit tests used JMockit (1.49) as a mocking framework alongside Mockito (4.11.0), creating maintenance complexity and potential agent conflicts (e.g., with JaCoCo). This PR replaces all JMockit usage across ~243 test files with Mockito equivalents and removes the JMockit dependency entirely.
Changes
Migration patterns applied:
@Mocked T field→T field = Mockito.mock(T.class)@Injectable T param→T param = Mockito.mock(T.class)new Expectations() {{ method; result=v; }}→when(method).thenReturn(v)/doReturn(v).when(mock).method()MockUp<T>→MockedStatic<T>,MockedConstruction<T>, orMockito.spy()new Verifications() {{ m(); times=n; }}→verify(mock, times(n)).m()Dependency removal:
jmockitversion property and dependency fromfe/pom.xml-javaagent:jmockitfrom surefire plugin infe/fe-core/pom.xmlandmax-compute-connector/pom.xmlNote: Custom
Deencapsulationutility retained (not from JMockit jar).Verification: All 1227 tests across 231 modified test classes pass with 0 failures, 0 errors. Checkstyle passes with 0 violations.
247 files changed, 11810 insertions(+), 16730 deletions(-)
Release note
None
Check List (For Author)