Merge pull request #8911 from edsantiago/prs_must_include_tests

CI: smoke test: insist on adding tests on PRs

3 files changed, 211 insertions, 0 deletions

diff --git a/contrib/cirrus/pr-should-include-tests b/contrib/cirrus/pr-should-include-tests
new file mode 100755
index 000000000..caf27cf83
--- /dev/null
+++ b/contrib/cirrus/pr-should-include-tests
@@ -0,0 +1,70 @@
+#!/bin/bash
+#
+# Intended for use in CI: check git commits, barf if no tests added.
+#
+
+# Docs-only changes are excused
+if [[ "${CIRRUS_CHANGE_TITLE}" =~ CI:DOCS ]]; then
+    exit 0
+fi
+
+# So are PRs where 'NO TESTS NEEDED' appears in the Github message
+if [[ "${CIRRUS_CHANGE_MESSAGE}" =~ NO.TESTS.NEEDED ]]; then
+    exit 0
+fi
+
+# HEAD should be good enough, but the CIRRUS envariable allows us to test
+head=${CIRRUS_CHANGE_IN_REPO:-HEAD}
+# Base of this PR. Here we absolutely rely on cirrus.
+base=$(git merge-base ${DEST_BRANCH:-master} $head)
+
+# This gives us a list of files touched in all commits, e.g.
+#    A    foo.c
+#    M    bar.c
+# We look for Added or Modified (not Deleted!) files under 'test'.
+if git diff --name-status $base $head | egrep -q '^[AM]\s+(test/|.*_test\.go)'; then
+    exit 0
+fi
+
+# Nothing changed under test subdirectory.
+#
+# This is OK if the only files being touched are "safe" ones.
+filtered_changes=$(git diff --name-status $base $head |
+                       awk '{print $2}'               |
+                       fgrep -vx .cirrus.yml          |
+                       fgrep -vx changelog.txt        |
+                       fgrep -vx go.mod               |
+                       fgrep -vx go.sum               |
+                       egrep -v  '^[^/]+\.md$'        |
+                       egrep -v  '^contrib/'          |
+                       egrep -v  '^docs/'             |
+                       egrep -v  '^hack/'             |
+                       egrep -v  '^vendor/'           |
+                       egrep -v  '^version/')
+if [[ -z "$filtered_changes" ]]; then
+    exit 0
+fi
+
+# One last chance: perhaps the developer included the magic '[NO TESTS NEEDED]'
+# string in an amended commit.
+if git log --format=%B ${base}..${head} | fgrep '[NO TESTS NEEDED]'; then
+   exit 0
+fi
+
+cat <<EOF
+$(basename $0): PR does not include changes in the 'tests' directory
+
+Please write a regression test for what you're fixing. Even if it
+seems trivial or obvious, try to add a test that will prevent
+regressions.
+
+If your change is minor, feel free to piggyback on already-written
+tests, possibly just adding a small step to a similar existing test.
+Every second counts in CI.
+
+If your commit really, truly does not need tests, you can proceed
+by adding '[NO TESTS NEEDED]' to the body of your commit message.
+Please think carefully before doing so.
+EOF
+
+exit 1
diff --git a/contrib/cirrus/pr-should-include-tests.t b/contrib/cirrus/pr-should-include-tests.t
new file mode 100755
index 000000000..4618060a3
--- /dev/null
+++ b/contrib/cirrus/pr-should-include-tests.t
@@ -0,0 +1,137 @@
+#!/bin/bash
+#
+# tests for pr-should-include-tests.t
+#
+# FIXME: I don't think this will work in CI, because IIRC the git-checkout
+# is a shallow one. But it works fine in a developer tree.
+#
+ME=$(basename $0)
+
+###############################################################################
+# BEGIN test cases
+#
+# Feel free to add as needed. Syntax is:
+#    <exit status>  <sha of commit>  <branch>=<sha of merge base>  # comments
+#
+# Where:
+#    exit status       is the expected exit status of the script
+#    sha of merge base is the SHA of the branch point of the commit
+#    sha of commit     is the SHA of a real commit in the podman repo
+#
+# We need the actual sha of the merge base because once a branch is
+# merged 'git merge-base' (used in our test script) becomes useless.
+#
+#
+# FIXME: as of 2021-01-07 we don't have "no tests needed" in our git
+#        commit history, but once we do, please add a new '0' test here.
+#
+tests="
+0  68c9e02df  db71759b1   PR 8821: multiple commits, includes tests
+0  bb82c37b7  eeb4c129b   PR 8832: single commit, w/tests, merge-base test
+1  1f5927699  864592c74   PR 8685, multiple commits, no tests
+0  7592f8fbb  6bbe54f2b   PR 8766, no tests, but CI:DOCS in commit message
+0  355e38769  bfbd915d6   PR 8884, a vendor bump
+0  ffe2b1e95  e467400eb   PR 8899, only .cirrus.yml
+0  06a6fd9f2  3cc080151   PR 8695, docs-only, without CI:DOCS
+0  a47515008  ecedda63a   PR 8816, unit tests only
+0  caa84cd35  e55320efd   PR 8565, hack/podman-socat only
+0  c342583da  12f835d12   PR 8523, version.go + podman.spec.in
+0  c342583da  db1d2ff11   version bump to v2.2.0
+0  8f75ed958  7b3ad6d89   PR 8835, only a README.md change
+"
+
+# The script we're testing
+test_script=$(dirname $0)/$(basename $0 .t)
+
+# END   test cases
+###############################################################################
+# BEGIN test-script runner and status checker
+
+function run_test_script() {
+    local expected_rc=$1
+    local testname=$2
+
+    testnum=$(( testnum + 1 ))
+
+    # DO NOT COMBINE 'local output=...' INTO ONE LINE. If you do, you lose $?
+    local output
+    output=$( $test_script )
+    local actual_rc=$?
+
+    if [[ $actual_rc != $expected_rc ]]; then
+        echo "not ok $testnum $testname"
+        echo "#  expected rc $expected_rc"
+        echo "#  actual rc   $actual_rc"
+        if [[ -n "$output" ]]; then
+            echo "# script output: $output"
+        fi
+        rc=1
+    else
+        if [[ $expected_rc == 1 ]]; then
+            # Confirm we get an error message
+            if [[ ! "$output" =~ "Please write a regression test" ]]; then
+                echo "not ok $testnum $testname"
+                echo "# Expected: ~ 'Please write a regression test'"
+                echo "# Actual:   $output"
+                rc=1
+            else
+                echo "ok $testnum $testname"
+            fi
+        else
+            echo "ok $testnum $testname"
+        fi
+    fi
+
+    # If we expect an error, confirm that we can override it. We only need
+    # to do this once.
+    if [[ $expected_rc == 1 ]]; then
+        if [[ -z "$tested_override" ]]; then
+            testnum=$(( testnum + 1 ))
+
+            CIRRUS_CHANGE_TITLE="[CI:DOCS] hi there" $test_script &>/dev/null
+            if [[ $? -ne 0 ]]; then
+                echo "not ok $testnum $rest (override with CI:DOCS)"
+                rc=1
+            else
+                echo "ok $testnum $rest (override with CI:DOCS)"
+            fi
+
+            testnum=$(( testnum + 1 ))
+            CIRRUS_CHANGE_MESSAGE="hi there [NO TESTS NEEDED] bye" $test_script &>/dev/null
+            if [[ $? -ne 0 ]]; then
+                echo "not ok $testnum $rest (override with '[NO TESTS NEEDED]')"
+                rc=1
+            else
+                echo "ok $testnum $rest (override with '[NO TESTS NEEDED]')"
+            fi
+
+            tested_override=1
+        fi
+    fi
+}
+
+# END   test-script runner and status checker
+###############################################################################
+# BEGIN test-case parsing
+
+rc=0
+testnum=0
+tested_override=
+
+while read expected_rc parent_sha  commit_sha rest; do
+    # Skip blank lines
+    test -z "$expected_rc" && continue
+
+    export DEST_BRANCH=$parent_sha
+    export CIRRUS_CHANGE_IN_REPO=$commit_sha
+    export CIRRUS_CHANGE_TITLE=$(git log -1 --format=%s $commit_sha)
+    export CIRRUS_CHANGE_MESSAGE=
+
+    run_test_script $expected_rc "$rest"
+done <<<"$tests"
+
+echo "1..$testnum"
+exit $rc
+
+# END   Test-case parsing
+###############################################################################
diff --git a/contrib/cirrus/runner.sh b/contrib/cirrus/runner.sh
index 572f0b44a..e968fac45 100755
--- a/contrib/cirrus/runner.sh
+++ b/contrib/cirrus/runner.sh
@@ -31,7 +31,11 @@ function _run_smoke() {
     # $CIRRUS_TAG is only non-empty when executing due to a tag-push
     # shellcheck disable=SC2154
     if [[ -z "$CIRRUS_TAG" ]]; then
+        # If PR consists of multiple commits, test that each compiles cleanly
         make .gitvalidation
+
+        # PRs should include some way to test.
+        $SCRIPT_BASE/pr-should-include-tests
     fi
 }