yashmayya opened a new pull request, #19162: URL: https://github.com/apache/pinot/pull/19162
`pinot-broker` test classes that extend `ControllerTest` were silently executing **zero** tests while the build reported success. ### Root cause `ControllerTest` (in the `pinot-controller` test-jar) uses `PinotAdminClient` / `PinotAdminException`, which live in `pinot-java-client`. `pinot-controller` declares `pinot-java-client` at `test` scope, and test-scoped dependencies are not transitive — so it never reaches `pinot-broker`'s test classpath. Test *compilation* still succeeds, so there is no build error. The failure only happens at runtime, when TestNG reflects over `ControllerTest`'s members during discovery and hits `NoClassDefFoundError: org/apache/pinot/client/admin/PinotAdminException`. TestNG then reports `Tests run: 0` instead of an error, and the build passes. This hides completely: the surefire XML for such a run contains no `<testcase>`, no `<error>`, and no `system-err` entry — the exception is recorded nowhere. ### Impact Eight test classes were affected, not just one: | | Before | After | |---|---|---| | The 8 `ControllerTest`-based classes | 0 | **39**, all passing | | Full `pinot-broker` suite | 282 | **321** | All 39 pass, so no test was failing while hidden — this is purely restored coverage. ### Scope check Five modules consume the `pinot-controller` test-jar. I verified that `ControllerTest` loads on each one's actual test classpath; `pinot-broker` was the only gap. The others already have `pinot-java-client`, either declared directly (`pinot-integration-test-base`, `pinot-compatibility-verifier`) or inherited from `pinot-integration-test-base` (`pinot-integration-tests`, `pinot-perf`). ### Note on failing loudly instead `-DfailIfNoTests=true` does turn this specific case into a build failure, but it is not a good general guard: - It breaks the standard `-pl <module> -am` workflow — upstream modules that legitimately run zero tests fail the build, even with `-Dsurefire.failIfNoSpecifiedTests=false`. - It would not have caught this in CI anyway, since CI runs the full module suite, which reported a non-zero 282 tests. `surefire.failIfNoSpecifiedTests` (already `true` by default) cannot help either: the `-Dtest` filter did match the class file; only the class load failed. A more durable fix would be to move shared test fixtures like `ControllerTest` into a module whose dependencies are compile-scoped, so consumers inherit them transitively. That is a larger refactor and is left out of this PR. -- 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]
