Skip to content

[fix](test) remove JUnit 4 from the fe reactor and unblock the dependency licence review - #67396

Merged
morningman merged 2 commits into
apache:masterfrom
morningman:ci-dep-review-allow-junit
Sep 5, 2026
Merged

[fix](test) remove JUnit 4 from the fe reactor and unblock the dependency licence review#67396
morningman merged 2 commits into
apache:masterfrom
morningman:ci-dep-review-allow-junit

Conversation

@morningman

@morningmanmorningman commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

What problem does this PR solve?

Issue Number: close #xxx

Related PR: #xxx

Problem Summary:

Dependency License Review fails any pull request that adds a JUnit dependency to a pom — in practice, any pull request that adds a Java module with a test. It happened twice on 2026-09-01 alone, on two unrelated branches:

The following dependencies have incompatible licenses:
fe/be-java-extensions/hive-apache-shade/pom.xml » junit:junit@ – License: EPL-1.0
fe/be-java-extensions/jni-spi/pom.xml » org.junit.jupiter:junit-jupiter@ – License: LicenseRef-bad-non-standard
...

This PR fixes the immediate failure and then removes the thing that made a licence exception necessary in the first place.


1. The workflow exception (third_party_review.yml)

Neither reported licence is one the project has not approved:

  • org.junit.jupiter:junit-jupiter is EPL-2.0, which allow-licenses already carries. GitHub's dependency graph reports it as LicenseRef-bad-non-standard, so the check rejects a licence the project already accepted — a data-quality gap on GitHub's side, not a dependency problem.
  • junit:junit is EPL-1.0, an ASF Category B licence, test scope, and no release artifact ships it.

Both are excluded by purl — the same package-specific shape the existing caniuse-lite exception uses — rather than by widening allow-licenses or dropping development from fail-on-scopes. allow-dependencies-licenses excludes a package from the licence check only, so vulnerability reporting for test-scope dependencies is unchanged.

One detail not in the action's README: it matches a purl on type and name and ignores the version (purlsMatch in its src/purl.ts). The version-less entries added here therefore cover every version, and the version pin on the existing caniuse-lite entry has no effect either.

2. Removing JUnit 4 from the fe reactor

junit:junit is the one EPL-1.0 artifact in this build. Nothing declares it — junit-vintage-engine drags it in, and that engine exists only to run the JUnit 4 tests still in the tree. 450 files, 441 of them in fe-core, were what kept it alive. They are JUnit 5 now.

what movedcount
AssertAssertions~8600 call sites
message argument moved from first to last407
@Test(expected = X.class)assertThrows94 methods
@Rule ExpectedExceptionassertThrows + message substring assertion18 sites, 8 files
@Rule TemporaryFolder@TempDir Path4 files
Assume.assumeTrue(msg, cond)assumeTrue(cond, msg)12 sites
@Before/@After/@BeforeClass/@AfterClass/@Ignoreall
@RunWith(MockitoJUnitRunner)MockitoAnnotations.openMocks1
@FixMethodOrder@TestMethodOrder1
junit.framework.AssertionFailedErrorjava.lang.AssertionError2 files

Two traps the compiler cannot catch, handled explicitly:

  • JUnit 4 puts an assertion message FIRST, JUnit 5 puts it LAST.javac catches the swap for assertTrue/assertFalse/assertNull/assertArrayEquals, but not for assertEquals/assertSame/assertNotEquals when the last argument is itself a String. The 8 three-argument assertEquals calls that are a float delta rather than a message were identified and left alone.
  • Assert.assertEquals(Object[], Object[]) compares arrays; the JUnit 5 assertEquals it would become compares references. One such call was converted to assertArrayEquals.

Three things only running the tests found

  • JdbcSourceOffsetProviderAsyncSplitTest spelled its teardown @org.junit.After, fully qualified — no import, so no import scan could see it. The jupiter engine does not fail on a JUnit 4 annotation, it ignores it: the teardown stopped running, its MockedStatic never closed, and 27 tests died on static mocking is already registered. This is why the checkstyle pattern below matches anywhere on a line and not just an import.
  • StatsCalculatorTest.testFilterOutofRange was annotated @org.junit.Test in a class the jupiter engine already ran — it has never executed. Spelled @Test it runs, and passes.
  • CloudAuthTest extends TestWithFeService, whose setup is driven by JUnit 5 annotations the JUnit 4 engine never saw: no cluster was started and the inherited connectContext was always null, which is what every command in the class was handed. Moving the class to JUnit 5 would have activated that setup for the first time, against a class that mocks Env and ConnectContext statically. The vestigial inheritance is dropped instead, with a comment.

