Merge pull request #8911 from edsantiago/prs_must_include_tests

CI: smoke test: insist on adding tests on PRs
This commit is contained in:
OpenShift Merge Robot 2021-01-20 05:57:09 -05:00 committed by GitHub
commit 54c465bda6
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
5 changed files with 223 additions and 4 deletions

View file

@ -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

View file

@ -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:

View file

@ -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

View file

@ -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
###############################################################################

View file

@ -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
}