fix(ci): preserve PD and Store coverage aggregation - #3161
Conversation
imbajin
left a comment
imbajin
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: The aggregate report and Maven lifecycle checks pass, but the validator can accept incomplete suite/module coverage and can hang on malformed arguments. Evidence: Exact-head PD/Store aggregate builds and validator contract tests passed; current-head CI Codecov uploads returned HTTP 429 and exited 0.
dosubot
Bot
added
size:XL
imbajin
left a comment
imbajin
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: The Store CI job drops two existing test suites while still publishing an aggregate coverage report, so the coverage gate no longer represents the full Store test scope. Evidence: exact-head .github/workflows/pd-store-ci.yml patch and hg-store-test/pom.xml suite definitions.
imbajin
left a comment
imbajin
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: The coverage workflow now exercises the intended PD/Store profiles and produces aggregate reports, but the validator still permits false-positive coverage evidence in two paths. Evidence: exact-head CI is green, while the findings below concern the validator's acceptance criteria and local contract portability.
| "$TEST_REPORT_DIR/TEST-org.apache.hugegraph.store.common.CommonSuiteTest.xml" \ | ||
| --require-test-report \ | ||
| "$TEST_REPORT_DIR/TEST-org.apache.hugegraph.store.client.ClientSuiteTest.xml" \ | ||
| --require-suite-report \ |
There was a problem hiding this comment.
Blocking: yes. Summary: The Store coverage gate accepts the Core and Server suites even when both execute zero tests, so it can publish a full-looking aggregate without exercising those modules. Evidence: exact-head pd-store-ci.yml:312-317 uses --require-suite-report, and the exact-head Store job reports CoreSuiteTest and ServerSuiteTest with Tests run: 0; CoreSuiteTest remains an empty @Suite.SuiteClasses({}). Please either add real tests and use --require-test-report, or remove these placeholder suites from the required coverage scope and validate the remaining modules explicitly.
| fi | ||
| } | ||
|
|
||
| for suite_report in "${REQUIRED_SUITE_REPORTS[@]}"; do |
There was a problem hiding this comment.
Blocking: no. Summary: The validator exits before normal validation on macOS's bundled Bash 3.2 whenever no suite reports are supplied, which is the normal PD path. Evidence: with set -uo pipefail, iterating "${REQUIRED_SUITE_REPORTS[@]}" after REQUIRED_SUITE_REPORTS=() returns REQUIRED_SUITE_REPORTS[@]: unbound variable under /bin/bash 3.2; the PD workflow does not pass --require-suite-report. Please guard the loop with an empty-array check and run the contract harness on Bash 3.2 or provide a portable fallback.
| validate_test_report "${test_report}" true || exit 1 | ||
| done | ||
|
|
||
| if ! grep -Eq '<counter type="INSTRUCTION" missed="[0-9]+" covered="[1-9][0-9]*"' \ |
There was a problem hiding this comment.
Blocking: yes. Summary: The validator searches raw JaCoCo XML text instead of parsing the report, so commented or truncated snippets can satisfy the session, module, and covered-instruction checks, while a valid report with reordered attributes can be rejected. Evidence: check-jacoco-report.sh:125-143 uses grep for all three element types; a report containing the required tags only inside XML comments still matches these expressions. Please parse the XML elements and attributes, reject malformed/comment-only reports, and add reordered-attribute and commented-snippet contract cases.
2 participants
Purpose of the PR
PD and Store CI run several Maven test profiles in separate invocations, but
each invocation previously replaced JaCoCo execution data and generated a
partial report. PD also ran
mvn clean packageafter two coverage-producingtest profiles, deleting their data before upload. As a result, Codecov could
receive only the last profile's coverage or an imprecisely selected report.
The existing JaCoCo 0.8.4 configuration also cannot instrument Java 17 class
files, which blocks the project's planned Java 17 migration.
Main Changes
data across Maven invocations.
verify the complete expected session set before upload.
existing Store core/server profile invocations and require their exact
reports, while allowing their current placeholder suites to report zero
tests.
CoreSuiteTestan explicit empty JUnit suite, without enabling itscommented child tests, so Surefire emits the required placeholder report.
verifyand upload that exact fileto Codecov.
hg-store-rocksdbas a direct report-aggregate dependency only inthe
jacocoprofile so normal Store test dependency resolution is unchanged.that already-completed check during the final aggregate-only
verify.active and placeholder zero-test rules, uncovered reports, missing
sessions/modules, Maven lifecycle bindings, dependency scope, and exact
workflow inputs.
Verifying these changes
hugegraph-server/hugegraph-dist/src/assembly/travis/test-check-jacoco-report.sh.github/workflows/pd-store-ci.ymlwith PyYAML and all changed POMswith
xmllint.mvn -q apache-rat:check -N -ntpmvn editorconfig:check -pl hugegraph-server/hugegraph-dist -am -ntpmvn editorconfig:check -pl hugegraph-pd/hg-pd-test -am -ntpmvn editorconfig:check -pl hugegraph-store/hg-store-test -am -ntpmvn verify -pl hugegraph-pd/hg-pd-test -am -P jacoco -DskipTests -Deditorconfig.skip=true -ntpmvn verify -pl hugegraph-store/hg-store-test -am -P jacoco -DskipTests -Deditorconfig.skip=true -ntporg.javassist:javassist:3.24.0-GA.0.8.8 instruments the same class and writes execution data successfully.
verify both expected XML reports are emitted with
tests=0.The complete four-suite PD/six-profile Store workflow and the external Codecov
upload are left to GitHub Actions because the local environment uses JDK 17
while this workflow currently installs JDK 11.
Does this PR potentially affect the following parts?
Documentation Status
Doc - TODODoc - DoneDoc - No Need