imbajin commented on code in PR #3161: URL: https://github.com/apache/hugegraph/pull/3161#discussion_r3838615228
########## hugegraph-server/hugegraph-dist/src/assembly/travis/check-jacoco-report.sh: ########## @@ -0,0 +1,145 @@ +#!/bin/bash +# +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# + +set -uo pipefail + +REQUIRED_SESSIONS=() +REQUIRED_TEST_REPORTS=() +REQUIRED_SUITE_REPORTS=() +while (( $# > 0 )); do + case "${1}" in + --require-session) + if (( $# < 2 )) || [[ -z "${2:-}" || "${2}" == --* ]]; then + echo "ERROR: --require-session requires a non-empty value" >&2 + exit 1 + fi + REQUIRED_SESSIONS+=("${2}") + shift 2 + ;; + --require-test-report) + if (( $# < 2 )) || [[ -z "${2:-}" || "${2}" == --* ]]; then + echo "ERROR: --require-test-report requires a non-empty value" >&2 + exit 1 + fi + REQUIRED_TEST_REPORTS+=("${2}") + shift 2 + ;; + --require-suite-report) + if (( $# < 2 )) || [[ -z "${2:-}" || "${2}" == --* ]]; then + echo "ERROR: --require-suite-report requires a non-empty value" >&2 + exit 1 + fi + REQUIRED_SUITE_REPORTS+=("${2}") + shift 2 + ;; + --*) + echo "ERROR: unknown option: ${1}" >&2 + exit 1 + ;; + *) + break + ;; + esac +done + +if (( ${#REQUIRED_SESSIONS[@]} == 0 )); then + echo "ERROR: at least one --require-session is required" >&2 + exit 1 +fi + +if (( ${#REQUIRED_TEST_REPORTS[@]} == 0 )); then + echo "ERROR: at least one --require-test-report is required" >&2 + exit 1 +fi + +REPORT_FILE="${1:-}" +if (( $# > 0 )); then + shift +fi + +if [[ -z "${REPORT_FILE}" || ! -s "${REPORT_FILE}" ]]; then + echo "ERROR: JaCoCo report not found or empty: ${REPORT_FILE:-<unset>}" >&2 + exit 1 +fi + +if (( $# == 0 )); then + echo "ERROR: at least one expected module is required" >&2 + exit 1 +fi + +validate_test_report() { + local test_report="${1}" + local require_tests="${2}" + + if [[ ! -s "${test_report}" ]]; then + echo "ERROR: Surefire report not found or empty: ${test_report}" >&2 + return 1 + fi + + local test_count + if ! test_count=$(python3 - "${test_report}" <<'PY' +import sys +import xml.etree.ElementTree as ET + +root = ET.parse(sys.argv[1]).getroot() +if root.tag.rsplit("}", 1)[-1] != "testsuite" or "tests" not in root.attrib: + raise ValueError("not a Surefire testsuite report") +test_count = int(root.attrib["tests"]) +if test_count < 0: + raise ValueError("negative Surefire test count") +print(test_count) +PY + ); then + echo "ERROR: unable to parse Surefire report: ${test_report}" >&2 + return 1 + fi + if [[ "${require_tests}" == "true" ]] && (( test_count <= 0 )); then + echo "ERROR: Surefire report has no tests: ${test_report}" >&2 + return 1 + fi +} + +for suite_report in "${REQUIRED_SUITE_REPORTS[@]}"; do Review Comment: ⚠️ Important 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. ########## .github/workflows/pd-store-ci.yml: ########## @@ -228,32 +268,69 @@ jobs: - name: Run common test run: | - mvn test -pl hugegraph-store/hg-store-test -am -P store-common-test + mvn test -pl hugegraph-store/hg-store-test -am \ + -P store-common-test -Djacoco.sessionId=store-common-test - name: Run client test run: | - mvn test -pl hugegraph-store/hg-store-test -am -P store-client-test + mvn test -pl hugegraph-store/hg-store-test -am \ + -P store-client-test -Djacoco.sessionId=store-client-test - name: Run core test run: | - mvn test -pl hugegraph-store/hg-store-test -am -P store-core-test + mvn test -pl hugegraph-store/hg-store-test -am \ + -P store-core-test -Djacoco.sessionId=store-core-test - name: Run rocksdb test run: | - mvn test -pl hugegraph-store/hg-store-test -am -P store-rocksdb-test + mvn test -pl hugegraph-store/hg-store-test -am \ + -P store-rocksdb-test -Djacoco.sessionId=store-rocksdb-test - name: Run server test run: | - mvn test -pl hugegraph-store/hg-store-test -am -P store-server-test + mvn test -pl hugegraph-store/hg-store-test -am \ + -P store-server-test -Djacoco.sessionId=store-server-test - name: Run raft-core test run: | - mvn test -pl hugegraph-store/hg-store-test -am -P store-raftcore-test + mvn test -pl hugegraph-store/hg-store-test -am \ + -P store-raftcore-test -Djacoco.sessionId=store-raftcore-test + + - name: Generate aggregate coverage report + run: | + mvn verify -pl hugegraph-store/hg-store-test -am -P jacoco \ + -DskipTests -Deditorconfig.skip=true -ntp + + - name: Validate aggregate coverage report + run: | + # CoreSuiteTest and ServerSuiteTest are existing placeholders with zero tests. + $TRAVIS_DIR/check-jacoco-report.sh \ + --require-test-report \ + "$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 \ Review Comment: ⚠️ Important 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. ########## hugegraph-server/hugegraph-dist/src/assembly/travis/check-jacoco-report.sh: ########## @@ -0,0 +1,145 @@ +#!/bin/bash +# +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# + +set -uo pipefail + +REQUIRED_SESSIONS=() +REQUIRED_TEST_REPORTS=() +REQUIRED_SUITE_REPORTS=() +while (( $# > 0 )); do + case "${1}" in + --require-session) + if (( $# < 2 )) || [[ -z "${2:-}" || "${2}" == --* ]]; then + echo "ERROR: --require-session requires a non-empty value" >&2 + exit 1 + fi + REQUIRED_SESSIONS+=("${2}") + shift 2 + ;; + --require-test-report) + if (( $# < 2 )) || [[ -z "${2:-}" || "${2}" == --* ]]; then + echo "ERROR: --require-test-report requires a non-empty value" >&2 + exit 1 + fi + REQUIRED_TEST_REPORTS+=("${2}") + shift 2 + ;; + --require-suite-report) + if (( $# < 2 )) || [[ -z "${2:-}" || "${2}" == --* ]]; then + echo "ERROR: --require-suite-report requires a non-empty value" >&2 + exit 1 + fi + REQUIRED_SUITE_REPORTS+=("${2}") + shift 2 + ;; + --*) + echo "ERROR: unknown option: ${1}" >&2 + exit 1 + ;; + *) + break + ;; + esac +done + +if (( ${#REQUIRED_SESSIONS[@]} == 0 )); then + echo "ERROR: at least one --require-session is required" >&2 + exit 1 +fi + +if (( ${#REQUIRED_TEST_REPORTS[@]} == 0 )); then + echo "ERROR: at least one --require-test-report is required" >&2 + exit 1 +fi + +REPORT_FILE="${1:-}" +if (( $# > 0 )); then + shift +fi + +if [[ -z "${REPORT_FILE}" || ! -s "${REPORT_FILE}" ]]; then + echo "ERROR: JaCoCo report not found or empty: ${REPORT_FILE:-<unset>}" >&2 + exit 1 +fi + +if (( $# == 0 )); then + echo "ERROR: at least one expected module is required" >&2 + exit 1 +fi + +validate_test_report() { + local test_report="${1}" + local require_tests="${2}" + + if [[ ! -s "${test_report}" ]]; then + echo "ERROR: Surefire report not found or empty: ${test_report}" >&2 + return 1 + fi + + local test_count + if ! test_count=$(python3 - "${test_report}" <<'PY' +import sys +import xml.etree.ElementTree as ET + +root = ET.parse(sys.argv[1]).getroot() +if root.tag.rsplit("}", 1)[-1] != "testsuite" or "tests" not in root.attrib: + raise ValueError("not a Surefire testsuite report") +test_count = int(root.attrib["tests"]) +if test_count < 0: + raise ValueError("negative Surefire test count") +print(test_count) +PY + ); then + echo "ERROR: unable to parse Surefire report: ${test_report}" >&2 + return 1 + fi + if [[ "${require_tests}" == "true" ]] && (( test_count <= 0 )); then + echo "ERROR: Surefire report has no tests: ${test_report}" >&2 + return 1 + fi +} + +for suite_report in "${REQUIRED_SUITE_REPORTS[@]}"; do + validate_test_report "${suite_report}" false || exit 1 +done + +for test_report in "${REQUIRED_TEST_REPORTS[@]}"; do + validate_test_report "${test_report}" true || exit 1 +done + +if ! grep -Eq '<counter type="INSTRUCTION" missed="[0-9]+" covered="[1-9][0-9]*"' \ Review Comment: ‼️ Critical 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. -- 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]
