Simplify generating and gathering test coverage metrics

.github/workflows/main.yml

@@ -281,7 +281,7 @@ jobs:
           kosli attest generic \
               --description="unit-test branch-coverage and metrics" \
               --name=differ.unit-test-coverage \
-              --user-data=./reports/server/coverage.json
+              --user-data=./reports/server/coverage_metrics.json
 
 
   integration-tests:
@@ -304,7 +304,7 @@ jobs:
 
       - name: Run integration tests
         run:
-          make test_client
+          make image_client test_client
 
       - name: Get integration test coverage
         id: coverage
@@ -335,7 +335,7 @@ jobs:
           kosli attest generic \
               --description="integration-test branch-coverage and metrics" \
               --name=differ.integration-test-coverage \
-              --user-data=./reports/client/coverage.json
+              --user-data=./reports/client/coverage_metrics.json
 
 
   snyk-container-scan:

.rubocop.yml

@@ -1,35 +1,46 @@
 AllCops:
     NewCops: enable
 
-Metrics/BlockLength:
+Layout/EmptyLineAfterMagicComment:
   Exclude:
-    - 'test/**/*.rb'
-    - source/server/app_base.rb
+    - test/*/lib/id58_test_base.rb
 
-Metrics/MethodLength:
+Layout/ExtraSpacing:
   Exclude:
-    - 'test/**/*.rb'
-    - source/client/html_demo.rb
-    - source/server/git_diff_parser.rb
-    - source/server/external/sheller.rb
-    - source/server/differ.rb
-    - source/client/http_json_hash/service_error.rb
+    - test/*/lib/check_test_metrics.rb
 
-Style/Documentation:
+Layout/LineLength:
   Exclude:
-    - '*/**/*.rb'
+    - test/*/lib/simplecov_formatter_json.rb
 
-Metrics/CyclomaticComplexity:
+Layout/SpaceInsideArrayLiteralBrackets:
+  Exclude:
+    - test/*/lib/check_test_metrics.rb
+
+Layout/SpaceInsideReferenceBrackets:
+  Exclude:
+    - test/*/lib/check_test_metrics.rb
+
+Lint/RescueException:
   Exclude:
+    - source/client/client.rb
+
+Lint/SymbolConversion:
+  Exclude:
+    - source/server/app_base.rb
     - test/*/lib/id58_test_base.rb
-    - source/server/git_diff_parser.rb
 
 Metrics/AbcSize:
   Exclude:
-    - test/*/lib/simplecov_json.rb
     - test/*/lib/id58_test_base.rb
     - source/server/git_diff_parser.rb
+    - test/*/lib/simplecov_formatter_json.rb
+    - test/*/lib/check_test_metrics.rb
 
+Metrics/BlockLength:
+  Exclude:
+    - test/**/*.rb
+    - source/server/app_base.rb
 
 Metrics/ClassLength:
   Exclude:
@@ -40,41 +51,61 @@ Metrics/ClassLength:
     - test/server/diff_summary_test.rb
     - source/server/git_diff_parser.rb
 
-Style/FormatStringToken:
+Metrics/CyclomaticComplexity:
   Exclude:
     - test/*/lib/id58_test_base.rb
+    - source/server/git_diff_parser.rb
 
-Layout/LineLength:
+Metrics/MethodLength:
+  Exclude:
+    - test/**/*.rb
+    - source/client/html_demo.rb
+    - source/server/git_diff_parser.rb
+    - source/server/external/sheller.rb
+    - source/server/differ.rb
+    - source/client/http_json_hash/service_error.rb
+
+Metrics/ParameterLists:
+  Exclude:
+    - test/server/saver.rb
+    - source/client/saver.rb
+
+Naming/VariableNumber:
   Exclude:
-    - test/*/lib/simplecov_json.rb
+    - source/server/external/gitter.rb
+
+Security/Eval:
+  Exclude:
+    - test/*/lib/check_test_metrics.rb
 
 Style/ClassVars:
   Exclude:
     - test/*/lib/id58_test_base.rb
 
-Naming/VariableNumber:
+Style/ClassAndModuleChildren:
   Exclude:
-    - source/server/external/gitter.rb
+    - test/*/lib/slim_json_reporter.rb
 
-Metrics/ParameterLists:
+Style/Documentation:
   Exclude:
-    - test/server/saver.rb
-    - source/client/saver.rb
+    - '*/**/*.rb'
 
-Style/OpenStructUse:
+Style/DocumentDynamicEvalDefinition:
   Exclude:
-    - test/server/prober_test.rb
-    - test/server/http_adapter_stub.rb
-    - test/client/http_response_unpacker_test.rb
+    - test/*/lib/check_test_metrics.rb
 
-Lint/RescueException:
+Style/EvalWithLocation:
   Exclude:
-    - source/client/client.rb
+    - test/*/lib/check_test_metrics.rb
 
-Lint/SymbolConversion:
+Style/FormatString:
+  Exclude:
+    - test/*/lib/check_test_metrics.rb
+
+Style/FormatStringToken:
   Exclude:
-    - source/server/app_base.rb
     - test/*/lib/id58_test_base.rb
+    - test/*/lib/check_test_metrics.rb
 
 Style/FetchEnvVar:
   Exclude:
@@ -88,5 +119,13 @@ Style/HashTransformKeys:
   Exclude:
     - source/server/app_base.rb
 
+Style/OpenStructUse:
+  Exclude:
+    - test/server/prober_test.rb
+    - test/server/http_adapter_stub.rb
+    - test/client/http_response_unpacker_test.rb
 
+Style/TrailingCommaInArrayLiteral:
+  Exclude:
+    - test/*/lib/check_test_metrics.rb
 

Dockerfile

@@ -1,4 +1,4 @@
-FROM cyberdojo/sinatra-base:11ddc45
+FROM cyberdojo/sinatra-base:026c095
 
 LABEL [email protected]
 

Makefile

@@ -18,7 +18,7 @@ all_client: test_client coverage_client
 image_client:
 	${PWD}/bin/build_image.sh client
 
-test_client: image_client
+test_client: 
 	${PWD}/bin/run_tests.sh client
 
 coverage_client:

bin/check_coverage.sh

@@ -40,28 +40,34 @@ check_args()
 
 check_coverage()
 {
-  # Process test-run results and coverage data against metrics.rb values and
-  #   - print output showing individual metrics and their pass/fail status
-  #   - return zero if all metrics pass, otherwise non-zero
-  # Does not create any new files.
-
   check_args "$@"
+  export $(echo_versioner_env_vars)
+
+  local -r TYPE="${1}"           # {server|client}
+  local -r TEST_LOG=test.log
+  local -r HOST_TEST_DIR="${ROOT_DIR}/test/${TYPE}"
+  local -r HOST_REPORTS_DIR="${ROOT_DIR}/reports/${TYPE}"  # where report json files have been written to
+  local -r CONTAINER_TMP_DIR=/tmp
 
-  local -r TYPE="${1}"  # client | server
+  exit_non_zero_unless_file_exists "${HOST_REPORTS_DIR}/${TEST_LOG}"
+  exit_non_zero_unless_file_exists "${HOST_REPORTS_DIR}/test_metrics.json"
+  exit_non_zero_unless_file_exists "${HOST_REPORTS_DIR}/coverage_metrics.json"
+  exit_non_zero_unless_file_exists "${HOST_TEST_DIR}/lib/check_test_metrics.rb"
 
   set +e
   docker run \
+    --read-only \
+    --rm \
+    --entrypoint="" \
+    --env COVERAGE_ROOT="${CONTAINER_TMP_DIR}" \
     --env COVERAGE_CODE_TAB_NAME=app \
     --env COVERAGE_TEST_TAB_NAME=test \
-    --rm \
-    --volume "${ROOT_DIR}/reports/${TYPE}":/reports/:ro \
-    --volume "${ROOT_DIR}/test/${TYPE}"/metrics.rb:/app/metrics.rb:ro \
-    cyberdojo/check-test-results:latest \
-      sh -c \
-        "ruby /app/check_test_results.rb \
-          /reports/test.log \
-          /reports/index.html \
-          /reports/coverage.json"
+    --volume ${HOST_REPORTS_DIR}/test_metrics.json:${CONTAINER_TMP_DIR}/test_metrics.json:ro \
+    --volume ${HOST_REPORTS_DIR}/coverage_metrics.json:${CONTAINER_TMP_DIR}/coverage_metrics.json:ro \
+    --volume ${HOST_TEST_DIR}/lib/check_test_metrics.rb:${CONTAINER_TMP_DIR}/check_test_metrics.rb:ro \
+      "${CYBER_DOJO_DIFFER_IMAGE}:${CYBER_DOJO_DIFFER_TAG}" \
+        sh -c "ruby ${CONTAINER_TMP_DIR}/check_test_metrics.rb" \
+        | tee -a "${HOST_REPORTS_DIR}/${TEST_LOG}"
 
   local -r STATUS=${PIPESTATUS[0]}
   set -e

bin/lib.sh

@@ -25,15 +25,22 @@ stderr()
   >&2 echo "ERROR: ${message}"
 }
 
