Fix shell script tests (#798)

* releaselib_test.sh: Add boilerplate header * Use shared test harness * Fix releaselib tests * Cannot export a map value Fixes: ``` Step #2: /workspace/go/src/k8s.io/release/lib/gitlib.sh: line 137: export: `PROGSTEP[gitlib::github_acls]': not a valid identifier ``` * Fix gitlib tests * Run common_test.sh with bazel * Adhere to style guide * Add TODO to make test scripts more strict
kubernetes · Jul 26, 2019 · cc72134 · cc72134
1 parent 4fd464e
commit cc72134
Show file tree

Hide file tree

Showing 8 changed files with 169 additions and 59 deletions.
diff --git a/anago b/anago
@@ -988,7 +988,7 @@ announce () {
 
     # When we create a new branch, we notify the publishing-bot folks by
     # creating an issue for them
-    gitlib::create_publishing_bot_issue "$RELEASE_BRANCH" || {
+    gitlib::create_publishing_bot_issue "$RELEASE_BRANCH" | gitlib::get_issue_url || {
       logecho "$WARNING: Could not create issue for the publishing-bot update"
     }
   else

diff --git a/lib/BUILD.bazel b/lib/BUILD.bazel
@@ -8,3 +8,8 @@ sh_test(
     srcs = ["gitlib_test.sh"],
     data = glob(["testdata/gitlib/*"]),
 )
+
+sh_test(
+    name = "common",
+    srcs = ["common_test.sh"],
+)
diff --git a/lib/common_test.sh b/lib/common_test.sh
@@ -23,7 +23,14 @@ source "$(dirname "$(readlink -ne "${BASH_SOURCE[0]}")")/testing.sh"
 source "$(dirname "$(readlink -ne "${BASH_SOURCE[0]}")")/common.sh"
 readonly TESTDATA="$( cd "$(dirname "$0")" && pwd )/testdata"
 
-set -e
+# TODO: We should see to
+#       - move that to the start of the script
+#       - add `set -o nounset`
+#
+#       We can do that when all the things we source do not rely on unset
+#       varaibales and ignoring errors. This will require quite some
+#       refactoring, so this is the best we can do for now.
+set -o errexit
 set -o pipefail
 
 TEST_run_stateful() {
@@ -39,10 +46,10 @@ TEST_run_stateful() {
     TPUT[BOLD]='' \
     TPUT[OFF]=''
 
-  assertEqualContent \
+  assert_equal_content \
     <( common::run_stateful --strip-args 'printf %s\n%s arg1 arg2' ) \
     <( echo -e "\n\nprintf\n\n\narg1\narg2" ) \
     "passing command and arguments"
 }
 
-testMain "$@"
+test_main "$@"
diff --git a/lib/gitlib.sh b/lib/gitlib.sh
@@ -136,6 +136,8 @@ gitlib::github_api_token () {
 ##############################################################################
 # Checks github ACLs
 # returns 1 on failure
+# Disable shellcheck for dynamically defined variable
+# shellcheck disable=SC2034,SC2154
 PROGSTEP[gitlib::github_acls]="CHECK GITHUB AUTH"
 gitlib::github_acls () {
 
@@ -293,7 +295,9 @@ gitlib::repo_state () {
   local expectedRemote="${RELEASE_TOOL_REPO:-[:/]kubernetes/release}"
   local expectedBranch="${RELEASE_TOOL_BRANCH:-master}"
 
-  local branch=$(gitlib::current_branch "$TOOL_ROOT") || return 1
+  local branch
+  branch=$(gitlib::current_branch "$TOOL_ROOT") || return 1
+
   if [ "${expectedBranch}" != "$branch" ]
   then
     logecho "$FAILED checked out branch $branch is not the same as $expectedBranch"
@@ -350,6 +354,15 @@ gitlib::create_issue() {
   ${GHCURL} "${K8S_GITHUB_API_ROOT}/${repo}/issues" --data "$data"
 }
 
+
+###############################################################################
+# Extract the issue url from an issue creation response
+gitlib::get_issue_url() {
+  local url=''
+  url="$( jq -r '.html_url // ""' || echo '' )"
+  echo "$url"
+}
+
 ###############################################################################
 # Post an issue for the publishing bot, to ask them to update their
 # configuration

diff --git a/lib/gitlib_test.sh b/lib/gitlib_test.sh
@@ -1,4 +1,19 @@
 #!/usr/bin/env bash
+#
+# Copyright 2019 The Kubernetes Authors.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
 #
 # gitlib.sh unit tests
 #
@@ -8,6 +23,16 @@ source $TOOL_LIB_PATH/releaselib.sh
 
 readonly TESTDATA="$( cd "$(dirname "$0")" && pwd )/testdata"
 
+# TODO: We should see to
+#       - move that to the start of the script
+#       - add `set -o nounset`
+#
+#       We can do that when all the things we source do not rely on unset
+#       varaibales and ignoring errors. This will require quite some
+#       refactoring, so this is the best we can do for now.
+set -o errexit
+set -o pipefail
+
 TEST_ver_regex() {
   echo "Testing VER_REGEX:"
   echo
@@ -67,25 +92,45 @@ TEST_create_issue() {
   # shellcheck disable=SC2034
   local GHCURL='echo'
 
-  assertEqualContent \
+  assert_equal_content \
     <( gitlib::create_issue "$repo" "$title" "$body" ) \
     "${TESTDATA}/gitlib/create_issue.txt" \
     'creating an issue'
 
-  assertEqualContent \
+  assert_equal_content \
     <( gitlib::create_issue "$repo" "$title" "$body" 12345 ) \
     "${TESTDATA}/gitlib/create_issue_milestone.txt" \
     'creating an issue with milestone'
 }
 
+TEST_get_issue_url() {
+  echo "Testing gitlib::get_issue_url"
+  echo
+
+  assert_equal_content \
+    <( echo '{ }' | gitlib::get_issue_url ) \
+    <( echo '' ) \
+    "should return an empty issue url"
+
+  assert_equal_content \
+    <( echo '{ "html_url" : "some issue url" }' | gitlib::get_issue_url ) \
+    <( echo 'some issue url' ) \
+    "should return the issue's url"
+
+  assert_equal_content \
+    <( echo '{ "borken }' | gitlib::get_issue_url ) \
+    <( echo '' ) \
+    "should not fail on malformed input"
+}
+
 TEST_create_publishing_bot_issue() {
   echo "Testing gitlib::create_publishing_bot_issue"
   echo
 
   # shellcheck disable=SC2034
   local GHCURL='echo'
 
-  assertEqualContent \
+  assert_equal_content \
     <( gitlib::create_publishing_bot_issue 'release-1.14' ) \
     "${TESTDATA}/gitlib/create_publishing_bot_issue.txt" \
     "simple, mock issue without special settings something"
@@ -97,7 +142,7 @@ TEST_create_publishing_bot_issue() {
     echo 'memberOne'
     echo 'memberTwo'
   }
-  assertEqualContent \
+  assert_equal_content \
     <( gitlib::create_publishing_bot_issue 'release-1.13' ) \
     "${TESTDATA}/gitlib/create_publishing_bot_issue_nomock.txt" \
     "for a nomock release, different repo & assignments are used"
@@ -106,7 +151,7 @@ TEST_create_publishing_bot_issue() {
   local FLAGS_nomock=1
   # mock gitlib::get_team_members
   gitlib::get_team_members() { :; }
-  assertEqualContent \
+  assert_equal_content \
     <( gitlib::create_publishing_bot_issue 'release-1.13' ) \
     "${TESTDATA}/gitlib/create_publishing_bot_issue_nomock_noassign.txt" \
     "for a nomock release, different repo is used, but no assignments when team members cannot be found"
@@ -122,21 +167,21 @@ TEST_get_team_members() {
   mock_github_api() {
     echo '{"data": {"organization":{"team":{"members":{"nodes":[{"login":"blipp"},{"login":"blupp"}]}}}}}'
   }
-  assertEqualContent \
+  assert_equal_content \
     <( gitlib::get_team_members 'ignored' 'ignored' ) \
     <( echo -e "blipp\nblupp" ) \
     'get_team_members issues a graphql qeury against the github API and pareses the response'
 
   mock_github_api() {
     echo 'this is some invalid response'
   }
-  assertEqualContent \
+  assert_equal_content \
     <( gitlib::get_team_members 'ignored' 'ignored' ) \
     <( echo -n '' ) \
     'get_team_members can handle invalid responses and reports no members'
 }
 
-assertEqualContent() {
+assert_equal_content() {
   local actual_file="$1"
   local expected_file="$2"
   local message="${3:-files do not match content}"

diff --git a/lib/releaselib.sh b/lib/releaselib.sh
@@ -774,7 +774,10 @@ release::gcs::verify_latest_update () {
   local -r version_prerelease_rev="${BASH_REMATCH[5]}"
   local -r version_commits="${BASH_REMATCH[7]}"
 
-  if gcs_version="$($GSUTIL cat $publish_file_dst 2>/dev/null)"; then
+  local -a catCmd=( 'cat' "$publish_file_dst" )
+  [ -n "${GSUTIL:-}" ] && catCmd=( "$GSUTIL" "${catCmd[@]}" )
+
+  if gcs_version="$( "${catCmd[@]}" )"; then
     if ! [[ $gcs_version =~ ${VER_REGEX[release]}(.${VER_REGEX[build]})* ]]; then
       logecho -r "$FAILED"
       logecho "* file contains invalid release version," \

diff --git a/lib/releaselib_test.sh b/lib/releaselib_test.sh
@@ -1,26 +1,62 @@
 #!/usr/bin/env bash
+#
+# Copyright 2019 The Kubernetes Authors.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
 #
 # releaselib.sh unit tests
 #
-source $(dirname $(readlink -ne $BASH_SOURCE))/common.sh
-source $TOOL_LIB_PATH/gitlib.sh
-source $TOOL_LIB_PATH/releaselib.sh
+# shellcheck source=./lib/common.sh
+source "$(dirname "$(readlink -ne "${BASH_SOURCE[0]}")")/common.sh"
+# shellcheck source=./lib/testing.sh
+source "${TOOL_LIB_PATH}/testing.sh"
+# shellcheck source=./lib/gitlib.sh
+source "${TOOL_LIB_PATH}/gitlib.sh"
+# shellcheck source=./lib/releaselib.sh
+source "${TOOL_LIB_PATH}/releaselib.sh"
+
+# TODO: We should see to
+#       - move that to the start of the script
+#       - add `set -o nounset`
+#
+#       We can do that when all the things we source do not rely on unset
+#       varaibales and ignoring errors. This will require quite some
+#       refactoring, so this is the best we can do for now.
+set -o errexit
+set -o pipefail
 
+TEST_verify_latest_update() {
+  ##############################################################################
+  # TESTING release::gcs::verify_latest_update()
+  ##############################################################################
+  # stable is always vX.Y.Z
+  # latest is always vX.Y.Z-(alpha|beta|rc).N
 
-##############################################################################
-# TESTING release::gcs::verify_latest_update()
-##############################################################################
-# stable is always vX.Y.Z
-# latest is always vX.Y.Z-(alpha|beta|rc).N
+  # > vs >= scenarios only relevant to 'latest' .N's.
+  # type release updates on >
+  # type ci updates on >=
 
-# > vs >= scenarios only relevant to 'latest' .N's.
-# type release updates on >
-# type ci updates on >=
+  published_file="$( mktemp -t 'published.XXXXXXXX')"
+  trap 'rm -f "$published_file"' EXIT
 
-published_file=$TMPDIR/published.$$
+  # We want to make sure that we explictly not use gsutils in this test, so
+  # that we do a `cat $file` instead of a `gsutil cat $file` (`gsutil cat` only
+  # supports remote urls, but not local files).
+  export GSUTIL=''
 
-# Fill $data with testing values and expected state
-read -r -d '' data <<'EOF' || true
+  # Fill $data with testing values and expected state
+  read -r -d '' data <<'EOF' || true
 ###############################################
 # stable test scenarios
 # release and ci use same logic here
@@ -54,35 +90,36 @@ read -r -d '' data <<'EOF' || true
 ## release v1.4.0         v1.4.0-rc.1    0
 EOF
 
+  # Test the data
+  # disable shellcheck for comment variable
+  # shellcheck disable=SC2034
+  while read -r comment type version pub_version expected; do
+    # Prepare test
+    echo "$pub_version" > "$published_file"
 
-# Test the data
-while read comment type version pub_version expected; do
-  # Prepare test
-  echo $pub_version > $published_file
-
-  # $type value passed in simply to trigger > vs. >= condition
-  # arg 2 (bucket) not used with optional arg 4 passed in
-  # arg 3 (version) is the incoming version to check
-  # arg 4 simply points to a local file to set a 'published' version
-  if release::gcs::verify_latest_update $type "" $version $published_file; then
-    echo -n "TEST CASE: "
-    case $expected in
-      0) echo "$PASSED" ;;
-      *) echo "$FAILED" ;;
-    esac
-  else
-    echo -n "TEST CASE: "
-    case $expected in
-      1) echo "$PASSED" ;;
-      *) echo "$FAILED" ;;
-    esac
-  fi
-  echo
-done < <(echo "$data" |egrep '^## ')
+    # $type value passed in simply to trigger > vs. >= condition
+    # arg 2 (bucket) not used with optional arg 4 passed in
+    # arg 3 (version) is the incoming version to check
+    # arg 4 simply points to a local file to set a 'published' version
+    if release::gcs::verify_latest_update "$type" "" "$version" "$published_file"; then
+      echo -n "TEST CASE: "
+      case $expected in
+        0) echo "$PASSED" ;;
+        *) echo "$FAILED" ; return 1 ;;
+      esac
+    else
+      echo -n "TEST CASE: "
+      case $expected in
+        1) echo "$PASSED" ;;
+        *) echo "$FAILED" ; retrun 1 ;;
+      esac
+    fi
+    echo
+  done < <(echo "$data" | grep -E '^## ')
 
-# Garbage collection
-rm -f $published_file
+  ##############################################################################
+  # END TESTING release::gcs::verify_latest_update()
+  ##############################################################################
+}
 
-##############################################################################
-# END TESTING release::gcs::verify_latest_update()
-##############################################################################
+test_main "$@"
diff --git a/lib/testing.sh b/lib/testing.sh
@@ -15,7 +15,7 @@
 # limitations under the License.
 
 
-testMain() {
+test_main() {
   local tests=( "$@" )
   local t
 
@@ -35,7 +35,7 @@ testMain() {
   done
 }
 
-assertEqualContent() {
+assert_equal_content() {
   local actual_file="$1"
   local expected_file="$2"
   local message="${3:-files do not match content}"