3. The gate

A checkstyle rule, because checkstyle runs at the validate phase with includeTestSourceDirectory: a JUnit 4 import fails a plain mvn testlocally, rather than waiting for CI. The pattern also covers junit.framework.* (how a JUnit 3 import had survived here) and matches fully qualified references, for the reason above.

fe/be-java-extensions is suppressed for now and the suppression says why: #66729 is rewriting those modules, so migrating them here would only conflict. junit-vintage-engine comes out of fe/pom.xml when that lands — not before, because without it a JUnit 4 test is silently not run rather than failed.

Deliberately out of scope: extension/kettle and samples/ are standalone maven projects, outside this reactor and built by no workflow here, so their JUnit 4 cannot be verified from this build. The pkg:maven/junit/junit exception therefore stays.

Release note

None

Check List (For Author)

  • Test
    • Unit Test
    • Manual test (add detailed scripts or steps below)

Workflow change, verified against the real payload rather than by inspection:

  1. The dependency-graph compare API for one of the branches that hit this returns package_urlpkg:maven/junit/junit and pkg:maven/org.junit.jupiter/junit-jupiter — byte-identical to the entries added here, and non-empty, which is what the action requires to exclude a change at all (groupChanges skips the exclusion list when change.package_url.length === 0).
  2. Replaying the action's own filter across all 223 changes in that payload takes the result from the nine incompatible licences the workflow actually reported to zero, with no other dependency newly excluded.
  3. The three purls parse under packageurl-js, the library the action validates them with; the folded YAML value parses to exactly those three comma-separated entries, which parseList trims.

Migration:

  • mvn test -pl fe-common,fe-core -am over every changed test class — 444 classes, 2564 tests, 1 failure and 2 errors. Those three were separated from pre-existing failures by running the same classes against unmodified master, where PropertyAnalyzerTest, ForwardToMasterTest and FileCacheAdmissionRuleRefresherTest fail identically and are untouched by this change.
  • mvn test-compile over the full 76-module reactor with the new checkstyle rule active: green.

The gate, verified from both sides: a probe file carrying org.junit.Test, org.junit.Assert, org.junit.rules.TemporaryFolder and junit.framework.AssertionFailedError is rejected with a message naming the replacement for each and fails the build; the migrated tree passes with 0 checkstyle errors.

  • Behavior changed:

    • No.
  • Does this need documentation?

    • No.

🤖 Generated with Claude Code

https://claude.ai/code/session_01WVPNcULjD4ieAxdLH1WhDr

@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?

@morningman

Copy link
Copy Markdown
ContributorAuthor

run buildall

@morningman
morningmanforce-pushed the ci-dep-review-allow-junit branch from 1b49834 to 11dfe05CompareSeptember 2, 2026 03:05
@morningmanmorningman changed the title [fix](ci) allow the JUnit test dependencies in the dependency license review[fix](test) remove JUnit 4 from the fe reactor and unblock the dependency licence reviewSep 2, 2026
@morningman
morningmanforce-pushed the ci-dep-review-allow-junit branch from 11dfe05 to 9e67622CompareSeptember 2, 2026 03:20
@morningman

Copy link
Copy Markdown
ContributorAuthor

run buildall

