[
https://issues.apache.org/jira/browse/HADOOP-19948?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Ferenc Erdelyi updated HADOOP-19948:
------------------------------------
Description:
JaCoCo coverage (introduced in HADOOP-15190) injects its agent by having the
shared Surefire/Failsafe configuration in {{hadoop-project/pom.xml}} end its
{{<argLine>}} with the late-bound {{@{argLine}}} token, which
{{jacoco:prepare-agent}} substitutes with the {{-javaagent}} option at test
time.
The problem is that a module-level {{<argLine>}} fully replaces the inherited
one. Any module that overrides {{<argLine>}} (there are ~21 today) must
remember to re-append {{@{argLine}}}; if it doesn't, the JaCoCo agent is never
attached to that module's forked test JVMs and the module contributes empty
coverage.
HADOOP-15190 added a build-time guard ({{check-coverage-modules.sh}}) that now
detects this — it fails the build when a test-bearing module overrides
{{<argLine>}} without {{@{argLine}}}. That closes the "silent" aspect, but it
is a policing net: every module author must still remember the token, and the
guard exists only to catch them when they forget.
Proposal: remove the footgun entirely so the guard has nothing to police. Stop
overriding {{<argLine>}} in individual modules; define the shared
Surefire/Failsafe {{<argLine>}} once (in {{hadoop-project/pom.xml}}) so it
always contains {{@{argLine}}}, and expose a property (e.g.
{{surefire.module.argLine}}, empty by default) for the extra per-module JVM
arguments:
{code:xml}
<!-- shared, in hadoop-project/pom.xml -->
<argLine>@{argLine} ${maven-surefire-plugin.argLine}
${surefire.module.argLine}</argLine>
{code}
Modules that today do {{<argLine>${maven-surefire-plugin.argLine} @{argLine}
-Xmx2048m</argLine>}} would instead set only:
{code:xml}
<surefire.module.argLine>-Xmx2048m</surefire.module.argLine>
{code}
With this, {{@{argLine}}} is guaranteed present for every module and cannot be
dropped.
Scope:
- Refactor the ~21 modules that currently override Surefire/Failsafe
{{<argLine>}} to set the new property instead.
- Update the shared configuration accordingly.
- Once no module overrides {{<argLine>}}, the guard's
{{<argLine>}}/{{@{argLine}}} check (from HADOOP-15190) becomes vestigial and
can be simplified or removed.
Acceptance criteria:
- No module declares its own {{<argLine>}} for Surefire/Failsafe.
- A full coverage build ({{mvn verify -Djacoco.skip=false ...}}) instruments
and reports coverage for every test-bearing module.
Testing / how to verify:
- No overrides remain: {{git grep -n '<argLine>' -- '**/pom.xml'}} returns only
the shared definition in {{hadoop-project/pom.xml}} — no module-level
{{<argLine>}}.
- Behavior unchanged (opt-in): {{mvn test -pl <small-module>}} produces no
{{target/jacoco.exec}} (coverage off by default); {{mvn test -pl <small-module>
-Djacoco.skip=false}} produces one (agent attached via the shared
{{@{argLine}}}).
- Per-module JVM args still applied: for a module that previously set a custom
{{<argLine>}} (e.g. one needing {{-Xmx2048m}}), confirm the new
{{surefire.module.argLine}} property still passes that arg to the forked JVM
(e.g. a test asserting max heap, or inspect the Surefire command line) — and
that the module is still instrumented.
- Full aggregate: {{mvn verify -Djacoco.skip=false
-Dmaven.test.failure.ignore=true --fail-at-end}} completes and every
test-bearing module that ran tests appears with non-empty coverage in
{{hadoop-coverage/target/site/jacoco-aggregate/index.html}} (i.e. none silently
empty because of a dropped agent).
- Guard is now vestigial: the {{<argLine>}}/{{@{argLine}}} check in
{{check-coverage-modules.sh}} (HADOOP-15190) finds nothing to flag, confirming
the footgun is gone; it can then be simplified or removed.
Relates to: HADOOP-15190 (added the {{@{argLine}}} mechanism and the detection
guard this issue makes unnecessary).
was:
JaCoCo coverage (introduced in HADOOP-15190) injects its agent by having the
shared Surefire/Failsafe configuration in {{hadoop-project/pom.xml}} end its
{{<argLine>}} with the late-bound {{@{argLine}}} token, which
{{jacoco:prepare-agent}} substitutes with the {{-javaagent}} option at test
time.
The problem is that a module-level {{<argLine>}} fully replaces the inherited
one. Any module that overrides {{<argLine>}} (there are ~21 today) must
remember to re-append {{@{argLine}}}; if it doesn't, the JaCoCo agent is never
attached to that module's forked test JVMs and the module contributes empty
coverage.
HADOOP-15190 added a build-time guard ({{check-coverage-modules.sh}}) that now
detects this — it fails the build when a test-bearing module overrides
{{<argLine>}} without {{@{argLine}}}. That closes the "silent" aspect, but it
is a policing net: every module author must still remember the token, and the
guard exists only to catch them when they forget.
Proposal: remove the footgun entirely so the guard has nothing to police. Stop
overriding {{<argLine>}} in individual modules; define the shared
Surefire/Failsafe {{<argLine>}} once (in {{hadoop-project/pom.xml}}) so it
always contains {{@{argLine}}}, and expose a property (e.g.
{{surefire.module.argLine}}, empty by default) for the extra per-module JVM
arguments:
{code:xml}
<!-- shared, in hadoop-project/pom.xml -->
<argLine>@{argLine} ${maven-surefire-plugin.argLine}
${surefire.module.argLine}</argLine>
{code}
Modules that today do {{<argLine>${maven-surefire-plugin.argLine} @{argLine}
-Xmx2048m</argLine>}} would instead set only:
{code:xml}
<surefire.module.argLine>-Xmx2048m</surefire.module.argLine>
{code}
With this, {{@{argLine}}} is guaranteed present for every module and cannot be
dropped.
Scope:
- Refactor the ~21 modules that currently override Surefire/Failsafe
{{<argLine>}} to set the new property instead.
- Update the shared configuration accordingly.
- Once no module overrides {{<argLine>}}, the guard's
{{<argLine>}}/{{@{argLine}}} check (from HADOOP-15190) becomes vestigial and
can be simplified or removed.
Acceptance criteria:
- No module declares its own {{<argLine>}} for Surefire/Failsafe.
- A full coverage build ({{mvn verify -Djacoco.skip=false ...}}) instruments
and reports coverage for every test-bearing module.
Relates to: HADOOP-15190 (added the {{@{argLine}}} mechanism and the detection
guard this issue makes unnecessary).
> Centralize the Surefire/Failsafe argLine so modules cannot drop JaCoCo
> instrumentation
> --------------------------------------------------------------------------------------
>
> Key: HADOOP-19948
> URL: https://issues.apache.org/jira/browse/HADOOP-19948
> Project: Hadoop Common
> Issue Type: Improvement
> Components: build
> Affects Versions: 3.6.0
> Reporter: Ferenc Erdelyi
> Assignee: Ferenc Erdelyi
> Priority: Minor
>
> JaCoCo coverage (introduced in HADOOP-15190) injects its agent by having the
> shared Surefire/Failsafe configuration in {{hadoop-project/pom.xml}} end its
> {{<argLine>}} with the late-bound {{@{argLine}}} token, which
> {{jacoco:prepare-agent}} substitutes with the {{-javaagent}} option at test
> time.
> The problem is that a module-level {{<argLine>}} fully replaces the inherited
> one. Any module that overrides {{<argLine>}} (there are ~21 today) must
> remember to re-append {{@{argLine}}}; if it doesn't, the JaCoCo agent is
> never attached to that module's forked test JVMs and the module contributes
> empty coverage.
> HADOOP-15190 added a build-time guard ({{check-coverage-modules.sh}}) that
> now detects this — it fails the build when a test-bearing module overrides
> {{<argLine>}} without {{@{argLine}}}. That closes the "silent" aspect, but it
> is a policing net: every module author must still remember the token, and the
> guard exists only to catch them when they forget.
> Proposal: remove the footgun entirely so the guard has nothing to police.
> Stop overriding {{<argLine>}} in individual modules; define the shared
> Surefire/Failsafe {{<argLine>}} once (in {{hadoop-project/pom.xml}}) so it
> always contains {{@{argLine}}}, and expose a property (e.g.
> {{surefire.module.argLine}}, empty by default) for the extra per-module JVM
> arguments:
> {code:xml}
> <!-- shared, in hadoop-project/pom.xml -->
> <argLine>@{argLine} ${maven-surefire-plugin.argLine}
> ${surefire.module.argLine}</argLine>
> {code}
> Modules that today do {{<argLine>${maven-surefire-plugin.argLine} @{argLine}
> -Xmx2048m</argLine>}} would instead set only:
> {code:xml}
> <surefire.module.argLine>-Xmx2048m</surefire.module.argLine>
> {code}
> With this, {{@{argLine}}} is guaranteed present for every module and cannot
> be dropped.
> Scope:
> - Refactor the ~21 modules that currently override Surefire/Failsafe
> {{<argLine>}} to set the new property instead.
> - Update the shared configuration accordingly.
> - Once no module overrides {{<argLine>}}, the guard's
> {{<argLine>}}/{{@{argLine}}} check (from HADOOP-15190) becomes vestigial and
> can be simplified or removed.
> Acceptance criteria:
> - No module declares its own {{<argLine>}} for Surefire/Failsafe.
> - A full coverage build ({{mvn verify -Djacoco.skip=false ...}}) instruments
> and reports coverage for every test-bearing module.
> Testing / how to verify:
> - No overrides remain: {{git grep -n '<argLine>' -- '**/pom.xml'}} returns
> only the shared definition in {{hadoop-project/pom.xml}} — no module-level
> {{<argLine>}}.
> - Behavior unchanged (opt-in): {{mvn test -pl <small-module>}} produces no
> {{target/jacoco.exec}} (coverage off by default); {{mvn test -pl
> <small-module> -Djacoco.skip=false}} produces one (agent attached via the
> shared {{@{argLine}}}).
> - Per-module JVM args still applied: for a module that previously set a
> custom {{<argLine>}} (e.g. one needing {{-Xmx2048m}}), confirm the new
> {{surefire.module.argLine}} property still passes that arg to the forked JVM
> (e.g. a test asserting max heap, or inspect the Surefire command line) — and
> that the module is still instrumented.
> - Full aggregate: {{mvn verify -Djacoco.skip=false
> -Dmaven.test.failure.ignore=true --fail-at-end}} completes and every
> test-bearing module that ran tests appears with non-empty coverage in
> {{hadoop-coverage/target/site/jacoco-aggregate/index.html}} (i.e. none
> silently empty because of a dropped agent).
> - Guard is now vestigial: the {{<argLine>}}/{{@{argLine}}} check in
> {{check-coverage-modules.sh}} (HADOOP-15190) finds nothing to flag,
> confirming the footgun is gone; it can then be simplified or removed.
> Relates to: HADOOP-15190 (added the {{@{argLine}}} mechanism and the
> detection guard this issue makes unnecessary).
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
