Enhance reporting, add shellcheck (#28240)

* Enhance reporting, add shellcheck

---------

Co-authored-by: Mattermost Build <build@mattermost.com>

.github/workflows/e2e-tests-ci-template.yml

@@ -163,6 +163,24 @@ jobs:
         run: |
           npm run check
 
+  shell-check:
+    runs-on: ubuntu-latest
+    needs:
+      - update-initial-status
+    defaults:
+      run:
+        working-directory: e2e-tests
+    steps:
+      - name: ci/checkout-repo
+        if: "${{ inputs.run_preflight_checks }}"
+        uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1
+        with:
+          ref: ${{ inputs.commit_sha }}
+          fetch-depth: 0
+      - name: ci/shell-check
+        if: "${{ inputs.run_preflight_checks }}"
+        run: make check-shell
+
   generate-build-variables:
     runs-on: ubuntu-latest
     needs:
@@ -248,6 +266,7 @@ jobs:
     needs:
       - cypress-check
       - playwright-check
+      - shell-check
       - generate-build-variables
       - generate-test-cycle
     defaults:
@@ -338,7 +357,10 @@ jobs:
         shell: bash
         working-directory: e2e-tests
     outputs:
-      failures: "${{ steps.calculate-failures.outputs.failures }}"
+      successes: "${{ steps.calculate-results.outputs.successes }}"
+      failures: "${{ steps.calculate-results.outputs.failures }}"
+      failures_expected: "${{ steps.calculate-results.outputs.failures_expected }}"
+      total_specs: "${{ steps.calculate-results.outputs.total_specs }}"
     steps:
       - name: ci/checkout-repo
         uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1
@@ -357,17 +379,6 @@ jobs:
           path: |
             e2e-tests/cypress/logs/
             e2e-tests/cypress/results/
-      - name: ci/report-calculate-failures
-        id: calculate-failures
-        run: |
-          FAILURES=$(find cypress/results -name '*.json' | xargs -l jq -r '.stats.failures' | jq -s add)
-          echo "failures=${FAILURES}" >> $GITHUB_OUTPUT
-          echo "Cypress run completed with $FAILURES failures"
-      - name: ci/e2e-test-assert-results
-        if: "${{ inputs.testcase_failure_fatal }}"
-        run: |
-          # Assert that the run contained 0 failures
-          [ "${{ steps.calculate-failures.outputs.failures }}" = "0" ]
       - name: ci/setup-node
         if: "${{ inputs.enable_reporting }}"
         uses: actions/setup-node@60edb5dd545a775178f52524783378180af0d1f8 # v4.0.2
@@ -390,7 +401,44 @@ jobs:
           TM4J_API_KEY: "${{ secrets.REPORT_TM4J_API_KEY }}"
           TEST_CYCLE_LINK_PREFIX: "${{ secrets.REPORT_TM4J_TEST_CYCLE_LINK_PREFIX }}"
         run: |
-          make publish-report
+          make report
+      # The results dir may have been modified as part of the reporting: re-upload
+      - name: ci/upload-report-global
+        if: "${{ inputs.enable_reporting }}"
+        uses: actions/upload-artifact@5d5d22a31266ced268874388b861e4b58bb5c2f3 # v4.3.1
+        with:
+          name: e2e-test-results
+          path: |
+            e2e-tests/cypress/logs/
+            e2e-tests/cypress/results/
+          overwrite: true
+      - name: ci/report-calculate-results
+        id: calculate-results
+        env:
+          AD_CYCLE_FILE: "cypress/results/ad_cycle.json"
+        run: |
+          if [ -f "$AD_CYCLE_FILE" ]; then
+            # Prefer using the Automation Dashboard's results to calculate failures
+            SUCCESSES=$(jq -r .pass "$AD_CYCLE_FILE")
+            FAILURES=$(jq -r .fail "$AD_CYCLE_FILE")
+            FAILURES_EXPECTED=$(jq -r ".known + .flaky + .skipped" "$AD_CYCLE_FILE")
+          else
+            # Otherwise, utilize the test specs to calculate the failures
+            SUCCESSES=$(find cypress/results/mochawesome-report/json/tests/ -name '*.json' | xargs -l jq -r '.stats.passes' | jq -s add)
+            FAILURES=$(find cypress/results/mochawesome-report/json/tests/ -name '*.json' | xargs -l jq -r '.stats.failures' | jq -s add)
+            FAILURES_EXPECTED="0"
+          fi
+          TOTAL_SPECS=$(( FAILURES + SUCCESSES ))
+          echo "successes=${SUCCESSES:?}" >> $GITHUB_OUTPUT
+          echo "failures=${FAILURES:?}" >> $GITHUB_OUTPUT
+          echo "failures_expected=${FAILURES_EXPECTED:?}" >> $GITHUB_OUTPUT
+          echo "total_specs=${TOTAL_SPECS:?}" >> $GITHUB_OUTPUT
+          echo "Cypress run completed:  ${SUCCESSES}/${TOTAL_SPECS} passing specs (plus ${FAILURES_EXPECTED} unrelated failures)"
+      - name: ci/e2e-test-assert-results
+        if: "${{ inputs.testcase_failure_fatal }}"
+        run: |
+          # Assert that the run contained 0 failures
+          [ "${{ steps.calculate-results.outputs.failures }}" = "0" ]
 
   update-failure-final-status:
     runs-on: ubuntu-latest
@@ -408,7 +456,7 @@ jobs:
           commit_sha: ${{ inputs.commit_sha }}
           context: ${{ inputs.status_check_context }}
           description: |
-            Completed with ${{ needs.report.outputs.failures }} failures
+            Completed: ${{ needs.report.outputs.successes }}/${{ needs.report.outputs.total_specs }} passing specs (plus ${{ needs.report.outputs.failures_expected }} unrelated failures)
           status: failure
           target_url: "${{ needs.generate-test-cycle.outputs.status_check_url }}"
 
@@ -428,6 +476,6 @@ jobs:
           commit_sha: ${{ inputs.commit_sha }}
           context: ${{ inputs.status_check_context }}
           description: |
-            Completed with ${{ needs.report.outputs.failures }} failures
+            Completed: ${{ needs.report.outputs.successes }}/${{ needs.report.outputs.total_specs }} passing specs (plus ${{ needs.report.outputs.failures_expected }} unrelated failures)
           status: success
           target_url: "${{ needs.generate-test-cycle.outputs.status_check_url }}"

e2e-tests/.ci/report.collect.sh

@@ -0,0 +1,27 @@
+#!/bin/bash
+# SC2034: <variable> appears unused.
+# https://www.shellcheck.net/wiki/SC2034
+# shellcheck disable=SC2034
+# shellcheck disable=SC2086,SC2223
+
+set -e -u -o pipefail
+cd "$(dirname "$0")"
+. .e2erc
+
+# The collected data will be written to the "e2e-tests/cypress/results/" directory
+cd ../cypress/
+if [ ! -d "results/" ]; then
+  mme2e_log "Error: 'results/' directory does not exist. Aborting report data collection." >&2
+  exit 1
+fi
+
+# If the Automation Dashboard is used, try to collect run data from it
+if [ -n "${AUTOMATION_DASHBOARD_URL:-}" ]; then
+  mme2e_log "Automation Dashboard usage detected: retrieving cycle results"
+  # Assume that we're in the cypress dir and that the 'results/' directory exists
+  : ${BUILD_ID:?}
+  AD_CYCLE_FILE="results/ad_cycle.json"
+  AD_SPECS_FILE="results/ad_specs.json"
+  curl -o "$AD_CYCLE_FILE" -fsSL "${AUTOMATION_DASHBOARD_URL}/cycle?build=${BUILD_ID}"
+  curl -o "$AD_SPECS_FILE" -fsSL "${AUTOMATION_DASHBOARD_URL}/executions/specs?cycle_id=$(jq -r .id "$AD_CYCLE_FILE")"
+fi

e2e-tests/.ci/server.prepare.sh

@@ -11,7 +11,8 @@ for SETTING in \
     PluginSettings.AutomaticPrepackagedPlugins=true
   do
   mme2e_log "Configuring parameter: $SETTING"
-  ${MME2E_DC_SERVER} exec -T -- server mmctl --local config set $(tr = ' ' <<<$SETTING)
+  # shellcheck disable=SC2046
+  ${MME2E_DC_SERVER} exec -T -- server mmctl --local config set $(tr '=' ' ' <<<$SETTING)
 done
 if [ -n "${MM_LICENSE:-}" ]; then
   # We prefer uploading the license here, instead of setting the env var for the server

e2e-tests/.ci/server.run_cypress.sh

@@ -33,12 +33,12 @@ LOGFILE_SUFFIX="${CI_BASE_URL//\//_}" # Remove slashes from CI_BASE_URL to produ
 # shellcheck disable=SC2016
 if ${MME2E_DC_SERVER} exec -T -u "$MME2E_UID" -- cypress bash -c '[ -n "${AUTOMATION_DASHBOARD_URL}" ]'; then
   mme2e_log "AUTOMATION_DASHBOARD_URL is set. Using run_test_cycle.js for the cypress run"
-  ${MME2E_DC_SERVER} exec -T -u "$MME2E_UID" -- cypress node run_test_cycle.js | tee ../cypress/logs/${LOGFILE_SUFFIX}_cypress.log
+  ${MME2E_DC_SERVER} exec -T -u "$MME2E_UID" -- cypress node run_test_cycle.js | tee "../cypress/logs/${LOGFILE_SUFFIX}_cypress.log"
 else
   mme2e_log "AUTOMATION_DASHBOARD_URL is unset. Using run_tests.js for the cypress run"
   # shellcheck disable=SC2086
-  ${MME2E_DC_SERVER} exec -T -u "$MME2E_UID" -- cypress node run_tests.js $TEST_FILTER | tee ../cypress/logs/${LOGFILE_SUFFIX}_cypress.log
+  ${MME2E_DC_SERVER} exec -T -u "$MME2E_UID" -- cypress node run_tests.js $TEST_FILTER | tee "../cypress/logs/${LOGFILE_SUFFIX}_cypress.log"
 fi
 
 # Collect server logs
-${MME2E_DC_SERVER} logs --no-log-prefix -- server >../cypress/logs/${LOGFILE_SUFFIX}_mattermost.log 2>&1
+${MME2E_DC_SERVER} logs --no-log-prefix -- server > "../cypress/logs/${LOGFILE_SUFFIX}_mattermost.log" 2>&1

e2e-tests/.ci/server.start.sh

@@ -15,6 +15,7 @@ if ! mme2e_wait_service_healthy server 60 10; then
   mme2e_log "Mattermost container not healthy, retry attempts exhausted. Giving up." >&2
   exit 1
 fi
+# shellcheck disable=SC2043
 for MIGRATION in migration_advanced_permissions_phase_2; do
   # Query explanation: if it doesn't find the migration in the table, there are 0 results and the command fails with a divide-by-zero error. Otherwise the command succeeds
   MIGRATION_CHECK_COMMAND="${MME2E_DC_SERVER} exec -T -- postgres psql -U mmuser mattermost_test -c \"select 1 / (select count(*) from Systems where name = '${MIGRATION}' and value = 'true');\""

e2e-tests/Makefile

@@ -4,6 +4,7 @@ SHELL := /bin/bash
 all: run
 run: generate-server start-server run-test
 stop: stop-server stop-dashboard clean
+report: collect-report-data publish-report
 clean:
 	rm -fv .ci/server.yml
 	rm -fv .ci/.env.{server,dashboard,cypress,playwright}
@@ -34,17 +35,20 @@ cloud-init: requirecmd-jq requirecmd-curl
 cloud-teardown:
 	bash ./.ci/server.cloud_teardown.sh
 
-.PHONY: publish-report
+.PHONY: collect-report-data publish-report
+collect-report-data:
+	bash ./.ci/report.collect.sh
 publish-report: requirecmd-node
 	bash ./.ci/report.publish.sh
 
-.PHONY: fmt-node fmt-shell fmt
+.PHONY: check-shell fmt-shell fmt-node fmt
 requirecmd-%:
 	@which "$(*)" >/dev/null || { echo "Error, missing required CLI tool: $(*). Aborting." >&2; exit 1; }
+check-shell: requirecmd-shellcheck
+	shellcheck ./.ci/*.sh ./.ci/.e2erc* # Install with https://webinstall.dev/shfmt/
+fmt-shell: requirecmd-shfmt
+	shfmt -w -s -i 2 ./.ci/*.sh         # Install with https://webinstall.dev/shellcheck/
 fmt-node: requirecmd-npx
 	# Formats yaml files
 	npx prettier ./.ci "!./.ci/dashboard" --write --cache
-fmt-shell: requirecmd-shfmt requirecmd-shellcheck
-	shfmt -w -s -i 2 ./.ci/*.sh         # Install with https://webinstall.dev/shellcheck/
-	shellcheck ./.ci/*.sh ./.ci/.e2erc* # Install with https://webinstall.dev/shfmt/
 fmt: fmt-node fmt-shell

e2e-tests/README.md

@@ -27,7 +27,7 @@ Instructions, detailed:
   * The following variables, which will be passed over to the cypress container: `BRANCH`, `BUILD_ID`, `CI_BASE_URL`, `BROWSER`, `AUTOMATION_DASHBOARD_URL` and `AUTOMATION_DASHBOARD_TOKEN`
   * The `SERVER_IMAGE` variable can also be set if you want to select a custom mattermost-server image. If not specified, the value of the `SERVER_IMAGE_DEFAULT` variable defined in file `.ci/.e2erc` is used.
   * The `TEST_FILTER` variable can also be set, to customize which tests you want Cypress to run. If not specified, only the smoke tests will run. Please check the `e2e-tests/cypress/run_tests.js` file for details about its format.
-  * More variables may be required to configure reporting and cloud interactions. Check the content of the `.ci/report.publish.sh` and `.ci/server.cloud_*.sh` scripts for reference.
+  * More variables may be required to configure reporting and cloud interactions. Check the content of the `.ci/report.*.sh` and `.ci/server.cloud_*.sh` scripts for reference.
 2. (optional) `make start-dashboard && make generate-test-cycle`: start the automation dashboard in the background, and initiate a test cycle on it, for the given `BUILD_ID`
   * NB: the `BUILD_ID` value should stay the same across the `make generate-test-cycle` command, and the subsequent `make` (see next step). If you need to initiate a new test cycle on the same dashboard, you'll need to change the `BUILD_ID` value and rerun both `make generate-test-cycle` and `make`.
   * Note that part of the dashboard functionality assumes the `BUILD_ID` to have a certain format (see [here](https://github.com/saturninoabril/automation-dashboard/blob/175891781bf1072c162c58c6ec0abfc5bcb3520e/lib/common_utils.ts#L3-L23) for details). This is not relevant for local running, but it's important to note in the testing pipelines.
@@ -54,7 +54,7 @@ Notes:
   * If their value is fixed (e.g. a static server configuration), these may be simply added to the `docker_compose_generator.sh` file, to the appropriate container.
   * If you need to introduce variables that you want to control from `.ci/env`: you need to update the scripts under the `.ci/` dir and configure them to write the new variables' values over to the appropriate `.env.*` file. In particular, avoid defining variables that depend on other variables within the docker-compose override files: this is to ensure uniformity in their availability and simplifies the question of what container has access to which variable considerably.
   * Exceptions are of course accepted wherever it makes sense (e.g. if you need to group variables based on some common functionality)
-- The `publish-report` Make target is meant for internal usage. Usage and variables are documented in the respective scripts.
+- The `report` Make target is meant for internal usage. Usage and variables are documented in the respective scripts.
 - `make start-server` won't cleanup containers that don't change across runs. This means that you can use it to emulate a Mattermost server upgrade while retaining your database data by simply changing the `SERVER_IMAGE` variable on your machine, and then re-running `make start-server`. But this also means that if you want to run a clean local environment, you may have to manually run `make stop` to cleanup any running containers and their volumes, which include e.g. the database.
 
 ##### For code changes: