Skip to content

branch-4.1: [fix](test) stabilize PruneOlapScanTabletTest mocks#65768

Draft
shuke987 wants to merge 1 commit into
apache:branch-4.1from
shuke987:agent/stabilize-prune-olap-scan-tablet-test
Draft

branch-4.1: [fix](test) stabilize PruneOlapScanTabletTest mocks#65768
shuke987 wants to merge 1 commit into
apache:branch-4.1from
shuke987:agent/stabilize-prune-olap-scan-tablet-test

Conversation

@shuke987

Copy link
Copy Markdown
Collaborator

What problem does this PR solve?

Issue Number: N/A

Related PR: N/A

Problem Summary:

PruneOlapScanTabletTest extends SqlTestBase, which starts a live FE, but the
test used method-parameter @Mocked objects for four core catalog types. JMockit
therefore mocked every instance of those types in the test JVM, including live FE
objects used by background threads. This made expectation recording/replay race
with FE activity and caused intermittent default results or missing invocations
before the tablet-pruning assertion. One example is the null table name failure
in FE UT build 998284.

This change replaces the four type-wide mocks with instance-scoped @Injectable
mocks. The expectations, pruning rule, and 19-tablet assertion are unchanged.

Release note

None

Check List (For Author)

  • Test

    • Regression test
    • Unit Test
    • Manual test (add detailed scripts or steps below)
    • No need to test or manual test. Explain why:
      • This is a refactor/code format and no logic has been changed.
      • Previous test can cover this change.
      • No code files have been changed.
      • Other reason

    Exact FE UT class passed in three independent runs (6/6 tests total):

    mvn -pl fe-core -am test -Dcheckstyle.skip=true \
        -DfailIfNoTests=false \
        -Dtest=org.apache.doris.nereids.rules.rewrite.PruneOlapScanTabletTest
    
  • Behavior changed:

    • No.
    • Yes.
  • Does this need documentation?

    • No.
    • Yes.

Check List (For Reviewer who merge this PR)

  • Confirm the release note
  • Confirm test cases
  • Confirm document
  • Add branch pick label

@hello-stephen

Copy link
Copy Markdown
Contributor

Thank you for your contribution to Apache Doris.
Don't know what should be done next? See How to process your PR.

Please clearly describe your PR:

  1. What problem was fixed (it's best to include specific error reporting information). How it was fixed.
  2. Which behaviors were modified. What was the previous behavior, what is it now, why was it modified, and what possible impacts might there be.
  3. What features were added. Why was this function added?
  4. Which code was refactored and why was this part of the code refactored?
  5. Which functions were optimized and what is the difference before and after the optimization?

@shuke987

Copy link
Copy Markdown
Collaborator Author

/review

@shuke987

Copy link
Copy Markdown
Collaborator Author

run buildall

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall opinion: approve. I found no blocking or actionable issues in this test-only change.

Critical checkpoints:

  • Correctness: @Injectable preserves the exact four mocked instances used by LogicalOlapScan, PruneOlapScanTablet, and HashDistributionPruner; the unchanged assertion still exercises the 19-tablet pruning result.
  • Lifecycle and isolation: unlike type-wide @Mocked, JMockit's instance-scoped injectable mocks leave the live FE's unrelated catalog objects untouched, directly addressing the reported race.
  • Framework compatibility: the FE module runs JMockit 1.49 as a Surefire javaagent, JMockit supports annotated JUnit 5 method parameters, and the repository has same-lifecycle method-parameter @Injectable precedents.
  • Tests and validation: the author reports three independent runs of this test class; the functional assertions are unchanged; git diff --check passes. Per the review-runner contract, I did not run a build. The aggregate FE UT check was still pending when reviewed.
  • Compatibility, performance, and documentation: only test annotations/imports change, so there is no production behavior, compatibility, performance, or documentation impact.
  • User focus: no additional review focus was supplied.

The main scan plus independent full and risk-focused review lanes found no new valuable findings, and all initial risk points were dismissed with concrete code and framework evidence.

@hello-stephen

Copy link
Copy Markdown
Contributor

FE UT Coverage Report

Increment line coverage `` 🎉
Increment coverage report
Complete coverage report

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants