ferdelyi commented on PR #8606:
URL: https://github.com/apache/hadoop/pull/8606#issuecomment-5121062906
> Two issues, both in `hadoop-bos`. It appears to have been missed while its
sibling `hadoop-tos` was handled correctly.
>
> ## Bug 1 (build-breaking): `hadoop-bos` missing from the coverage aggregate
> `hadoop-bos` is a test-bearing jar module (`<packaging>jar</packaging>` +
`src/test/java/`), but it is listed in neither `hadoop-coverage/pom.xml`'s
`<dependencies>` (the cloud-storage section lists `hadoop-cos`, `hadoop-gcp`,
`hadoop-huaweicloud`, `hadoop-tos`, but not `hadoop-bos`) nor
`coverage-modules-allowlist.txt`.
>
> Running this PR's own `check-coverage-modules.sh` against the current tree
fails at `hadoop-coverage`'s `validate` phase:
>
> ```
> check-coverage-modules: FAILED
> The following test-bearing modules are missing from the coverage aggregate:
> - hadoop-bos
> ```
>
> So the documented command (`mvn verify -Djacoco.skip=false ...`) fails at
`hadoop-coverage` before `report-aggregate` ever runs, until `hadoop-bos` is
added as a `<dependency>` in `hadoop-coverage/pom.xml`.
>
> ## Bug 2 (silent coverage gap): `hadoop-bos` surefire argLine missing
`@{argLine}`
> Even after Bug 1 is fixed,
`hadoop-cloud-storage-project/hadoop-bos/pom.xml:265` overrides surefire's
`<argLine>` without `@{argLine}`:
>
> ```
> <argLine>${maven-surefire-plugin.argLine}</argLine>
> ```
>
> A module-level `<argLine>` fully replaces the parent's, so the
`@{argLine}` token that `jacoco:prepare-agent` substitutes to inject the agent
is dropped. The forked test JVMs for `hadoop-bos` run without the JaCoCo agent,
producing no exec data, so its coverage shows as empty in the aggregate report,
silently. The guard script can't catch this; it only checks deps/allowlist
presence.
>
> Of the 21 surefire/failsafe `<argLine>` overrides in the repo, this PR
patched 20; `hadoop-bos` is the only one missed.
>
> Fix for both:
>
> ```
> <argLine>${maven-surefire-plugin.argLine} @{argLine}</argLine>
> ```
>
> plus a `hadoop-bos` `<dependency>` entry in `hadoop-coverage/pom.xml`.
>
> ## Minor
> The reorder of `maven-resources-plugin` after `exec-maven-plugin` in
`pluginManagement` (`hadoop-project/pom.xml`) is cosmetic only;
`pluginManagement` order has no effect in Maven.
Thanks @pan3793 — both are real, and they only surfaced once I rebased onto
current trunk (hadoop-bos landed after this branch was originally cut). All
three points are addressed in the latest push, which is rebased onto current
trunk:
Bug 1 (build-breaking): Added hadoop-bos as a <dependency> in
hadoop-coverage/pom.xml, so it's part of the aggregate and the guard passes —
check-coverage-modules: OK (72 test-bearing modules accounted for).
Bug 2 (silent coverage gap): Appended @{argLine} to hadoop-bos/pom.xml's
surefire <argLine>, matching every other argLine-overriding module, so
jacoco:prepare-agent injects the agent. Confirmed on a small module that
coverage is off by default and -Djacoco.skip=false produces target/jacoco.exec.
Minor (reorder): Agreed, pluginManagement order is irrelevant — I've
restored trunk's ordering (maven-resources-plugin before exec-maven-plugin) so
hadoop-project/pom.xml carries no gratuitous diff.
Separately, I extended check-coverage-modules.sh to catch the Bug 2 class
going forward: it now fails the build at the validate phase if a test-bearing,
non-allowlisted module overrides <argLine> without @{argLine} (verified it
flags hadoop-bos when the token is removed). The structural fix that removes
the footgun entirely — centralizing the argLine so a module can't drop
@{argLine} — is tracked as a follow-up in HADOOP-19948.
--
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]
