Merge pull request #8911 from edsantiago/prs_must_include_tests

CI: smoke test: insist on adding tests on PRs

5 files changed, 223 insertions, 4 deletions

diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md
index 30fddf82b..cb8bce9ed 100644
--- a/CONTRIBUTING.md
+++ b/CONTRIBUTING.md
@@ -142,9 +142,13 @@ larger PRs into smaller ones - it's easier to review smaller
 code changes. But only if those smaller ones make sense as stand-alone PRs.
 
 Regardless of the type of PR, all PRs should include:
-* well documented code changes
-* additional testcases. Ideally, they should fail w/o your code change applied
-* documentation changes
+* well documented code changes.
+* additional testcases. Ideally, they should fail w/o your code change applied.
+  (With a few exceptions, CI hooks will block your PR unless your change
+  includes files named `*_test.go` or under the `test/` subdirectory. To
+  bypass this block, include the string `[NO TESTS NEEDED]` in your
+  commit message).
+* documentation changes.
 
 Squash your commits into logical pieces of work that might want to be reviewed
 separate from the rest of the PRs. But, squashing down to just one commit is ok
diff --git a/Makefile b/Makefile
index 8d1a34183..5b31ecfae 100644
--- a/Makefile
+++ b/Makefile
@@ -407,6 +407,10 @@ man-page-check:
 swagger-check:
 	hack/swagger-check
 
+.PHONY: tests-included
+tests-included:
+	contrib/cirrus/pr-should-include-tests
+
 .PHONY: codespell
 codespell:
 	codespell -S bin,vendor,.git,go.sum,changelog.txt,.cirrus.yml,"RELEASE_NOTES.md,*.xz,*.gz,*.tar,*.tgz,bin2img,*ico,*.png,*.1,*.5,copyimg,*.orig,apidoc.go" -L uint,iff,od,seeked,splitted,marge,ERRO,hist -w
@@ -644,7 +648,7 @@ validate.completions:
 	if [ -x /bin/fish ]; then /bin/fish completions/fish/podman.fish; fi
 
 .PHONY: validate
-validate: gofmt lint .gitvalidation validate.completions man-page-check swagger-check
+validate: gofmt lint .gitvalidation validate.completions man-page-check swagger-check tests-included
 
 .PHONY: build-all-new-commits
 build-all-new-commits:
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
 }