From 60b38bb91a839654317fca62001f11d788093d8d Mon Sep 17 00:00:00 2001 From: Mario Vitale Date: Fri, 20 Sep 2024 15:05:51 +0200 Subject: [PATCH] Enhance reporting, add shellcheck (#28240) * Enhance reporting, add shellcheck --------- Co-authored-by: Mattermost Build --- .github/workflows/e2e-tests-ci-template.yml | 78 +++++++++++++++++---- e2e-tests/.ci/report.collect.sh | 27 +++++++ e2e-tests/.ci/server.prepare.sh | 3 +- e2e-tests/.ci/server.run_cypress.sh | 6 +- e2e-tests/.ci/server.start.sh | 1 + e2e-tests/Makefile | 14 ++-- e2e-tests/README.md | 4 +- 7 files changed, 107 insertions(+), 26 deletions(-) create mode 100755 e2e-tests/.ci/report.collect.sh diff --git a/.github/workflows/e2e-tests-ci-template.yml b/.github/workflows/e2e-tests-ci-template.yml index 38e86781466..b7c9cfe1963 100644 --- a/.github/workflows/e2e-tests-ci-template.yml +++ b/.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 }}" diff --git a/e2e-tests/.ci/report.collect.sh b/e2e-tests/.ci/report.collect.sh new file mode 100755 index 00000000000..1a9e5198f78 --- /dev/null +++ b/e2e-tests/.ci/report.collect.sh @@ -0,0 +1,27 @@ +#!/bin/bash +# SC2034: 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 diff --git a/e2e-tests/.ci/server.prepare.sh b/e2e-tests/.ci/server.prepare.sh index 404efb43ff1..9430f5c0b81 100755 --- a/e2e-tests/.ci/server.prepare.sh +++ b/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 diff --git a/e2e-tests/.ci/server.run_cypress.sh b/e2e-tests/.ci/server.run_cypress.sh index 628e603622d..36d6deb51c3 100755 --- a/e2e-tests/.ci/server.run_cypress.sh +++ b/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 diff --git a/e2e-tests/.ci/server.start.sh b/e2e-tests/.ci/server.start.sh index 73f8d03b203..5ea94e9de58 100755 --- a/e2e-tests/.ci/server.start.sh +++ b/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');\"" diff --git a/e2e-tests/Makefile b/e2e-tests/Makefile index 547d2fa4829..5be40b38986 100644 --- a/e2e-tests/Makefile +++ b/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 diff --git a/e2e-tests/README.md b/e2e-tests/README.md index 1bdf89d204f..e00324a9b83 100644 --- a/e2e-tests/README.md +++ b/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: