ci: make the GNU testsuite run and its PR comment resilient

GnuTests on #262 reported "PASSED: -8 / FAILED: +12" although the branch
changes nothing for the testsuite.

.github/workflows/GnuTests.yml

@@ -64,7 +64,16 @@ jobs:
         shell: bash
         run: |
           ## Run GNU diffutils testsuite
-          ./tests/run-upstream-testsuite.sh release || true
+          # Exit code 1 means some tests failed, which is expected and handled
+          # by the comparison below; 2 means the suite couldn't be run at all
+          # 'shell: bash' runs with -e, so don't let the expected exit code 1
+          # abort the step before the exit code is inspected
+          result=0
+          ./tests/run-upstream-testsuite.sh release || result=$?
+          if [[ $result -ge 2 ]]; then
+            echo "::error ::The GNU testsuite could not be run (exit code $result); see the log above"
+            exit 1
+          fi
         env:
           TERM: xterm
 
@@ -128,12 +137,22 @@ jobs:
             exit 1
           fi
 
+          if ! jq -e . "$RESULT_FILE" > /dev/null; then
+            echo "::error ::Test results at $RESULT_FILE are not valid JSON"
+            exit 1
+          fi
+
           TOTAL=$(jq '[.tests[]] | length' "$RESULT_FILE")
           PASS=$(jq '[.tests[] | select(.result=="PASS")] | length' "$RESULT_FILE")
           FAIL=$(jq '[.tests[] | select(.result=="FAIL")] | length' "$RESULT_FILE")
           SKIP=$(jq '[.tests[] | select(.result=="SKIP")] | length' "$RESULT_FILE")
           ERROR=0
 
+          if [[ "$TOTAL" -eq 0 ]]; then
+            echo "::error ::No test was run; refusing to report or compare an empty test run"
+            exit 1
+          fi
+
           output="GNU diffutils tests summary = TOTAL: $TOTAL / PASS: $PASS / FAIL: $FAIL / SKIP: $SKIP"
           echo "${output}"
 
@@ -175,14 +194,20 @@ jobs:
 
           IGNORE_INTERMITTENT=".github/workflows/ignore-intermittent.txt"
 
-          COMMENT_DIR="reference/comment"
+          # Build the comment in a directory of its own: 'reference/' holds the
+          # artifacts downloaded from the reference run, including its own
+          # 'comment' artifact, and re-uploading those would post a comparison
+          # belonging to another run (see https://github.com/uutils/diffutils/pull/262)
+          COMMENT_DIR="comment"
+          rm -rf ${COMMENT_DIR}
           mkdir -p ${COMMENT_DIR}
           echo ${{ github.event.number }} > ${COMMENT_DIR}/NR
           COMMENT_LOG="${COMMENT_DIR}/result.txt"
+          : > "${COMMENT_LOG}"
 
           COMPARISON_RESULT=0
           if test -f "${CURRENT_SUMMARY_FILE}"; then
-            if test -f "${REF_SUMMARY_FILE}"; then
+            if test -s "${REF_SUMMARY_FILE}"; then
               echo "Reference summary SHA1/ID: $(sha1sum -- "${REF_SUMMARY_FILE}")"
               echo "Current summary SHA1/ID: $(sha1sum -- "${CURRENT_SUMMARY_FILE}")"
 
@@ -193,7 +218,7 @@ jobs:
 
               COMPARISON_RESULT=$?
             else
-              echo "::warning ::Skipping test comparison; no prior reference summary is available at '${REF_SUMMARY_FILE}'."
+              echo "::warning ::Skipping test comparison; no usable reference summary is available at '${REF_SUMMARY_FILE}'."
             fi
           else
             echo "::error ::Failed to find summary of test results (missing '${CURRENT_SUMMARY_FILE}'); failing early"
@@ -203,6 +228,11 @@ jobs:
           if [ ${COMPARISON_RESULT} -eq 1 ]; then
             echo "::error ::Found new non-intermittent test failures"
             exit 1
+          elif [ ${COMPARISON_RESULT} -ge 2 ]; then
+            # The comparison itself failed (e.g. an unusable reference summary).
+            # Don't post a comment rather than post a misleading one.
+            : > "${COMMENT_LOG}"
+            echo "::warning ::Could not compare the test results against the reference"
           else
             echo "::notice ::No new test failures detected"
           fi
@@ -212,7 +242,7 @@ jobs:
         uses: actions/upload-artifact@v4
         with:
           name: comment
-          path: reference/comment/
+          path: comment/
 
       - name: Report test results
         if: success() || failure()

.gitignore

@@ -1,2 +1,3 @@
 /target
 *.swp
+/tests/test-results.json

tests/run-upstream-testsuite.sh

@@ -16,6 +16,11 @@
 # tests are run might not match exactly that used when the upstream tests are
 # run through the autotools.
 
+# Exit codes: 0 if all tests passed, 1 if at least one test failed, and 2 if
+# the test suite could not be run at all (e.g. the upstream repository couldn't
+# be fetched). Callers must treat 2 as an infrastructure error: no meaningful
+# result was produced.
+
 # By default it expects a release build of the diffutils binary, but a
 # different build profile can be specified as an argument
 # (e.g. 'dev' or 'test').
@@ -26,6 +31,12 @@
 scriptpath=$(dirname "$(readlink -f "$0")")
 rev=$(git rev-parse HEAD)
 
+# Report an infrastructure error: the test suite could not be run at all
+die() {
+  echo "ERROR: $*" >&2
+  exit 2
+}
+
 # Allow passing a specific profile as parameter (default to "release")
 profile="release"
 [[ -n $1 ]] && profile="$1"
@@ -34,35 +45,47 @@ profile="release"
 binary="$scriptpath/../target/$profile/diffutils"
 if [[ ! -x "$binary" ]]
 then