+exit_non_zero_unless_file_exists()
+{
+  local -r filename="${1}"
+  if [ ! -f "${filename}" ]; then
+    stderr "${filename} does not exist"
+    exit 42
+  fi
+}
+
 copy_in_saver_test_data()
 {
-  local -r SRC_PATH=${ROOT_DIR}/test/server/data/cyber-dojo
   local -r SAVER_CID=$(docker ps --filter status=running --format '{{.Names}}' | grep "saver")
+  local -r SRC_PATH=${ROOT_DIR}/test/server/data/cyber-dojo
   local -r DEST_PATH=/cyber-dojo
   # You cannot docker cp to a tmpfs, so tar-piping instead...
-  pushd "${SRC_PATH}" || exit 99
-  tar -c . | docker exec -i "${SAVER_CID}" tar x -C ${DEST_PATH}
-  popd || exit 99
+  tar --no-xattrs -c -C "${SRC_PATH}" - . | docker exec -i "${SAVER_CID}" tar x -C ${DEST_PATH}
 }
 
 containers_down()

bin/run_tests.sh

@@ -72,15 +72,14 @@ run_tests()
   copy_in_saver_test_data
 
   local -r TYPE="${1}"           # {server|client}
+  local -r TEST_LOG=test.log
+  local -r CONTAINER_COVERAGE_DIR="/tmp/reports"
+  local -r HOST_REPORTS_DIR="${ROOT_DIR}/reports/${TYPE}"
 
   echo '=================================='
   echo "Running ${TYPE} tests"
   echo '=================================='
 
-  local -r COVERAGE_CODE_TAB_NAME=app
-  local -r COVERAGE_TEST_TAB_NAME=test
-  local -r TEST_LOG=test.log
-
   # CONTAINER_NAME is running with read-only:true and a non-root user (in docker-compose.yml). I want to
   # keep those settings in the docker-exec call below since that is how I want the microservice to run.
   # The [docker exec run.sh] is creating coverage files which I process on the host after it completes.
@@ -89,27 +88,27 @@ run_tests()
   # on both my M2 laptop, and in the CI workflow. So I am writing the coverage files to /tmp and
   # tar-piping them out.
 
-  local -r CONTAINER_COVERAGE_DIR="/tmp/reports"
-
   set +e
   docker exec \
-    --env COVERAGE_CODE_TAB_NAME=${COVERAGE_CODE_TAB_NAME} \
-    --env COVERAGE_TEST_TAB_NAME=${COVERAGE_TEST_TAB_NAME} \
+    --env COVERAGE_CODE_TAB_NAME=app \
+    --env COVERAGE_TEST_TAB_NAME=test \
     --user "${USER}" \
     "${CONTAINER_NAME}" \
       sh -c "/differ/test/lib/run.sh ${CONTAINER_COVERAGE_DIR} ${TEST_LOG} ${*:2}"
   local -r STATUS=$?
   set -e
 
-  local -r HOST_REPORTS_DIR="${ROOT_DIR}/reports/${TYPE}"
-
   rm -rf "${HOST_REPORTS_DIR}" &> /dev/null || true
   mkdir -p "${HOST_REPORTS_DIR}" &> /dev/null || true
 
