From 7388cda36c914776d0d8359c863f0c677b9d9fe5 Mon Sep 17 00:00:00 2001 From: Shai Almog <67850168+shai-almog@users.noreply.github.com> Date: Wed, 19 Nov 2025 07:16:50 +0200 Subject: [PATCH] Revert "Add Android coverage summary to screenshot comment (#4140)" This reverts commit cd81fc4b6e336ce65e0514a867b339c000050567. --- .../android/tests/RenderScreenshotReport.java | 283 +----------------- scripts/generate-android-coverage-report.sh | 181 ----------- scripts/run-android-instrumentation-tests.sh | 26 +- 3 files changed, 13 insertions(+), 477 deletions(-) diff --git a/scripts/android/tests/RenderScreenshotReport.java b/scripts/android/tests/RenderScreenshotReport.java index 6ede7c9b1b..0276ec5748 100644 --- a/scripts/android/tests/RenderScreenshotReport.java +++ b/scripts/android/tests/RenderScreenshotReport.java @@ -30,9 +30,7 @@ public static void main(String[] args) throws Exception { String title = arguments.title != null ? arguments.title : DEFAULT_TITLE; String successMessage = arguments.successMessage != null ? arguments.successMessage : DEFAULT_SUCCESS_MESSAGE; - CoverageSummary coverage = loadCoverage(arguments.coverageSummary, arguments.coverageHtmlUrl); - - SummaryAndComment output = buildSummaryAndComment(data, title, marker, successMessage, coverage); + SummaryAndComment output = buildSummaryAndComment(data, title, marker, successMessage); writeLines(arguments.summaryOut, output.summaryLines); writeLines(arguments.commentOut, output.commentLines); } @@ -51,14 +49,12 @@ private static void writeLines(Path path, List lines) throws IOException Files.writeString(path, sb.toString(), StandardCharsets.UTF_8); } - private static SummaryAndComment buildSummaryAndComment(Map data, String title, String marker, String successMessage, CoverageSummary coverage) { + private static SummaryAndComment buildSummaryAndComment(Map data, String title, String marker, String successMessage) { List summaryLines = new ArrayList<>(); List commentLines = new ArrayList<>(); Object resultsObj = data.get("results"); List results = resultsObj instanceof List list ? (List) list : List.of(); List> commentEntries = new ArrayList<>(); - ComparisonSummary comparisonSummary = new ComparisonSummary(); - boolean comparisonOverviewAdded = false; for (Object item : results) { Map result = JsonUtil.asObject(item); String test = stringValue(result.get("test"), "unknown"); @@ -121,17 +117,13 @@ private static SummaryAndComment buildSummaryAndComment(Map data } String noteColumn = previewNote != null ? previewNote : base64Note != null ? base64Note : ""; summaryLines.add(String.join("|", List.of(status, test, message, copyFlag, actualPath, noteColumn))); - comparisonSummary = comparisonSummary.record(status); } - appendCoverageSummary(summaryLines, coverage); - if (!commentEntries.isEmpty()) { if (title != null && !title.isEmpty()) { commentLines.add("### " + title); commentLines.add(""); } - comparisonOverviewAdded = appendComparisonOverview(commentLines, comparisonSummary); for (Map entry : commentEntries) { String test = stringValue(entry.get("test"), ""); String status = stringValue(entry.get("status"), ""); @@ -140,157 +132,18 @@ private static SummaryAndComment buildSummaryAndComment(Map data addPreviewSection(commentLines, entry); commentLines.add(""); } - } - - if (!comparisonOverviewAdded) { - comparisonOverviewAdded = appendComparisonOverview(commentLines, comparisonSummary); - } - - appendCoverageComment(commentLines, coverage); - - if (commentLines.isEmpty()) { - commentLines.add(successMessage != null ? successMessage : DEFAULT_SUCCESS_MESSAGE); - commentLines.add(""); - comparisonOverviewAdded = appendComparisonOverview(commentLines, comparisonSummary); - appendCoverageComment(commentLines, coverage); - } else if (commentLines.size() == 1 && commentLines.get(0).isEmpty()) { - commentLines.add(successMessage != null ? successMessage : DEFAULT_SUCCESS_MESSAGE); - commentLines.add(""); - comparisonOverviewAdded = appendComparisonOverview(commentLines, comparisonSummary); - appendCoverageComment(commentLines, coverage); - } - - if (commentLines.isEmpty()) { + if (!commentLines.isEmpty() && !commentLines.get(commentLines.size() - 1).isEmpty()) { + commentLines.add(""); + } + commentLines.add(marker); + } else { commentLines.add(successMessage != null ? successMessage : DEFAULT_SUCCESS_MESSAGE); commentLines.add(""); - appendComparisonOverview(commentLines, comparisonSummary); - } - - if (marker != null && !marker.isEmpty()) { commentLines.add(marker); } return new SummaryAndComment(summaryLines, commentLines); } - private static boolean appendComparisonOverview(List commentLines, ComparisonSummary summary) { - if (summary.total == 0) { - return false; - } - if (!commentLines.isEmpty() && !commentLines.get(commentLines.size() - 1).isEmpty()) { - commentLines.add(""); - } - List parts = new ArrayList<>(); - parts.add(summary.total + (summary.total == 1 ? " screenshot" : " screenshots")); - List statusParts = new ArrayList<>(); - statusParts.add(summary.equal + (summary.equal == 1 ? " matched" : " matched")); - if (summary.different > 0) { - statusParts.add(summary.different + (summary.different == 1 ? " updated" : " updated")); - } - if (summary.missingExpected > 0) { - statusParts.add(summary.missingExpected + (summary.missingExpected == 1 ? " missing reference" : " missing references")); - } - if (summary.missingActual > 0) { - statusParts.add(summary.missingActual + (summary.missingActual == 1 ? " missing actual" : " missing actuals")); - } - if (summary.errors > 0) { - statusParts.add(summary.errors + (summary.errors == 1 ? " error" : " errors")); - } - if (summary.other > 0) { - statusParts.add(summary.other + (summary.other == 1 ? " other" : " other")); - } - commentLines.add("Compared " + String.join("; ", parts) + ": " + String.join(", ", statusParts) + "."); - return true; - } - - private static void appendCoverageSummary(List summaryLines, CoverageSummary coverage) { - if (coverage == null || coverage.counters().isEmpty()) { - return; - } - for (Map.Entry entry : coverage.counters().entrySet()) { - CoverageCounter counter = entry.getValue(); - summaryLines.add(String.join("|", List.of( - "coverage", - entry.getKey().toLowerCase(), - String.format("%s/%s (%.2f%%)", counter.covered(), counter.total(), counter.coverage()), - "0", - coverage.htmlIndex() != null ? coverage.htmlIndex() : "", - coverage.artifact() != null ? coverage.artifact() : "" - ))); - } - } - - private static void appendCoverageComment(List commentLines, CoverageSummary coverage) { - if (coverage == null) { - return; - } - if (coverage.counters().isEmpty() && (coverage.htmlIndex() == null || coverage.artifact() == null)) { - return; - } - if (!commentLines.isEmpty() && !commentLines.get(commentLines.size() - 1).isEmpty()) { - commentLines.add(""); - } - commentLines.add("### Native Android coverage"); - commentLines.add(""); - - CoverageCounter lineCounter = coverage.counters().get("LINE"); - String linkText = formatCoverageLinks(coverage); - if (lineCounter != null) { - commentLines.add(String.format("- 📊 **Line coverage:** %.2f%% (%d/%d lines covered)%s", - lineCounter.coverage(), lineCounter.covered(), lineCounter.total(), linkText)); - } else if (!linkText.isEmpty()) { - commentLines.add("- 📊 Coverage report:" + linkText); - } - - if (!coverage.counters().isEmpty()) { - List counterSummaries = new ArrayList<>(); - for (Map.Entry entry : coverage.counters().entrySet()) { - if ("LINE".equals(entry.getKey())) { - continue; - } - CoverageCounter counter = entry.getValue(); - counterSummaries.add(String.format("%s %.2f%% (%d/%d)", - entry.getKey().toLowerCase(), counter.coverage(), counter.covered(), counter.total())); - } - if (!counterSummaries.isEmpty()) { - commentLines.add(" - Other counters: " + String.join(", ", counterSummaries)); - } - } - - if (!coverage.topClasses().isEmpty()) { - commentLines.add(" - **Lowest covered classes**"); - int rank = 1; - for (CoverageClass cls : coverage.topClasses()) { - if (rank > 10) { - break; - } - commentLines.add(String.format(" - `%s` – %.2f%% (%d/%d lines covered)", - cls.name(), cls.coverage(), cls.covered(), cls.total())); - rank++; - } - } - - if (lineCounter == null && coverage.topClasses().isEmpty() && linkText.isEmpty()) { - commentLines.add("- Coverage summary not available."); - } - - commentLines.add(""); - } - - private static String formatCoverageLinks(CoverageSummary coverage) { - List links = new ArrayList<>(); - if (coverage.htmlUrl() != null && !coverage.htmlUrl().isEmpty()) { - links.add(String.format(" [[HTML preview]](%s)", coverage.htmlUrl())); - } - if (coverage.artifact() != null) { - if (coverage.htmlIndex() != null && !coverage.htmlIndex().isEmpty()) { - links.add(String.format(" (artifact `%s`, %s)", coverage.artifact(), coverage.htmlIndex())); - } else { - links.add(String.format(" (artifact `%s`)", coverage.artifact())); - } - } - return String.join("", links); - } - private static Map commentEntry( String test, String status, @@ -416,18 +269,14 @@ private static class Arguments { final String marker; final String title; final String successMessage; - final Path coverageSummary; - final String coverageHtmlUrl; - private Arguments(Path compareJson, Path commentOut, Path summaryOut, String marker, String title, String successMessage, Path coverageSummary, String coverageHtmlUrl) { + private Arguments(Path compareJson, Path commentOut, Path summaryOut, String marker, String title, String successMessage) { this.compareJson = compareJson; this.commentOut = commentOut; this.summaryOut = summaryOut; this.marker = marker; this.title = title; this.successMessage = successMessage; - this.coverageSummary = coverageSummary; - this.coverageHtmlUrl = coverageHtmlUrl; } static Arguments parse(String[] args) { @@ -437,8 +286,6 @@ static Arguments parse(String[] args) { String marker = null; String title = null; String successMessage = null; - Path coverageSummary = null; - String coverageHtmlUrl = null; for (int i = 0; i < args.length; i++) { String arg = args[i]; switch (arg) { @@ -484,20 +331,6 @@ static Arguments parse(String[] args) { } successMessage = args[i]; } - case "--coverage-summary" -> { - if (++i >= args.length) { - System.err.println("Missing value for --coverage-summary"); - return null; - } - coverageSummary = Path.of(args[i]); - } - case "--coverage-html-url" -> { - if (++i >= args.length) { - System.err.println("Missing value for --coverage-html-url"); - return null; - } - coverageHtmlUrl = args[i]; - } default -> { System.err.println("Unknown argument: " + arg); return null; @@ -508,105 +341,7 @@ static Arguments parse(String[] args) { System.err.println("--compare-json, --comment-out, and --summary-out are required"); return null; } - return new Arguments(compare, comment, summary, marker, title, successMessage, coverageSummary, coverageHtmlUrl); - } - } - - private static CoverageSummary loadCoverage(Path summaryPath, String htmlUrlOverride) { - if (summaryPath == null) { - return null; - } - if (!Files.isRegularFile(summaryPath)) { - return null; - } - try { - String text = Files.readString(summaryPath, StandardCharsets.UTF_8); - Object parsed = JsonUtil.parse(text); - Map obj = JsonUtil.asObject(parsed); - String artifact = stringValue(obj.get("artifact"), null); - String htmlIndex = stringValue(obj.get("html_index"), null); - String htmlUrl = htmlUrlOverride != null ? htmlUrlOverride : stringValue(obj.get("html_url"), null); - Map counters = new LinkedHashMap<>(); - Map counterMap = JsonUtil.asObject(obj.get("counters")); - for (Map.Entry entry : counterMap.entrySet()) { - Map counterObj = JsonUtil.asObject(entry.getValue()); - Integer covered = toInteger(counterObj.get("covered")); - Integer total = toInteger(counterObj.get("total")); - Integer missed = toInteger(counterObj.get("missed")); - Double coverage = null; - if (counterObj.get("coverage") instanceof Number number) { - coverage = number.doubleValue(); - } - if (covered != null && total != null && coverage != null && missed != null) { - counters.put(entry.getKey(), new CoverageCounter(covered, total, coverage, missed)); - } - } - List lowestCoverage = new ArrayList<>(); - for (Object item : JsonUtil.asArray(obj.get("top_classes"))) { - Map classObj = JsonUtil.asObject(item); - String name = stringValue(classObj.get("name"), null); - Integer covered = toInteger(classObj.get("covered")); - Integer total = toInteger(classObj.get("total")); - Double coverage = classObj.get("coverage") instanceof Number number ? number.doubleValue() : null; - if (name != null && covered != null && total != null && coverage != null) { - lowestCoverage.add(new CoverageClass(name, covered, total, coverage)); - } - } - return new CoverageSummary(counters, artifact, htmlIndex, htmlUrl, lowestCoverage); - } catch (IOException ignored) { - return null; - } - } - - private record CoverageSummary(Map counters, String artifact, String htmlIndex, String htmlUrl, List topClasses) { - } - - private record CoverageCounter(int covered, int total, double coverage, int missed) { - } - - private record CoverageClass(String name, int covered, int total, double coverage) { - } - - private static class ComparisonSummary { - final int total; - final int equal; - final int different; - final int missingExpected; - final int missingActual; - final int errors; - final int other; - - ComparisonSummary() { - this(0, 0, 0, 0, 0, 0, 0); - } - - ComparisonSummary(int total, int equal, int different, int missingExpected, int missingActual, int errors, int other) { - this.total = total; - this.equal = equal; - this.different = different; - this.missingExpected = missingExpected; - this.missingActual = missingActual; - this.errors = errors; - this.other = other; - } - - ComparisonSummary record(String status) { - int t = total + 1; - int eq = equal; - int diff = different; - int mex = missingExpected; - int mac = missingActual; - int err = errors; - int oth = other; - switch (status) { - case "equal" -> eq++; - case "different" -> diff++; - case "missing_expected" -> mex++; - case "missing_actual" -> mac++; - case "error" -> err++; - default -> oth++; - } - return new ComparisonSummary(t, eq, diff, mex, mac, err, oth); + return new Arguments(compare, comment, summary, marker, title, successMessage); } } } diff --git a/scripts/generate-android-coverage-report.sh b/scripts/generate-android-coverage-report.sh index e0da4d6285..c5c276290b 100755 --- a/scripts/generate-android-coverage-report.sh +++ b/scripts/generate-android-coverage-report.sh @@ -4,67 +4,6 @@ set -euo pipefail cov_log() { echo "[generate-android-coverage] $1"; } -publish_coverage_preview() { - local source_dir="$1" html_index="$2" - local server_url="${GITHUB_SERVER_URL:-}" repository="${GITHUB_REPOSITORY:-}" token="${GITHUB_TOKEN:-${GH_TOKEN:-}}" - local run_id="${GITHUB_RUN_ID:-local}" run_attempt="${GITHUB_RUN_ATTEMPT:-1}" actor="${GITHUB_ACTOR:-github-actions[bot]}" - - if [ "$server_url" != "https://github.com" ]; then - cov_log "Skipping coverage preview publish (unsupported server: ${server_url:-})" - return 1 - fi - if [ -z "$repository" ] || [ -z "$token" ]; then - cov_log "Skipping coverage preview publish (missing repository or token)" - return 1 - fi - if [ ! -d "$source_dir" ]; then - cov_log "Skipping coverage preview publish (source directory missing: $source_dir)" - return 1 - fi - if [ -z "$html_index" ] || [ ! -f "$source_dir/$html_index" ]; then - cov_log "Skipping coverage preview publish (HTML index not found: $source_dir/$html_index)" - return 1 - fi - - local tmp_dir run_dir dest_dir remote_url commit_sha raw_base preview_base - tmp_dir="$(mktemp -d)" - run_dir="runs/${run_id}-${run_attempt}/android-coverage" - dest_dir="${tmp_dir}/${run_dir}" - mkdir -p "$dest_dir" - - cp -R "$source_dir"/. "$dest_dir"/ - - if ! git -C "$tmp_dir" init -b previews >/dev/null 2>&1; then - cov_log "Failed to initialize preview git repository" - rm -rf "$tmp_dir" - return 1 - fi - git -C "$tmp_dir" config user.name "$actor" >/dev/null - git -C "$tmp_dir" config user.email "github-actions@users.noreply.github.com" >/dev/null - git -C "$tmp_dir" add . >/dev/null - if ! git -C "$tmp_dir" commit -m "Publish Android coverage preview for run ${run_id} (attempt ${run_attempt})" >/dev/null 2>&1; then - cov_log "No changes to commit for coverage preview" - rm -rf "$tmp_dir" - return 1 - fi - - remote_url="${server_url}/${repository}.git" - remote_url="${remote_url/https:\/\//https://x-access-token:${token}@}" - if ! git -C "$tmp_dir" push --force "$remote_url" previews:quality-report-previews >/dev/null 2>&1; then - cov_log "Failed to push coverage preview to quality-report-previews" - rm -rf "$tmp_dir" - return 1 - fi - - commit_sha="$(git -C "$tmp_dir" rev-parse HEAD)" - raw_base="https://raw.githubusercontent.com/${repository}/${commit_sha}/${run_dir}" - preview_base="https://htmlpreview.github.io/?${raw_base}" - echo "${preview_base}/${html_index}" - - rm -rf "$tmp_dir" - return 0 -} - if [ $# -lt 1 ]; then cov_log "Usage: $0 " >&2 exit 2 @@ -121,124 +60,4 @@ rm -rf "$REPORT_DEST_DIR" mkdir -p "$REPORT_DEST_DIR" cp -R "$REPORT_SOURCE_DIR"/ "${REPORT_DEST_DIR}"/ -SUMMARY_OUT="$REPORT_DEST_DIR/coverage-summary.json" -ARTIFACT_NAME="android-coverage-report" -HTML_INDEX="jacocoAndroidReport/html/index.html" -REPORT_XML_PATH="$REPORT_DEST_DIR/jacocoAndroidReport.xml" - -if [ ! -f "$REPORT_XML_PATH" ]; then - alt_xml="$(find "$REPORT_DEST_DIR" -maxdepth 3 -type f -name '*.xml' | head -n1)" - if [ -n "$alt_xml" ]; then - cov_log "Using fallback coverage XML: $alt_xml" - REPORT_XML_PATH="$alt_xml" - fi -fi - -if preview_url=$(publish_coverage_preview "$REPORT_DEST_DIR" "$HTML_INDEX"); then - export ANDROID_COVERAGE_HTML_URL="$preview_url" - cov_log "Published coverage HTML preview: $ANDROID_COVERAGE_HTML_URL" -fi - -python3 - "$REPORT_XML_PATH" "$SUMMARY_OUT" "$ARTIFACT_NAME" "$HTML_INDEX" <<'PY' -import json -import sys -import os -from pathlib import Path -from xml.etree import ElementTree as ET - -xml_path = Path(sys.argv[1]) -summary_path = Path(sys.argv[2]) -artifact_name = sys.argv[3] -html_index = sys.argv[4] - -data = { - "artifact": artifact_name, - "html_index": html_index, - "counters": {}, - "top_classes": [], -} - -if not xml_path.is_file(): - json.dump(data, summary_path.open("w", encoding="utf-8"), indent=2) - sys.exit(0) - -def safe_int(value, default=0): - try: - return int(value) - except Exception: - return default - - -def format_class_name(package, class_name): - pkg = package.replace("/", ".").strip(".") - cls = class_name.replace("/", ".") - if pkg: - return f"{pkg}.{cls}" - return cls - - -def parse_class_coverage(root): - classes = [] - for package in root.findall("package"): - pkg_name = package.get("name", "") - for cls in package.findall("class"): - class_name = cls.get("name", "") - line_counter = None - for counter in cls.findall("counter"): - if counter.get("type") == "LINE": - line_counter = counter - break - if line_counter is None: - continue - missed = safe_int(line_counter.get("missed", 0)) - covered = safe_int(line_counter.get("covered", 0)) - total = missed + covered - if total <= 0: - continue - pct = covered / total * 100.0 - classes.append( - { - "name": format_class_name(pkg_name, class_name), - "missed": missed, - "covered": covered, - "total": total, - "coverage": pct, - } - ) - classes.sort(key=lambda c: (c["coverage"], -c["total"])) - return classes[:10] - - -try: - tree = ET.parse(xml_path) -except ET.ParseError: - json.dump(data, summary_path.open("w", encoding="utf-8"), indent=2) - sys.exit(0) - -root = tree.getroot() -for counter in root.findall("counter"): - ctype = counter.get("type") - missed = safe_int(counter.get("missed", 0)) - covered = safe_int(counter.get("covered", 0)) - total = missed + covered - pct = (covered / total * 100.0) if total else 0.0 - data["counters"][ctype] = { - "missed": missed, - "covered": covered, - "total": total, - "coverage": pct, - } - -data["top_classes"] = parse_class_coverage(root) - -html_url_env = os.environ.get("ANDROID_COVERAGE_HTML_URL") or os.environ.get("COVERAGE_HTML_URL") -if html_url_env: - data["html_url"] = html_url_env - -json.dump(data, summary_path.open("w", encoding="utf-8"), indent=2) -PY - cov_log "Copied Jacoco coverage report to $REPORT_DEST_DIR" -if [ -f "$SUMMARY_OUT" ]; then - cov_log "Wrote coverage summary to $SUMMARY_OUT" -fi diff --git a/scripts/run-android-instrumentation-tests.sh b/scripts/run-android-instrumentation-tests.sh index c953109bda..2e81e9bf9b 100755 --- a/scripts/run-android-instrumentation-tests.sh +++ b/scripts/run-android-instrumentation-tests.sh @@ -47,7 +47,6 @@ SCREENSHOT_REF_DIR="$SCRIPT_DIR/android/screenshots" SCREENSHOT_TMP_DIR="$(mktemp -d "${TMPDIR}/cn1ss-XXXXXX" 2>/dev/null || echo "${TMPDIR}/cn1ss-tmp")" ensure_dir "$SCREENSHOT_TMP_DIR" SCREENSHOT_PREVIEW_DIR="$SCREENSHOT_TMP_DIR/previews" -COVERAGE_SUMMARY="$ARTIFACTS_DIR/android-coverage-report/coverage-summary.json" ra_log "Loading workspace environment from $ENV_FILE" [ -f "$ENV_FILE" ] || { ra_log "Missing env file: $ENV_FILE"; exit 3; } @@ -144,17 +143,6 @@ done sleep 3 -ra_log "STAGE:COVERAGE -> Collecting Jacoco coverage report" -if ARTIFACTS_DIR="$ARTIFACTS_DIR" "$SCRIPT_DIR/generate-android-coverage-report.sh" "$GRADLE_PROJECT_DIR"; then - if [ -f "$COVERAGE_SUMMARY" ]; then - ra_log " -> Coverage summary detected at $COVERAGE_SUMMARY" - else - ra_log " -> Coverage summary not found after report generation" - fi -else - ra_log "WARNING: Coverage report generation failed; continuing without coverage details" -fi - declare -a CN1SS_SOURCES=("LOGCAT:$TEST_LOG") @@ -243,16 +231,10 @@ SUMMARY_FILE="$SCREENSHOT_TMP_DIR/screenshot-summary.txt" COMMENT_FILE="$SCREENSHOT_TMP_DIR/screenshot-comment.md" ra_log "STAGE:COMMENT_BUILD -> Rendering summary and PR comment markdown" -render_args=( - --compare-json "$COMPARE_JSON" - --comment-out "$COMMENT_FILE" - --summary-out "$SUMMARY_FILE" - --coverage-summary "$COVERAGE_SUMMARY" -) -if [ -n "${ANDROID_COVERAGE_HTML_URL:-}" ]; then - render_args+=(--coverage-html-url "${ANDROID_COVERAGE_HTML_URL}") -fi -if ! cn1ss_java_run "$RENDER_SCREENSHOT_REPORT_CLASS" "${render_args[@]}"; then +if ! cn1ss_java_run "$RENDER_SCREENSHOT_REPORT_CLASS" \ + --compare-json "$COMPARE_JSON" \ + --comment-out "$COMMENT_FILE" \ + --summary-out "$SUMMARY_FILE"; then ra_log "FATAL: Failed to render screenshot summary/comment" exit 14 fi