FrankChen021 commented on issue #13948:
URL: https://github.com/apache/druid/issues/13948#issuecomment-5214878088
### Proposed next steps after #19875: remove cross-PR test-fixture
dependencies
I reviewed the backed-up migration branch and the current migration PRs
#19908–#19911. The main problem is not the production-module dependency graph;
it is reusable code from one module's **test JAR** exposing JUnit 4 or Hamcrest
types to tests in another module.
#### Current dependency edges
- **#19911 (server) → processing test fixtures**
- Server tests use `processing`'s `TestHelper`, which still calls
`org.junit.Assert`.
- **#19909 (MSQ) → #19910 (indexing-service)**
- `CompactionTaskRunBase` exposes `TaskActionTestKit`, which is currently
a JUnit 4 `ExternalResource`.
- **#19909 (SQL/MSQ) → processing test fixtures**
- The PR changes and consumes shared group-by fixtures such as
`GroupByQueryRunnerTestHelper` and `TestGroupByBuffers`.
- **#19908 (extensions-contrib) → SQL test fixtures**
- Several extension tests extend `BaseCalciteQueryTest`; its reusable API
currently exposes Hamcrest matcher types.
- **#19908 (extensions-contrib) → server test fixtures**
- gRPC and moving-average tests use `QueryStackTests`, whose reusable
paths still expose JUnit 4 classes.
The large PRs also introduced separate copies of assertion, matcher,
logger-capture, temporary-directory, and lifecycle helpers. Those copies avoid
an immediate compile-time dependency between PR branches, but leave duplicate
infrastructure and make the eventual cleanup harder.
#### Recommendation: one small shared-fixture foundation PR
After #19875 is merged, create a fresh PR from the latest `master`,
tentatively titled:
> `test: add JUnit 5-compatible shared test fixtures`
This foundation should contain **no bulk test migration and no dependency
removal**. It should preserve legacy JUnit 4 callers while adding
framework-neutral or JUnit 5-compatible surfaces for the later migrations:
1. **processing**
- Make reusable `TestHelper` assertions Jupiter-based internally.
- Move reusable group-by factory construction into
`GroupByQueryRunnerTestHelper`; keep the legacy test class delegating to it.
- Preserve the existing `AggregationTestHelper` method names and both
legacy and `File`-based temp-directory APIs added by #19882. Do not restore the
backed-up destructive signature changes.
2. **indexing-service**
- Extract framework-neutral `TaskActionTestKit` lifecycle support.
- Provide separate JUnit 4 rule and Jupiter extension adapters during the
transition.
- Let `CompactionTaskRunBase` expose the neutral support rather than a
JUnit 4 `ExternalResource`.
3. **server**
- Add one shared Jupiter-compatible Derby test connector in the server
test JAR.
- Make the reusable portions of `QueryStackTests` JUnit 5-safe while
retaining a small legacy adapter for unmigrated callers.
4. **sql**
- Remove Hamcrest types from APIs exposed by `BaseCalciteQueryTest` /
`QueryTestRunner` to downstream test JAR consumers.
- Add one Jupiter-native Druid-exception assertion API and migrate only
the existing SQL callers affected by that signature change.
This is expected to be roughly **25–35 files**. A processing-only foundation
of about five files would not resolve the SQL, server, and indexing-service
edges above. If the foundation must be smaller than about ten files,
extensions-contrib would need to retain temporary JUnit 4/Hamcrest
compatibility and be cleaned up again later.
#### Independent migration PRs after the foundation merges
All migration PRs should branch independently from `master` after the
foundation is merged, rather than stacking on one another:
1. processing
2. benchmark + cloud
3. extensions-contrib
4. indexing-service
5. SQL + Quidem
6. MSQ
7. server
8. services + embedded-tests
#19908 and #19910 can likely be salvaged by rebasing and removing files
owned by the foundation. #19909 and #19911 should preferably be replaced/split
because they currently mix module ownership and are very large (about 249 and
469 files respectively). The old PRs can be closed with links to their
replacements so their review history remains discoverable.
If fewer PRs are preferred, SQL + Quidem + MSQ may remain one PR, and server
+ services + embedded-tests may remain another; they will still be independent
once the shared foundation is merged.
#### Final cleanup
After all migration PRs merge, perform one repository-wide cleanup to remove:
- `junit:junit`
- JUnit Vintage
- Jupiter migration support
- JUnitParams
- Hamcrest where no production/test API requires it
- the JUnit OpenRewrite recipe
- `dev/validate-junit5-usage` (or replace it with a permanent JUnit 5-only
guard)
Each PR should be locally test-compiled before pushing, run affected tests,
and run Checkstyle plus PMD/SpotBugs/other applicable static checks. It should
also audit imports and Maven dependencies to ensure that removing JUnit
4/Hamcrest is real rather than hidden by a transitive dependency.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