-  echo "Missing build for profile $profile"
-  exit 1
+  die "Missing build for profile $profile"
 fi
 
 # Work in a temporary directory
 tempdir=$(mktemp -d)
-cd "$tempdir"
+trap 'rm -rf "$tempdir"' EXIT
+cd "$tempdir" || die "Cannot enter temporary directory $tempdir"
 
-# Check out the upstream test suite
+# Check out the upstream test suite. git.savannah.gnu.org is regularly
+# unavailable or slow, so retry a few times before giving up.
 gitserver="https://git.savannah.gnu.org"
 testsuite="$gitserver/git/diffutils.git"
-echo "Fetching upstream test suite from $testsuite"
-git clone -n --depth=1 --filter=tree:0 "$testsuite" &> /dev/null
-cd diffutils
-git sparse-checkout set --no-cone tests &> /dev/null
-git checkout &> /dev/null
+attempts=3
+for (( attempt = 1; attempt <= attempts; attempt++ ))
+do
+  echo "Fetching upstream test suite from $testsuite (attempt $attempt/$attempts)"
+  rm -rf diffutils
+  git clone -n --depth=1 --filter=tree:0 "$testsuite" && break
+  (( attempt < attempts )) && sleep $(( attempt * 10 ))
+done
+[[ -d diffutils ]] || die "Failed to fetch the upstream test suite from $testsuite"
+cd diffutils || die "Failed to fetch the upstream test suite from $testsuite"
+git sparse-checkout set --no-cone tests &> /dev/null || die "Cannot sparse-checkout the upstream tests"
+git checkout &> /dev/null || die "Cannot check out the upstream tests"
 upstreamrev=$(git rev-parse HEAD)
+[[ -d tests ]] || die "The upstream checkout contains no tests directory"
 
 # Ensure that calling `diff` invokes the built `diffutils` binary instead of
 # the upstream `diff` binary that is most likely installed on the system
 mkdir src
-cd src
+cd src || die "Cannot create the directory holding the diff and cmp symlinks"
 ln -s "$binary" diff
 ln -s "$binary" cmp
-cd ../tests
+cd ../tests || die "Cannot enter the upstream tests directory"
 
 # Fetch tests/init.sh from the gnulib repository (needed since
 # https://git.savannah.gnu.org/cgit/diffutils.git/commit/tests?id=1d2456f539)
-curl -sL "$gitserver/gitweb/?p=gnulib.git;a=blob_plain;f=tests/init.sh;hb=HEAD" -o init.sh
+curl -sL --fail --retry 3 --retry-delay 5 --retry-all-errors \
+  "$gitserver/gitweb/?p=gnulib.git;a=blob_plain;f=tests/init.sh;hb=HEAD" -o init.sh \
+  || die "Failed to fetch tests/init.sh from the gnulib repository"
+[[ -s init.sh ]] || die "Fetched an empty tests/init.sh from the gnulib repository"
 
 if [[ -n "$TESTS" ]]
 then
@@ -73,6 +96,7 @@ else
   tests=$(make -f Makefile.am printtests)
 fi
 total=$(echo "$tests" | wc -w)
+(( total > 0 )) || die "No test to run: the upstream test list is empty"
 echo "Running $total tests"
 export LC_ALL=C
 export KEEP=yes
@@ -105,7 +129,9 @@ do
       # but there isn't much value added in doing so
       for file in *
       do
-        [[ -f "$file" ]] && json+="\"$file\":\"$(base64 -w0 < "$file")\","
+        # Encode the name with jq: some tests create files whose name contains
+        # quotes or control characters, which would produce invalid JSON
+        [[ -f "$file" ]] && json+="$(jq -Rn --arg name "$file" '$name'):\"$(base64 -w0 < "$file")\","
       done
       json="${json%,}}},"
       cd - > /dev/null
@@ -144,10 +170,16 @@ json="{$metadata $json}"
 
 # Clean up
 cd "$scriptpath"
-rm -rf "$tempdir"
 
+# Write the results out only once they are known to be valid JSON, so that a
+# malformed (or truncated) file is never left behind for the caller to consume
 resultsfile="test-results.json"
-echo "$json" | jq > "$resultsfile"
+if ! echo "$json" | jq > "$resultsfile.tmp"
+then
+  rm -f "$resultsfile.tmp"
+  die "Generated invalid JSON results"
+fi
+mv "$resultsfile.tmp" "$resultsfile"
 echo "Results written to $scriptpath/$resultsfile"
 
 (( failed > 0 )) && exit 1

util/compare_test_results.py

@@ -4,6 +4,10 @@
 Compare the current GNU test results to the last results gathered from the main branch to
 highlight if a PR is making the results better/worse.
 Don't exit with error code if all failing tests are in the ignore-intermittent.txt list.
+
+Exit status: 0 if the comparison shows no new failure, 1 if new non-intermittent
+failures appeared, and 2 if the comparison couldn't be performed at all (e.g. one
+of the result files is missing or malformed).
 """
 
 import json
@@ -49,7 +53,7 @@ def compare_results(current_file, reference_file, ignore_file=None, output_file=
         current_summary, current_failed = extract_test_results(current_data)
     except Exception as e:
         print(f"Error loading current results: {e}")
-        return 1
+        return 2
 
     try:
         with open(reference_file, "r") as f:
@@ -57,7 +61,7 @@ def compare_results(current_file, reference_file, ignore_file=None, output_file=
         reference_summary, reference_failed = extract_test_results(reference_data)
     except Exception as e:
         print(f"Error loading reference results: {e}")
-        return 1
+        return 2
 
     # Calculate differences
     pass_diff  = int(current_summary.get("passed",  0)) - int(reference_summary.get("passed",  0))