[HOTE-803] feat: Add some improvements to E2E tests#355
Conversation
Lambdas Coverage Report
|
UI Coverage Report
|
There was a problem hiding this comment.
Pull request overview
Updates the Playwright E2E GitHub Actions workflow to better align local CI startup with the repo’s local environment scripts and to surface flaky tests (tests that pass on retry) in CI output.
Changes:
- Switches local startup command from
npm run starttonpm run local:startin the E2E workflow. - Adds a “Detect flaky tests” step that parses Playwright JSON results and emits GitHub warnings.
- Extends the job summary to include flaky test counts and details.
| playwright-${{ runner.os }}- | ||
|
|
||
| - name: "Install Playwright browsers" | ||
| timeout-minutes: 3 |
There was a problem hiding this comment.
The 3-minute step timeout for npx playwright install --with-deps is likely too low on cache-miss runners (apt deps + browser downloads can exceed this), causing avoidable CI failures; increase/remove the timeout or apply it only to the download portion when cache is warm.
| timeout-minutes: 3 |
| FLAKY_TESTS=$(jq -r ' | ||
| [.suites[]? | .. | .tests?[]? | select(.status == "flaky")] | | ||
| if length > 0 then | ||
| .[] | "\(.title) (\(.location.file):\(.location.line))" | ||
| else | ||
| empty | ||
| end | ||
| ' "$RESULTS_FILE" 2>/dev/null) |
There was a problem hiding this comment.
The flaky-test detection suppresses all jq parse/runtime errors (2>/dev/null), which can silently hide a broken/changed Playwright JSON schema and report “No flaky tests” incorrectly; capture jq’s exit code and emit a warning (or fail the step) when parsing fails.
| FLAKY_COUNT=$(jq '[.suites[]? | .. | .tests?[]? | select(.status == "flaky")] | length' "$RESULTS_FILE" 2>/dev/null || echo 0) | ||
| if [ "$FLAKY_COUNT" -gt 0 ]; then | ||
| echo "" >> $GITHUB_STEP_SUMMARY | ||
| echo "### :warning: Flaky Tests ($FLAKY_COUNT)" >> $GITHUB_STEP_SUMMARY | ||
| echo "These tests failed initially but passed on retry:" >> $GITHUB_STEP_SUMMARY | ||
| echo "" >> $GITHUB_STEP_SUMMARY | ||
| jq -r '[.suites[]? | .. | .tests?[]? | select(.status == "flaky")] | .[] | "- **\(.title)** (\(.location.file):\(.location.line))"' "$RESULTS_FILE" 2>/dev/null >> $GITHUB_STEP_SUMMARY |
There was a problem hiding this comment.
In the summary generation, jq ... 2>/dev/null || echo 0 treats JSON parse errors the same as “0 flaky tests”, which can make the job summary misleading; handle jq failures explicitly and surface a warning when the results file can’t be parsed.
| FLAKY_COUNT=$(jq '[.suites[]? | .. | .tests?[]? | select(.status == "flaky")] | length' "$RESULTS_FILE" 2>/dev/null || echo 0) | |
| if [ "$FLAKY_COUNT" -gt 0 ]; then | |
| echo "" >> $GITHUB_STEP_SUMMARY | |
| echo "### :warning: Flaky Tests ($FLAKY_COUNT)" >> $GITHUB_STEP_SUMMARY | |
| echo "These tests failed initially but passed on retry:" >> $GITHUB_STEP_SUMMARY | |
| echo "" >> $GITHUB_STEP_SUMMARY | |
| jq -r '[.suites[]? | .. | .tests?[]? | select(.status == "flaky")] | .[] | "- **\(.title)** (\(.location.file):\(.location.line))"' "$RESULTS_FILE" 2>/dev/null >> $GITHUB_STEP_SUMMARY | |
| if FLAKY_COUNT=$(jq '[.suites[]? | .. | .tests?[]? | select(.status == "flaky")] | length' "$RESULTS_FILE" 2>/dev/null); then | |
| if [ "$FLAKY_COUNT" -gt 0 ]; then | |
| echo "" >> $GITHUB_STEP_SUMMARY | |
| echo "### :warning: Flaky Tests ($FLAKY_COUNT)" >> $GITHUB_STEP_SUMMARY | |
| echo "These tests failed initially but passed on retry:" >> $GITHUB_STEP_SUMMARY | |
| echo "" >> $GITHUB_STEP_SUMMARY | |
| if FLAKY_DETAILS=$(jq -r '[.suites[]? | .. | .tests?[]? | select(.status == "flaky")] | .[] | "- **\(.title)** (\(.location.file):\(.location.line))"' "$RESULTS_FILE" 2>/dev/null); then | |
| printf '%s\n' "$FLAKY_DETAILS" >> $GITHUB_STEP_SUMMARY | |
| else | |
| echo ":warning: Could not parse flaky test details from $RESULTS_FILE" >> $GITHUB_STEP_SUMMARY | |
| fi | |
| fi | |
| else | |
| echo "" >> $GITHUB_STEP_SUMMARY | |
| echo ":warning: Could not parse flaky test results from $RESULTS_FILE" >> $GITHUB_STEP_SUMMARY |
| - id: grype | ||
| name: grype | ||
| entry: bash -c 'echo $PWD; whereis grype; grype --version; grype "dir:$PWD" --quiet -o template -t $PWD/scripts/config/grype-table.tmpl --name hometest-service -c $PWD/scripts/config/grype.yaml' | ||
| language: system | ||
| pass_filenames: false | ||
|
|
There was a problem hiding this comment.
grype-fs-scan and the additional grype hook both run a full filesystem Grype scan on every commit, which will double the runtime of pre-commit (and CI pre-commit) without adding coverage; consider keeping just one hook (ideally the scripted one) to avoid redundant scanning.
| - id: grype | |
| name: grype | |
| entry: bash -c 'echo $PWD; whereis grype; grype --version; grype "dir:$PWD" --quiet -o template -t $PWD/scripts/config/grype-table.tmpl --name hometest-service -c $PWD/scripts/config/grype.yaml' | |
| language: system | |
| pass_filenames: false |
|
|
||
| - id: grype | ||
| name: grype | ||
| entry: bash -c 'echo $PWD; whereis grype; grype --version; grype "dir:$PWD" --quiet -o template -t $PWD/scripts/config/grype-table.tmpl --name hometest-service -c $PWD/scripts/config/grype.yaml' |
There was a problem hiding this comment.
The grype hook entry uses unquoted $PWD paths (e.g. -t $PWD/scripts/...), which will break if the repo path contains spaces and makes the hook less portable; quote those arguments or call scripts/grype-scan.sh instead of inlining a long bash -c command.
| entry: bash -c 'echo $PWD; whereis grype; grype --version; grype "dir:$PWD" --quiet -o template -t $PWD/scripts/config/grype-table.tmpl --name hometest-service -c $PWD/scripts/config/grype.yaml' | |
| entry: bash -c 'echo "$PWD"; whereis grype; grype --version; grype "dir:$PWD" --quiet -o template -t "$PWD/scripts/config/grype-table.tmpl" --name hometest-service -c "$PWD/scripts/config/grype.yaml"' |
| - name: "Start the application" | ||
| if: env.TARGET_ENV == 'local' | ||
| run: | | ||
| npm run start | ||
| npm run local:start | ||
| env: | ||
| BUILDKIT_PROGRESS: plain # or "quiet" to fully suppress build output | ||
| DOCKER_CLI_HINTS: false # removes "What's next?" hints |
There was a problem hiding this comment.
The PR title/description focuses on E2E test improvements, but this change set also introduces dependency vulnerability scanning (Grype/OSV) and refactors local Terraform; update the PR description/title to reflect the broader scope so reviewers know what to focus on.
mikeeq
force-pushed
the
feature/hote-803/improve-lambdas-v2
branch
from
c0e04ba to
9207b17
Compare
|
| entry: bash -c 'echo $PWD; whereis grype; grype --version; grype "dir:$PWD" --quiet -o template -t $PWD/scripts/config/grype-table.tmpl --name hometest-service -c $PWD/scripts/config/grype.yaml' | ||
| language: system | ||
| pass_filenames: false |
There was a problem hiding this comment.
The grype pre-commit hook runs a long inline bash -c command with unquoted $PWD paths, which will break if the repo path contains spaces and makes the hook harder to maintain. Prefer calling the dedicated scripts/grype-scan.sh (or the existing scripts/reports/scan-vulnerabilities.sh) and ensure all paths are quoted; also drop the echo $PWD/whereis debug output unless it’s needed.
| entry: bash -c 'echo $PWD; whereis grype; grype --version; grype "dir:$PWD" --quiet -o template -t $PWD/scripts/config/grype-table.tmpl --name hometest-service -c $PWD/scripts/config/grype.yaml' | |
| language: system | |
| pass_filenames: false | |
| entry: scripts/grype-scan.sh | |
| language: script | |
| pass_filenames: false | |
| always_run: true | |
| require_serial: true |
| @@ -6,10 +6,6 @@ install_before = "7d" | |||
| [settings.python] | |||
| compile = false | |||
|
|
|||
There was a problem hiding this comment.
Removing the [settings.aqua] cosign = false stanza changes how mise installs aqua-backed tools; in this repo grype/osv-scanner are still resolved via the aqua backend (see mise.lock), so this may reintroduce GitHub/cosign verification calls and break installs in CI or restricted networks. Consider restoring the setting or documenting/validating the new expected behaviour.
| [settings.aqua] | |
| cosign = false |
| - name: "Install Playwright browsers" | ||
| timeout-minutes: 3 | ||
| working-directory: tests | ||
| run: npx playwright install --with-deps |
There was a problem hiding this comment.
timeout-minutes: 3 for npx playwright install --with-deps is likely to cause intermittent CI failures on cache misses or slower runners because browser downloads can exceed 3 minutes. Consider increasing this timeout or making it conditional on the cache hit status.
Description
Context
Type of changes
Checklist
Sensitive Information Declaration
To ensure the utmost confidentiality and protect your and others privacy, we kindly ask you to NOT including PII (Personal Identifiable Information) / PID (Personal Identifiable Data) or any other sensitive data in this PR (Pull Request) and the codebase changes. We will remove any PR that do contain any sensitive information. We really appreciate your cooperation in this matter.