-  docker exec \
-    "${CONTAINER_NAME}" \
-    tar Ccf "${CONTAINER_COVERAGE_DIR}" - . \
-        | tar Cxf "${HOST_REPORTS_DIR}" -
+  docker exec --user "${USER}" "${CONTAINER_NAME}" tar Ccf "${CONTAINER_COVERAGE_DIR}" - . \
+    | tar Cxf "${HOST_REPORTS_DIR}" -
+
+  # Check we generated the expected files.
+  exit_non_zero_unless_file_exists "${HOST_REPORTS_DIR}/${TEST_LOG}"
+  exit_non_zero_unless_file_exists "${HOST_REPORTS_DIR}/index.html"
+  exit_non_zero_unless_file_exists "${HOST_REPORTS_DIR}/test_metrics.json"
+  exit_non_zero_unless_file_exists "${HOST_REPORTS_DIR}/coverage_metrics.json"
 
   echo "${TYPE} test branch-coverage report is at:"
   echo "${HOST_REPORTS_DIR}/index.html"

source/client/Dockerfile

@@ -1,4 +1,4 @@
-FROM cyberdojo/sinatra-base:11ddc45
+FROM cyberdojo/sinatra-base:026c095
 LABEL [email protected]
 
 WORKDIR /differ

test/client/lib/check_test_metrics.rb

@@ -0,0 +1,62 @@
+# frozen_string_literal: true
+
+# Uses data from two json files:
+# - reports/client/test_metrics.json     generated in slim_json_reporter.rb by minitest. See id58_test_base.rb
+# - reports/client/coverage_metrics.json generated in simplecov_formatter_json.rb by simplecov. See coverage.rb
+
+require 'json'
+
+def coloured(arg)
+  red = 31
+  green = 32
+  colourize(arg ? green : red, arg)
+end
+
+def colourize(code, word)
+  "\e[#{code}m #{word} \e[0m"
+end
+
+def table_data
+  cov_root = ENV.fetch('COVERAGE_ROOT')
+  stats = JSON.parse(File.read("#{cov_root}/test_metrics.json"))
+
+  cov_json = JSON.parse(File.read("#{cov_root}/coverage_metrics.json"))
+  test_cov = cov_json['groups'][ENV.fetch('COVERAGE_TEST_TAB_NAME')]
+  code_cov = cov_json['groups'][ENV.fetch('COVERAGE_CODE_TAB_NAME')]
+
+  [
+    [ nil ],
+    [ 'test.count',    stats['test_count'],    '>=',  34 ],
+    [ 'test.duration', stats['total_time'],    '<=',  50 ],
+    [ nil ],
+    [ 'test.failures', stats['failure_count'], '<=',  0 ],
+    [ 'test.errors',   stats['error_count'  ], '<=',  0 ],
+    [ 'test.skips',    stats['skip_count'   ], '<=',  0 ],
+    [ nil ],
+    [ 'test.lines.total',      test_cov['lines'   ]['total' ], '<=', 234 ],
+    [ 'test.lines.missed',     test_cov['lines'   ]['missed'], '<=', 0   ],
+    [ 'test.branches.total',   test_cov['branches']['total' ], '<=', 0   ],
+    [ 'test.branches.missed',  test_cov['branches']['missed'], '<=', 0   ],
+    [ nil ],
+    [ 'code.lines.total',      code_cov['lines'   ]['total' ], '<=', 101 ],
+    [ 'code.lines.missed',     code_cov['lines'   ]['missed'], '<=', 0   ],
+    [ 'code.branches.total',   code_cov['branches']['total' ], '<=', 8  ],
+    [ 'code.branches.missed',  code_cov['branches']['missed'], '<=', 0   ],
+  ]
+end
+
+results = []
+table_data.each do |name, value, op, limit|
+  if name.nil?
+    puts
+    next
+  end
+  # puts "name=#{name}, value=#{value}, op=#{op}, limit=#{limit}"  # debug
+  result = eval("#{value} #{op} #{limit}")
+  puts '%s | %s %s %s | %s' % [
+    name.rjust(25), value.to_s.rjust(5), "  #{op}", limit.to_s.rjust(5), coloured(result)
+  ]
+  results << result
+end
+puts
+exit results.all?