test(processing): migrate query tests to JUnit 5 - #19980
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Addressed the CodeQL findings in commit c86509a: added explicit guards before all four lastResult dereferences; replaced all seven deprecated ColumnAnalysis.getType() calls in SegmentAnalyzerTest with type-signature APIs (using getComplexTypeName where the expected complex name is required); and removed the deprecated ScanQuery.isLegacy() call while retaining the legacy=false serde assertion. Focused tests pass. The current token cannot execute GitHub inline review replies or resolveReviewThread, so this top-level response records the fixes. |
FrankChen021
left a comment
FrankChen021
left a comment
There was a problem hiding this comment.
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 0 |
| P2 | 1 |
| P3 | 1 |
| Total | 2 |
Reviewed 77 of 77 changed files.
This is an automated review by Codex GPT-5.6-Luna(max)
|
|
||
| private static boolean containsMessage(final Throwable thrown, final String expectedMessage) | ||
| { | ||
| for (Throwable current = thrown; current != null; current = current.getCause()) { |
There was a problem hiding this comment.
[P2] Expected-message matching is too permissive
The adapter searches all causes, whereas JUnit 4 ExpectedException matches the thrown exception's message. A wrapper with the expected text only in a nested cause can now make migrated tests pass despite a top-level message regression. Match only thrown.getMessage(), or preserve explicit cause-matcher semantics.
| @Ignore | ||
| public class TopNQueryRunnerBenchmark extends AbstractBenchmark | ||
| @Disabled | ||
| public class TopNQueryRunnerBenchmark |
There was a problem hiding this comment.
[P3] TopN benchmark loses measurement harness
Removing AbstractBenchmark and all BenchmarkOptions leaves this as a plain disabled JUnit 5 test. When manually run as a benchmark, testmMapped no longer receives JUnitBenchmarks warmup and measurement rounds. Retain the harness/options or migrate to an equivalent runner.
Summary
JupiterAssertionsandExpectThrowsadapters required by the exact migration base.Scope
processing/src/test/java/org/apache/druid/query, plus the two current test adapters.NestedDataTestUtils.java,GroupByQueryRunnerTestHelper.java,processing/pom.xml, excluded filter tests, and expression tests unchanged.org.apache.druid.testing.matchersframework or CI automation added.Resolves #13948
Checks
mvn -ntp -pl processing -am -DskipTests -Dweb.console.skip=true -T1C test-compile— passed with Checkstyle, PMD, main/test forbidden-API scans, and compilation.mvn -ntp -pl processing -am -DskipTests -Dweb.console.skip=true -T1C spotbugs:check— passed with zero SpotBugs bugs/errors.