Skip to content

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

Open
morningman wants to merge 2 commits into
apache:masterfrom
morningman:ci-dep-review-allow-junit
Open

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

Conversation

@morningman

@morningman morningman 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 moved count
AssertAssertions ~8600 call sites
message argument moved from first to last 407
@Test(expected = X.class)assertThrows 94 methods
@Rule ExpectedExceptionassertThrows + message substring assertion 18 sites, 8 files
@Rule TemporaryFolder@TempDir Path 4 files
Assume.assumeTrue(msg, cond)assumeTrue(cond, msg) 12 sites
@Before/@After/@BeforeClass/@AfterClass/@Ignore all
@RunWith(MockitoJUnitRunner)MockitoAnnotations.openMocks 1
@FixMethodOrder@TestMethodOrder 1
junit.framework.AssertionFailedErrorjava.lang.AssertionError 2 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 test locally, 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_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 (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
Contributor Author

run buildall

@morningman
morningman force-pushed the ci-dep-review-allow-junit branch from 1b49834 to 11dfe05 Compare September 2, 2026 03:05
@morningman morningman 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 review Sep 2, 2026
morningman and others added 2 commits September 2, 2026 11:16
… 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
@morningman
morningman force-pushed the ci-dep-review-allow-junit branch from 11dfe05 to 9e67622 Compare September 2, 2026 03:20
@morningman

Copy link
Copy Markdown
Contributor Author

run buildall

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.

5 participants