[fix](test) remove JUnit 4 from the fe reactor and unblock the dependency licence review - #67396
Open
morningman wants to merge 2 commits into
Open
[fix](test) remove JUnit 4 from the fe reactor and unblock the dependency licence review#67396morningman wants to merge 2 commits into
morningman wants to merge 2 commits into
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
morningman
requested review from
924060929,
CalvinKirs,
Gabriel39,
deardeng,
gavinchou,
liaoxin01,
luwei16 and
mymeiyi
as code owners
September 2, 2026 03:04
morningman
force-pushed
the
ci-dep-review-allow-junit
branch
from
September 2, 2026 03:05
1b49834 to
11dfe05
Compare
… 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
force-pushed
the
ci-dep-review-allow-junit
branch
from
September 2, 2026 03:20
11dfe05 to
9e67622
Compare
Contributor
Author
|
run buildall |
CalvinKirs
approved these changes
Sep 2, 2026
gavinchou
approved these changes
Sep 2, 2026
morrySnow
approved these changes
Sep 2, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Dependency License Reviewfails 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: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-jupiteris EPL-2.0, whichallow-licensesalready carries. GitHub's dependency graph reports it asLicenseRef-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:junitis 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-liteexception uses — rather than by wideningallow-licensesor droppingdevelopmentfromfail-on-scopes.allow-dependencies-licensesexcludes 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 (
purlsMatchin itssrc/purl.ts). The version-less entries added here therefore cover every version, and the version pin on the existingcaniuse-liteentry has no effect either.2. Removing JUnit 4 from the fe reactor
junit:junitis the one EPL-1.0 artifact in this build. Nothing declares it —junit-vintage-enginedrags it in, and that engine exists only to run the JUnit 4 tests still in the tree. 450 files, 441 of them infe-core, were what kept it alive. They are JUnit 5 now.Assert→Assertions@Test(expected = X.class)→assertThrows@Rule ExpectedException→assertThrows+ message substring assertion@Rule TemporaryFolder→@TempDir PathAssume.assumeTrue(msg, cond)→assumeTrue(cond, msg)@Before/@After/@BeforeClass/@AfterClass/@Ignore@RunWith(MockitoJUnitRunner)→MockitoAnnotations.openMocks@FixMethodOrder→@TestMethodOrderjunit.framework.AssertionFailedError→java.lang.AssertionErrorTwo traps the compiler cannot catch, handled explicitly:
javaccatches the swap forassertTrue/assertFalse/assertNull/assertArrayEquals, but not forassertEquals/assertSame/assertNotEqualswhen the last argument is itself aString. The 8 three-argumentassertEqualscalls that are a float delta rather than a message were identified and left alone.Assert.assertEquals(Object[], Object[])compares arrays; the JUnit 5assertEqualsit would become compares references. One such call was converted toassertArrayEquals.Three things only running the tests found
JdbcSourceOffsetProviderAsyncSplitTestspelled 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, itsMockedStaticnever closed, and 27 tests died onstatic mocking is already registered. This is why the checkstyle pattern below matches anywhere on a line and not just an import.StatsCalculatorTest.testFilterOutofRangewas annotated@org.junit.Testin a class the jupiter engine already ran — it has never executed. Spelled@Testit 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 inheritedconnectContextwas alwaysnull, 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 mocksEnvandConnectContextstatically. The vestigial inheritance is dropped instead, with a comment.3. The gate
A checkstyle rule, because checkstyle runs at the
validatephase withincludeTestSourceDirectory: a JUnit 4 import fails a plainmvn testlocally, rather than waiting for CI. The pattern also coversjunit.framework.*(how a JUnit 3 import had survived here) and matches fully qualified references, for the reason above.fe/be-java-extensionsis suppressed for now and the suppression says why: #66729 is rewriting those modules, so migrating them here would only conflict.junit-vintage-enginecomes out offe/pom.xmlwhen that lands — not before, because without it a JUnit 4 test is silently not run rather than failed.Deliberately out of scope:
extension/kettleandsamples/are standalone maven projects, outside this reactor and built by no workflow here, so their JUnit 4 cannot be verified from this build. Thepkg:maven/junit/junitexception therefore stays.Release note
None
Check List (For Author)
Workflow change, verified against the real payload rather than by inspection:
package_urlpkg:maven/junit/junitandpkg: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 (groupChangesskips the exclusion list whenchange.package_url.length === 0).packageurl-js, the library the action validates them with; the folded YAML value parses to exactly those three comma-separated entries, whichparseListtrims.Migration:
mvn test -pl fe-common,fe-core -amover 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, wherePropertyAnalyzerTest,ForwardToMasterTestandFileCacheAdmissionRuleRefresherTestfail identically and are untouched by this change.mvn test-compileover 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.TemporaryFolderandjunit.framework.AssertionFailedErroris rejected with a message naming the replacement for each and fails the build; the migrated tree passes with 0 checkstyle errors.Behavior changed:
Does this need documentation?
🤖 Generated with Claude Code
https://claude.ai/code/session_01WVPNcULjD4ieAxdLH1WhDr