Remove dead surefire executions in root pom#17696
Open
JackieTien97 wants to merge 1 commit into
Open
Conversation
The root pom declared two extra surefire executions, `unit-tests` (bound to
the `test` phase) and `integration-tests` (bound to the `integration-test`
phase), each with an `<includes>` pattern prefixed by `src/test/`:
<includes><include>src/test/**/*Test.java</include></includes>
Surefire scans `${project.build.testOutputDirectory}` (target/test-classes/)
for compiled tests and matches `<includes>` against paths relative to that
directory. Those paths never start with `src/test/`, so both executions match
zero tests on every build — verified against the latest master Unit-Test run
(job 76358113571): each completes in <1 second with no test output.
The TODO comment above the plugin already noted that integration tests should
move to the failsafe plugin. That's now the case: both modules containing
`*IT.java` (`iotdb-core/datanode/` and `integration-test/`) have explicit
`maven-failsafe-plugin` configurations driving their ITs. Removing these two
dead executions therefore changes nothing observable on master — same set of
tests, same wall clock — and eliminates a hidden trap: anyone passing a
surefire CLI flag like `-Dsurefire.includesFile=` or `-Dtest=` would
otherwise discover that the broken `<includes>` gets replaced by their valid
selector, accidentally activating the previously-dormant executions and
running the chosen tests three times instead of once.
Regression checked:
- Only `iotdb-core/datanode/pom.xml` and `integration-test/pom.xml` contain
`*IT.java`; both configure failsafe directly.
- No other module-level pom redeclares an execution with id `unit-tests`
or `integration-tests`.
- No CI workflow passes a surefire test-selector flag that would have
been affected.
- The `with-code-coverage` profile only overrides the `<argLine>` for
surefire/failsafe; it does not depend on these executions.
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
2 participants
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.
Summary
Removes two surefire
<execution>blocks from the rootpom.xmlthat never actually run any tests, but silently cause triple test-execution if anyone passes a CLI flag like-Dsurefire.includesFile=or-Dtest=. The TODO comment above the plugin already acknowledged they should be removed.What the dead executions look like
Why they are dead
Surefire scans
${project.build.testOutputDirectory}— i.e.target/test-classes/— and matches<includes>against paths relative to that directory. Those paths look likeorg/apache/iotdb/.../FooTest.class, never prefixed bysrc/test/. So the patterns above match zero test classes on every build.Evidence from the latest master Unit-Test run (job
76358113571, Windows datanode):surefire:test (default-test)surefire:test (unit-tests)failsafe:integration-test (run-integration-tests)EnvScriptIT)surefire:test (integration-tests)Why they're worth removing even though master speed doesn't change
If someone tries to optimize CI by sharding via
-Dsurefire.includesFile=...or selecting tests with-Dtest=..., the CLI flag replaces the broken<includes>with a valid pattern. Suddenly all three surefire executions (default-test,unit-tests,integration-tests) run the selected tests — the test list executes 3×. This trap was discovered while attempting #17693 (closed).Regression check
*IT.java: onlyiotdb-core/datanode/andintegration-test/. Both already configuremaven-failsafe-pluginexplicitly, so IT coverage is unaffected.unit-testsorintegration-tests.mastercurrently passes a surefire test-selector flag.with-code-coverageprofile only overrides the surefire/failsafe<argLine>; it does not depend on these executions' presence.Test plan
default-testreportsTests run: 3629on master Windows datanode).Cluster IT - 1C1D,Table Cluster IT - 1C1D, etc.) are unaffected.with-code-coverageprofile) still produces output.