From: Alex Rousskov Date: Mon, 10 Oct 2022 04:24:22 +0000 (+0000) Subject: Start using Github Actions for CI tests (#1160) X-Git-Tag: SQUID_6_0_1~91 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=2ed8cc4;p=thirdparty%2Fsquid.git Start using Github Actions for CI tests (#1160) This change imports existing Sempaphore CI functionality test harness and migrates it to Github Actions. Using a stand-alone harness offered many advantages, but that approach goes against the current custom of importing CI configurations into being-tested repositories. Semaphore CI itself switched to such invasive configurations since v2. We have to start using invasive CI configurations to be able to evolve Squid CI and solve known CI problems. While importing the test harness, we made basic CI functionality tests available from the command line so that developers can run them even before submitting a PR and while addressing reviewer concerns. They do require internet access because the tests are downloaded from GitHub, but they avoid permanent service installations in developer environment. The tests do create/overwrite logs and start services like Squid. A lot more work is still required to make these tests user friendly. The new test configuration/harness consists of two primary parts: * Github Actions "workflow" configuration, including embedded scripts. * Various test scripts used by the "workflow" configuration above. There are many ways to split the test harness among those two parts. This particular split is based on these split-specific principles/ideas: 1. When running tests locally, developers do not want to risk disruptive, permanent, potentially dangerous actions such as globally installing new packages or overwriting checked out Squid sources. These actions belong to the workflow configuration file that is only interpreted by Github Actions, on Github virtual machines. The test scripts have only one sudo command (to start a Squid manipulating agent as nobody) which is bypassed of the agent is already running. 2. Developers should be able to repeat basic tests from the command line, against their version of Squid sources, their Squid builds, and without learning the ever-changing testing details. If their PR has failed a CI test, they may simple want to run the corresponding script locally to reproduce the issue. This means that nearly all testing logic belongs to the test scripts, not the Actions configuration file (that developers cannot easily interpret/apply). We also thought that some developers would want to separate functionality from code quality tests (because those are often applied at different development stages) and to repeat (many times) just the failed test case (because that often helps when triaging and fixing bugs). These considerations resulted in the creation of two scripts, each accepting individual test case names as optional parameters: * test-suite/test-functionality.sh * test-suite/test-sources.sh --- diff --git a/.github/workflows/default.yaml b/.github/workflows/default.yaml new file mode 100644 index 0000000000..2b078abdbd --- /dev/null +++ b/.github/workflows/default.yaml @@ -0,0 +1,123 @@ +# Test the default branch, supported release branches, and their PRs +name: GitHub CI + +on: + push: + branches: [ "master", "v5", "auto" ] + + pull_request: + branches: [ "master", "v5" ] + +env: + # empty except for pull_request events + PULL_REQUEST_NUMBER: ${{ github.event.pull_request.number }} + + # Full clones of Squid repository branches (depth=19000+) waste resources, + # while excessively shallow clones break tests that check for past commits + # (e.g., to skip a particular test until a known bug is fixed) or generate + # diffs against them (e.g., for `git diff --check`). This arbitrary limit + # tries to balance the two concerns. + CHECKOUT_FETCH_DEPTH: 1001 + +jobs: + + functionality-tests: + + runs-on: ubuntu-22.04 + + steps: + - name: Install prerequisite packages + run: | + sudo apt-get --quiet=2 update + sudo apt-get --quiet=2 install libtool-bin + + - name: Setup a nodejs environment + uses: actions/setup-node@v3 + with: + node-version: 16 + + - name: Checkout Squid sources + uses: actions/checkout@v3 + with: + fetch-depth: ${{ env.CHECKOUT_FETCH_DEPTH }} + + - run: ./bootstrap.sh + - run: ./configure --with-openssl + - run: make -j2 + - run: | + sudo make install + sudo chown -R nobody:nogroup /usr/local/squid + + - run: ./test-suite/test-functionality.sh + + # Squid logs are not readable to actions/upload-artifact below + - name: Prep test logs + if: success() || failure() + run: sudo chmod -R a+rX /usr/local/squid + + - name: Publish test logs + if: success() || failure() + uses: actions/upload-artifact@v3 + with: + name: test-logs + path: | + ${{ runner.temp }}/*.log + /usr/local/squid/var/logs/overlord/*.log + + source-maintenance-tests: + + runs-on: ubuntu-22.04 + + steps: + - name: Install prerequisite packages + run: | + sudo apt-get --quiet=2 update + sudo apt-get --quiet=2 install astyle + sudo apt-get --quiet=2 install gperf + pip install \ + --user \ + --no-cache-dir \ + --disable-pip-version-check \ + --quiet \ + --progress-bar off \ + codespell==1.16 # TODO: Upgrade to codespell v2 + + - uses: actions/checkout@v3 + with: + fetch-depth: ${{ env.CHECKOUT_FETCH_DEPTH }} + + - run: ./test-suite/test-sources.sh + + build-tests: + + strategy: + fail-fast: true + matrix: + os: [ ubuntu-22.04 ] + + runs-on: ${{ matrix.os }} + + steps: + + - name: Install prerequisite Linux packages + if: runner.os == 'Linux' + run: | + sudo apt-get --quiet=2 update + sudo apt-get --quiet=2 install libtool-bin + sudo apt-get --quiet=2 install libcppunit-dev + sudo apt-get --quiet=2 install libldap2-dev + # TODO: Add some deb-src URI to sources.list and do build-dep + # instead of the above installs? + # sudo apt-get --quiet=2 build-dep squid + + - name: Checkout sources + uses: actions/checkout@v3 + + - run: ./test-builds.sh + + - name: Publish build logs + if: success() || failure() + uses: actions/upload-artifact@v3 + with: + name: build-logs-${{ runner.os }} + path: btlayer-*.log diff --git a/test-suite/Makefile.am b/test-suite/Makefile.am index 6dfef1c10c..2c98b313d4 100644 --- a/test-suite/Makefile.am +++ b/test-suite/Makefile.am @@ -22,6 +22,8 @@ EXTRA_PROGRAMS = mem_node_test splay tcp-banger2 EXTRA_DIST = \ $(srcdir)/squidconf/* \ + test-functionality.sh \ + test-sources.sh \ test-squid-conf.sh \ testheaders.sh diff --git a/test-suite/test-functionality.sh b/test-suite/test-functionality.sh new file mode 100755 index 0000000000..0fd2d9f006 --- /dev/null +++ b/test-suite/test-functionality.sh @@ -0,0 +1,264 @@ +#!/bin/sh +# +## Copyright (C) 1996-2022 The Squid Software Foundation and contributors +## +## Squid software is distributed under GPLv2+ license and includes +## contributions from numerous individuals and organizations. +## Please see the COPYING and CONTRIBUTORS files for details. +## + +# This script tests a few Squid functionality requirements. +# It is suitable for automated CI and manual testing. + +# Default-set and report used environment variables: +# * the root directory for storing test tools and test artifacts. +echo "TMPDIR=${TMPDIR:=${RUNNER_TEMP:-/tmp}}" +# * directory for cloning git repositories containing various test tools +echo "CLONES_DIR=${CLONES_DIR:=$TMPDIR/clones}" +# * directories of cloned repositories +echo "DAFT_DIR=${DAFT_DIR:=$CLONES_DIR/daft}" +echo "SQUID_DAFTS_DIR=${SQUID_DAFTS_DIR:=$CLONES_DIR/squid-dafts}" +echo "SQUID_OVERLORD_DIR=${SQUID_OVERLORD_DIR:=$CLONES_DIR/squid-overlord}" + +# print an error message (with special markers recognized by GitHub Actions) +echo_error() { + echo "::error ::" "$@" +} + +# print a warning message (with special markers recognized by GitHub Actions) +echo_warning() { + echo "::warning ::" "$@" +} + +run() { + echo "running: $@" + "$@" +} + +# Whether the branch has a commit with a message matching the given regex. +# The first argument is the SHA of the primary commit with the desired change. +# We want to find up/backported commits that will have different SHAs and that +# may have minor commit message variations, so that SHA is for reference only. +has_commit_by_message() { + local commit="$1" + shift + + if git log --grep "$@" | grep -q ^commit + then + return 0; + fi + + echo "Was looking for branch commit with a message matching the following" + echo " regex: " "$@" + echo " This code lacks commit $commit or equivalent." + return 1; +} + +clone_repo() { + local repo_url="$1" + local destination_dir="$2" + + if test -e $destination_dir + then + echo "Skipping already fetched $destination_dir" + elif run git clone --no-tags --quiet --depth=1 --branch production -- "$repo_url" "$destination_dir" + then + if test -e "$destination_dir/package.json" + then + ( cd $destination_dir && run npm install --no-audit --no-save ) || return + fi + else + echo_error "Failed to fetch $repo_url into $destination_dir" + return 1 + fi + + (cd $destination_dir && echo "Using commit `git rev-parse HEAD`") +} + +start_overlord() { + local url=http://localhost:13128/ + local log=$TMPDIR/squid-overlord.log + if test -e $log && curl -H 'Pop-Version: 4' --no-progress-meter $url/check > /dev/null + then + echo "Will reuse squid-overlord service running at $url" + return 0; + fi + + # Do not be tempted to simply run `sudo ... overlord.pl`: User nobody will + # lack read permissions, and sudo will ask for a password. + sudo -n --background -u nobody perl < $SQUID_OVERLORD_DIR/overlord.pl > $log 2>&1 || return + echo "Started squid-overlord service at $url" +} + +setup_test_tools() { + echo "::group::Setup test tools" + + clone_repo https://github.com/measurement-factory/daft $DAFT_DIR || return + clone_repo https://github.com/measurement-factory/squid-dafts $SQUID_DAFTS_DIR || return + clone_repo https://github.com/measurement-factory/squid-overlord $SQUID_OVERLORD_DIR || return + + if ! test -e $SQUID_DAFTS_DIR/src + then + run ln -s `realpath $DAFT_DIR/src` $SQUID_DAFTS_DIR/src || return + fi + if ! test -e $SQUID_DAFTS_DIR/node_modules + then + run ln -s `realpath $DAFT_DIR/node_modules` $SQUID_DAFTS_DIR/node_modules || return + fi + + start_overlord || return + + # TODO: Find a good way to end group on (currently fatal) errors as well. + echo "::endgroup::" +} + +# executes a single test after the caller confirms that the test is applicable +run_confirmed_test() { + local testId="$1" + + local testRunner="$DAFT_DIR/src/cli/daft.js" + if ! test -e $testRunner + then + echo_error "Missing Daft test execution script" + echo "Expected to find it at $testRunner" + exit 1; + fi + + local testsDir="$SQUID_DAFTS_DIR/tests" + if ! test -d $testsDir + then + echo_error "Missing collection of Squid-specific Daft tests" + echo "Expected to find them in $testsDir/" + exit 1; + fi + + local testScript=$testsDir/$testId.js + if ! test -e $testScript + then + echo_error "Unknown test requested: $testId" + echo "Expected to find it at $testScript" + return 1; + fi + + local log="$TMPDIR/$testId.log" + + echo "Running test: $testId" + local result=undefined + if $testRunner run $testScript > $log 2>&1 + then + echo "Test $testId: OK" + return 0; + else + result=$? + fi + + # TODO: Report skipped tests and ignored failures more prominently. See + # test-sources.sh for CHECK_OUTCOME_PHRASE tricks (but avoid duplication). + echo + echo_error "Test $testId: Failed with exit code $result" + echo "::group::Test log tail:" + tail -n 100 $log + echo "::endgroup::" + + # TODO: Link to the artifact + echo "See the test log (tailed above) for failure details: $log" + return $result +} + +check_pconn() { + run_confirmed_test pconn +} + +check_busy_restart() { + if ! run_confirmed_test busy-restart + then + # XXX: Make the test stable instead! + echo_warning "Ignoring unstable test failure: busy-restart" + fi + return 0 +} + +check_proxy_collapsed_forwarding() { + if ! has_commit_by_message 1af789e 'Do not stall if xactions overwrite a recently active' + then + echo "No proxy-collapsed-forwarding due to stalling transactions" + return 0; + fi + run_confirmed_test proxy-collapsed-forwarding +} + +check_proxy_update_headers_after_304() { + if grep 'AC_INIT.*Proxy.,.[1234][.]' configure.ac + then + echo "No proxy-update-headers-after-304 until v5"; + return 0; + fi + run_confirmed_test proxy-update-headers-after-304 +} + +check_upgrade_protocols() { + if ! grep -q http_upgrade_request_protocols src/cf.data.pre + then + echo "No upgrade-protocols without http_upgrade_request_protocols support"; + return 0; + fi + run_confirmed_test upgrade-protocols +} + +# executes a single check_name test named by the parameter +run_one_test() { + local testName=$1 + + # convert a test name foo into a check_foo() function name suffix; e.g. + # busy-restart becomes busy_restart (to be called below as check_busy_restart) + check=`echo $testName | sed s/-/_/g` + + check_$check +} + +# executes all of the given tests, providing a summary of their failures +run_tests() { + local result=0 + local failed_tests="" + for testName in "$@" + do + if run_one_test $testName + then + continue; + else + result=$? + failed_tests="$failed_tests $testName" + fi + done + + if test -n "$failed_tests" + then + echo_error "Failed test(s):$failed_tests" + fi + return $result +} + +# run the tests named on the command line (if any) or the default tests (otherwise) +main() { + + setup_test_tools || return + + local tests="$@" + + if test -z "$tests" + then + local default_tests=" + pconn + proxy-update-headers-after-304 + upgrade-protocols + proxy-collapsed-forwarding + busy-restart + " + tests="$default_tests" + fi + + run_tests $tests +} + +main +exit $? diff --git a/test-suite/test-sources.sh b/test-suite/test-sources.sh new file mode 100755 index 0000000000..e8bfb33aac --- /dev/null +++ b/test-suite/test-sources.sh @@ -0,0 +1,247 @@ +#!/bin/sh +# +## Copyright (C) 1996-2022 The Squid Software Foundation and contributors +## +## Squid software is distributed under GPLv2+ license and includes +## contributions from numerous individuals and organizations. +## Please see the COPYING and CONTRIBUTORS files for details. +## + +# This script checks source code compliance with a few Squid Project policies +# and recommendations. Some of the checks are applied to a subset of +# repository files, depending on the environment variables (first match wins): +# * If $PULL_REQUEST_NUMBER is set, checks sources modified by that GitHub PR; +# * If $FORK_POINT is set, checks sources modified since that commit; +# * Otherwise, checks sources modified since the parent commit (HEAD^). +STARTING_POINT="HEAD^" +if test -n "${PULL_REQUEST_NUMBER}" +then + STARTING_POINT="refs/pull/${PULL_REQUEST_NUMBER}/merge^1" +elif test -n "${FORK_POINT}" +then + STARTING_POINT="${FORK_POINT}" +fi + +# Customizable check outcome description logged by run_one_check() +CHECK_OUTCOME_PHRASE="" + +# XXX: echo_*() and run() functions are duplicated in test-functionality.sh. + +# print an error message (with special markers recognized by GitHub Actions) +echo_error() { + echo "::error ::" "$@" +} + +# print a warning message (with special markers recognized by GitHub Actions) +echo_warning() { + echo "::warning ::" "$@" +} + +# print and execute the given command +run() { + echo "running: $@" + "$@" +} + +# Get key pull request commits to be able to diff against them, avoiding: +# 'refs/pull/167/merge^1': unknown revision or path not in the working tree +fetch_pr_refs() { + # TODO: There may be a simpler way to get the necessary commits via GitHub + # Actions contexts: github.event.pull_request.base.sha and github.sha. + + # quiet "adding host key" warnings + export GIT_SSH_COMMAND="ssh -o LogLevel=ERROR" + run git fetch --force --quiet origin \ + "refs/pull/${PULL_REQUEST_NUMBER}/merge:refs/pull/${PULL_REQUEST_NUMBER}/merge" \ + "refs/pull/${PULL_REQUEST_NUMBER}/head:refs/pull/${PULL_REQUEST_NUMBER}/head"; +} + +# check changed lines for conflict markers or whitespace errors +check_diff() { + if run git -c core.whitespace=-blank-at-eof diff --check ${STARTING_POINT} + then + return 0; + fi + + local authorEmail="`git show --format="%ae" HEAD`"; + if test "$authorEmail" = 'squidadm@users.noreply.github.com' + then + CHECK_OUTCOME_PHRASE="Ignored 'git diff --check' failure for an automated commit"; + return 0; + fi + + echo_error "git diff detected bad whitespace changes." + echo "Please see 'git diff --check' output above for details." + return 1; +} + +# check changed files for certain misspelled words +check_spelling() { + if ! test -e ./scripts/spell-check.sh + then + CHECK_OUTCOME_PHRASE="Skipped because this Squid version is missing support for spelling checks." + return 0; + fi + + if ! git diff --quiet + then + CHECK_OUTCOME_PHRASE="Skipped due to a dirty working directory" + return 0; + fi + + # To avoid flagging out-of-scope misspellings, only check modified files. + # This also speeds up tests and helps avoid slow-codespell timeouts. + local changed_files="`git diff --name-only ${STARTING_POINT}`" + if test -z "$changed_files" + then + echo "ERROR: Unable to determine which files have changed." + return 1; # be conservative until we learn why that is a bad idea + fi + echo "changed files: $changed_files" + + run ./scripts/spell-check.sh $changed_files + + if run git diff --word-diff --exit-code + then + return 0; + fi + + echo_error "Spelling mistakes detected." + echo "The log above ends with a word diff showing suggested fixes." + echo "Please adjust scripts/codespell-whitelist.txt or fix spelling." + + return 1; +} + +# check for certain problems fixed by a ./scripts/source-maintenance.sh +check_source_maintenance() { + if ! git diff --quiet + then + CHECK_OUTCOME_PHRASE="Skipped due to a dirty working directory" + return 0; + fi + + local checker=./scripts/source-maintenance.sh + + local copyrightOption='--check-and-update-copyright' + if ! grep -q -e $copyrightOption $checker + then + echo_warning "Skipping $checker checks because $checker does not support $copyrightOption" + CHECK_OUTCOME_PHRASE="Skipped due to missing $copyrightOption support" + return 0; + fi + + # The OS may not provide the right version of the astyle package, but + # source formatting (by developers) is not yet enforced, so we run with + # whatever astyle is provided, abusing the fact that $checker skips + # formatting iff it can execute a binary called astyle but does not like + # astyle's version. + + # Avoid distracting $checker warnings; TODO: Fix $checker instead. + touch boilerplate_fix.sed + + if ! run $checker $copyrightOption no + then + echo_error "Running $checker modified sources" + CHECK_OUTCOME_PHRASE="Ignored $checker failure" # maybe overwritten below + # TODO: Require source-maintenance.sh application instead of ignoring this error. + fi + + if run git diff --exit-code + then + return 0 + fi + + echo_error "Running $checker modified sources" + echo "The diff above details these modifications. Consider running $checker." + # TODO: Provide a downloadable patch that developers can apply. + CHECK_OUTCOME_PHRASE="Ignored the need to run $checker" + # TODO: Require source-maintenance.sh application instead of ignoring these changes. + return 0 +} + +run_one_check() { + local checkName=$1 + + # convert a check name foo into a check_foo() function name + # e.g. busy-restart becomes check_busy_restart + local check=`echo $checkName | sed s/-/_/g` + + CHECK_OUTCOME_PHRASE="" + + echo "::group::Check $checkName" + local result=undefined + check_$check + result=$? + + if test "$result" -eq 0 + then + if test -n "$CHECK_OUTCOME_PHRASE" + then + echo "::endgroup::" + # place this custom outcome outside of the check group so that it + # remains visible in the default/unexpanded Actions job log + echo_warning "Check $checkName: $CHECK_OUTCOME_PHRASE" + else + echo "Check $checkName: OK" + echo "::endgroup::" + fi + else + echo "Check exit code: $result" + echo "::endgroup::" + # place this error message outside of the check group so that it + # remains visible in the default/unexpanded Actions job log + echo_error "Check $checkName: ${CHECK_OUTCOME_PHRASE:-Failure}" + fi + + return $result +} + +# executes all of the given checks, providing a summary of their failures +run_checks() { + local result=0 + local failed_checks="" + + for checkName in "$@" + do + if run_one_check $checkName + then + continue; + else + result=$? + failed_checks="$failed_checks $checkName" + fi + done + + if test -n "$failed_checks" + then + echo_error "Failed check(s):$failed_checks" + fi + return $result +} + +# run the checks named on the command line (if any) or the default checks (otherwise) +main() { + + if test -n "${PULL_REQUEST_NUMBER}" + then + fetch_pr_refs || return + fi + echo "Starting point: $STARTING_POINT (`git rev-parse $STARTING_POINT`)" + + checks="$@" + if test -z "$checks" + then + local default_checks=" + diff + spelling + source-maintenance + " + checks="$default_checks" + fi + + run_checks $checks +} + +main +exit $?