diff options
| -rw-r--r-- | CONTRIBUTING.md | 10 | ||||
| -rw-r--r-- | Makefile | 6 | ||||
| -rwxr-xr-x | contrib/cirrus/pr-should-include-tests | 70 | ||||
| -rwxr-xr-x | contrib/cirrus/pr-should-include-tests.t | 137 | ||||
| -rwxr-xr-x | contrib/cirrus/runner.sh | 4 |
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 @@ -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 b0060163e..e08bb5f19 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 } |