CalvinKirs
CalvinKirs previously approved these changes Sep 2, 2026
gavinchou
gavinchou previously approved these changes Sep 2, 2026
morrySnow
morrySnow previously approved these changes Sep 2, 2026
morningmanand others added 2 commits September 4, 2026 15:48
… review
Dependency License Review fails any pull request that adds a JUnit dependency to
a pom - in practice, any pull request that adds a Java module with a test. It
happened twice on 2026-09-01 alone, on two unrelated branches.
Neither failure is about a licence the project has not approved.
- org.junit.jupiter:junit-jupiter is EPL-2.0, which allow-licenses already
carries. GitHub's dependency graph reports its licence as
LicenseRef-bad-non-standard, so the check rejects a licence the project has
already accepted.
- junit:junit is EPL-1.0, an ASF Category B licence. It is test scope, reached by
the JUnit 4 tests that run through junit-vintage-engine, and no release
artifact ships it.
Both are excluded by purl - the same package-specific shape the caniuse-lite
exception already uses - rather than by widening allow-licenses or by dropping
`development` from fail-on-scopes. Vulnerability reporting for test-scope
dependencies is unaffected: allow-dependencies-licenses excludes a package from
the licence check only.
Note that the action matches a purl on type and name and ignores the version
(purlsMatch in its src/purl.ts), so these entries cover every version - and the
version pin on the existing caniuse-lite entry has no effect either.
Tests: verified against the real payload rather than by inspection. The
dependency-graph compare API for one of the branches that hit this returns
package_url "pkg:maven/junit/junit" and
"pkg:maven/org.junit.jupiter/junit-jupiter" - byte-identical to the entries added
here, and non-empty, which is what the action requires to exclude a change at
all. Replaying the action's own filter (purlsMatch over type and name) across
those 223 changes takes the result from the nine incompatible licences the
workflow actually reported to zero, with no other dependency newly excluded. The
three purls parse under packageurl-js, the library the action validates them
with, and the folded YAML value parses to exactly those three entries.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WVPNcULjD4ieAxdLH1WhDr
junit:junit is the one EPL-1.0 artifact in this build, and the reason the
previous commit had to carry a package licence exception at all. Nothing
declares it: junit-vintage-engine drags it in, and junit-vintage-engine is
there only to run the JUnit 4 tests that were still in this tree. 451 files,
442 of them in fe-core, were what kept it alive.
They are JUnit 5 now, and checkstyle keeps them that way.
WHAT MOVED
- Assert -> Assertions, ~8600 call sites. 407 of those carried a message, and
JUnit 4 puts the message FIRST while JUnit 5 puts it LAST, so the argument
had to be moved rather than the call renamed. javac catches the swap for
assertTrue/assertFalse/assertNull/assertArrayEquals but NOT for
assertEquals/assertSame/assertNotEquals when the last argument is itself a
String, so it is done explicitly and the 8 three-argument assertEquals calls
that are a float delta rather than a message were left alone.
- @test(expected = X.class) -> Assertions.assertThrows around the method body,
94 methods.
- @rule ExpectedException -> assertThrows plus a substring assertion on the
message, which is what expectMessage() did, 18 sites in 8 files.
- @rule TemporaryFolder -> @tempdir Path, with newFile/newFolder/getRoot as
Files.createFile/createDirectories/toFile, 4 files.
- @Before/@After/@BeforeClass/@AfterClass/@ignore -> the JUnit 5 spelling.
- @RunWith(MockitoJUnitRunner.class) -> MockitoAnnotations.openMocks(this) in
the setUp that was already there, rather than adding mockito-junit-jupiter
for a single file.
- @FixMethodOrder(NAME_ASCENDING) -> @TestMethodOrder(MethodOrderer.MethodName).
- Assume.assumeTrue(message, condition) -> Assumptions.assumeTrue(condition,
message), 12 sites; same first-argument trap as the assertions.
- junit.framework.AssertionFailedError -> java.lang.AssertionError, its
superclass, in the two helpers that threw it. Nothing catches the type, and
this keeps JUnit 3 out without making opentest4j a direct import.
- Assert.assertEquals(new String[]{...}, arr) -> assertArrayEquals. JUnit 4's
assertEquals(Object[], Object[]) compares arrays; the JUnit 5 assertEquals it
would otherwise have become compares references.
THREE THINGS ONLY RUNNING THE TESTS FOUND
- JdbcSourceOffsetProviderAsyncSplitTest spelled its teardown
@org.junit.After, fully qualified, so it needed no import and no import scan
could see it. The jupiter engine does not fail on a JUnit 4 annotation, it
IGNORES it - the teardown stopped running, its MockedStatic never closed, and
27 tests died on "static mocking is already registered". This is why the
checkstyle pattern below matches anywhere on a line and not just an import.
- StatsCalculatorTest.testFilterOutofRange was annotated @org.junit.Test in a
class the jupiter engine already ran, so it has never executed. Spelled @test
it runs, and passes.
- CloudAuthTest extended TestWithFeService, whose setup is driven by JUnit 5
annotations that the JUnit 4 engine never saw: no cluster was started and the
inherited connectContext was always null, which is what every command in the
class was handed. Moving the class to JUnit 5 would have activated that setup
for the first time, against a class that mocks Env and ConnectContext
statically. The vestigial inheritance is dropped instead, with a comment.
THE GATE
checkstyle, because it runs at the validate phase with
includeTestSourceDirectory: a JUnit 4 import fails a plain `mvn test` locally
rather than waiting for CI. The pattern also covers junit.framework.*, which is
how a JUnit 3 import had survived here, and matches fully qualified references
for the reason above. Verified from both sides - a probe file carrying
org.junit.Test, org.junit.Assert, org.junit.rules.TemporaryFolder and
junit.framework.AssertionFailedError is rejected with a message naming the
replacement for each, and the migrated tree passes.
fe/be-java-extensions is suppressed for now and the suppression says why:
apache#66729 is rewriting those modules, so migrating them here would
only conflict. junit-vintage-engine comes out of fe/pom.xml when that lands -
not before, because without it a JUnit 4 test is silently not run rather than
failed.
Deliberately out of scope: extension/kettle and samples/ are standalone maven
projects, outside this reactor and built by no workflow here, so their JUnit 4
cannot be verified from this build. The workflow's pkg:maven/junit/junit
exception therefore stays.
Tests: `mvn test -pl fe-common,fe-core -am` over every changed test class -
444 classes, 2564 tests, 1 failure and 2 errors. Those three were separated from
pre-existing ones by running the same classes against unmodified master, where
PropertyAnalyzerTest, ForwardToMasterTest and
FileCacheAdmissionRuleRefresherTest fail identically and are untouched by this
change. `mvn test-compile` over the full 76-module reactor, with the new
checkstyle rule active, is green.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WVPNcULjD4ieAxdLH1WhDr
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W29vnZeWYBXE8E9myjeQS4
@morningman

Copy link
Copy Markdown
ContributorAuthor

run compile

@morningman

Copy link
Copy Markdown
ContributorAuthor

run feut

@morningman

Copy link
Copy Markdown
ContributorAuthor

skip buildall

@morningman
morningman merged commit 050442d into apache:masterSep 5, 2026
48 of 60 checks passed
morningman added a commit to morningman/doris that referenced this pull request Sep 5, 2026
…retire junit-vintage-engine
apache#67396 moved the fe reactor to JUnit 5 but left be-java-extensions alone, and
said why: those modules are being rewritten here, so migrating them there would
only have conflicted. It parked a checkstyle suppression for them and kept
junit-vintage-engine in fe/pom.xml, with an explicit note that the engine comes
out when this lands - not before, because without it a JUnit 4 test is silently
not run rather than failed.
This is that landing.
## The modules
11 test classes across 6 modules, 261 assertion calls:
| what moved | count |
|---|---|
| `Assert` -> `Assertions` | 261 calls |
| **message argument moved from first to last** | 32 |
| `@Rule TemporaryFolder` -> `@TempDir` | 3 files |
| `@Before`/`@After`/`@BeforeClass`/`@AfterClass` | all |
Nothing here used `@Test(expected=)`, the `ExpectedException` rule, `@Ignore`,
`Assume`, `@RunWith` or `junit.framework`, so those forms do not appear.
The message position is the one the compiler cannot catch: JUnit 4 puts an
assertion message FIRST and JUnit 5 puts it LAST, and for the three-argument
`assertEquals`/`assertSame`/`assertNotSame` forms `javac` accepts the unmoved
call and quietly reads the message as the expected value. Every `Assert` call in
these modules was therefore parsed rather than eyeballed, and the 32 that carry a
message were selected by argument count. The result was checked the same way: for
all 261 calls the non-message arguments come out in the order they went in.
`@TempDir` is a `Path`, as in the fe-core migration, except in
HadoopHudiPluginTest, which already imports `org.apache.hadoop.fs.Path`; that one
takes a `File`, which `@TempDir` supports equally and which is what its uses
wanted anyway. `newFolder()` created its directory, so it becomes
`Files.createDirectory(...)` and not a bare `resolve(...)`.
## The gate
With the modules migrated the be-java-extensions suppression is deleted and
junit-vintage-engine comes out of fe/pom.xml, so nothing in this reactor runs a
JUnit 4 test.
One thing apache#67396 assumed, and this change had to correct: removing the vintage
engine does NOT take junit:junit off the classpath. No pom declares it, but it
still arrives transitively - guava-testlib in fe-core (test) and
hive-exec -> zookeeper -> jline in hive-udf (provided). So a JUnit 4 test still
compiles; what it no longer does is run, because the jupiter engine ignores it
rather than failing. That is precisely the failure the checkstyle rule exists to
catch, and the workflow's junit:junit exception has to stay for the transitive
paths as well as for samples/. The three comments that described the old
arrangement are corrected rather than left to rot.
## Merge order: apache#67543 first
StreamingInsertJobStatusTransitionTest arrived in fe-core with apache#67529, after
apache#67396 had migrated everything around it, and is JUnit 4. It already fails the
`banJUnit4` check on master; once this change removes the vintage engine, the
three cases in it would also stop running with nothing to say so - exactly the
failure mode the engine was being held back to prevent.
It is fixed in apache#67543, not here, so that a one-file fix to master's own red
check does not have to wait on this branch. **apache#67543 must merge first.** Until
it does, fe-core checkstyle fails on this branch - which is the gate doing its
job, and the reason apache#67396 made it a gate.
## Verified
`mvn package` over all 14 be-java-extensions modules: BUILD SUCCESS, and every
migrated class ran - 86 tests, each class's count equal to its `@Test` method
count, so nothing was silently skipped by the missing engine.
`mvn test` over fe-core and its 59 upstream modules, with apache#67543 applied: BUILD
SUCCESS and 0 checkstyle violations in all 60. Without apache#67543 the only failure
is `banJUnit4` on that one file, as described above.
Re-adding a JUnit 4 import to a be-java-extensions test was confirmed to fail
checkstyle now that the suppression is gone, and `dependency:tree` over the
reactor confirms the two transitive junit:junit paths named above are the only
ones left.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01A58s52vfYcoTyJiMVv3cdA
morningman added a commit to morningman/doris that referenced this pull request Sep 5, 2026
master removed JUnit 4 from the fe reactor (apache#67396) and gates it with a
checkstyle rule that runs at the validate phase, so every test this branch
adds or edits has to come across with it. The rename itself is mechanical.
Three things in it are not:
* JUnit 4 puts an assertion message FIRST and JUnit 5 puts it LAST. javac
catches the swap for assertTrue/assertFalse/assertNull/assertNotNull and for
assertThrows, and does not catch it for assertEquals/assertSame when the
trailing argument is itself a String - the silent case, and the reason each
of the 31 calls moved here was read rather than pattern-matched.
* Where a hunk of this branch merged cleanly into a file master had already
migrated, it left `Assert.` behind with no import left to resolve it.
RoutineLoadManagerTest and RangerDorisAccessControllerFactoryTest each had
such calls.
* RangerHiveAccessControllerTest is where the fe-core test apache#67206 added ends
up, because this branch moves that source into a plugin module of its own.
The access type mapping that test pinned is pinned here instead: select,
update, and USE asking for any access at all rather than for an access type
no Hive service definition declares.
StreamingInsertJobStatusTransitionTest comes across too, and is not this
branch's: apache#67529 landed while apache#67396 was in flight, so master's own gate
rejects it - `mvn checkstyle:check -pl fe-core` on master reports its two
JUnit 4 imports, and nothing in fe-core builds until they are gone.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018bbhc4p18XwJWzX5i4LUwn
morningman added a commit that referenced this pull request Sep 5, 2026
…67543)
### What problem does this PR solve?
Issue Number: close #xxx
Related PR: #67396, #67529, #66729
Problem Summary:
**master is red on `fe-core` checkstyle.** #67396 removed JUnit 4 from
the fe
reactor and added a `banJUnit4` checkstyle rule so it could not come
back.
#67529 added `StreamingInsertJobStatusTransitionTest`, written with
JUnit 4.
The two crossed in flight: the migration could not cover a file that did
not
exist when it was written, and the new test was branched before the rule
existed.
`mvn checkstyle:check -pl fe-core` on master reports exactly two errors,
both
in this file, on its two JUnit 4 imports:
```
src/test/java/org/apache/doris/job/extensions/insert/streaming/
StreamingInsertJobStatusTransitionTest.java:23 error
StreamingInsertJobStatusTransitionTest.java:24 error
```
The fix is mechanical: two imports and six `Assert.` call sites. All six
are
one- or two-argument forms carrying no assertion message, so none is
affected
by the JUnit 4 -> 5 message reordering (JUnit 4 puts an assertion
message
**first**, JUnit 5 puts it **last**). Every argument stays exactly where
it is.
**Why this is worth its own PR rather than waiting.**
`junit-vintage-engine` is
what runs a JUnit 4 test in this reactor, and it is on its way out -
#66729
removes it once the `be-java-extensions` modules are migrated, which is
the last
thing keeping it alive. Without that engine, the jupiter engine does not
fail on
a JUnit 4 test, it **ignores** it: these three cases would stop running
and
nothing would say so. That silent-skip is exactly the failure #67396
made the
gate a checkstyle rule for, and exactly why it held the engine back
until the
tree was clean. Landing this first keeps that ordering safe.
Sign up for freeto 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.

8 participants

@morningman@hello-stephen@mymeiyi@gavinchou@luwei16@yiguolei@CalvinKirs@morrySnow