Development: Add different execution modes for local playwright setup
#10064
Conversation
WalkthroughThis pull request focuses on enhancing the Playwright testing setup and documentation. Key changes include updating the README.md file to provide clearer instructions for running Playwright tests, modifying the Changes
Possibly related PRs
Suggested Labels
Suggested Reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
supporting_scripts/playwright/startPlaywright.sh (1)
15-16: Consider adding error handling for the test command.While the script uses
set -efor general error handling, consider adding specific error handling for the test command to provide better feedback on test failures.echo "Run all playwright tests" -npm run playwright:test +if ! npm run playwright:test; then + echo "Playwright tests failed. Check the test output above for details." + exit 1 +fi
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
supporting_scripts/playwright/README.md(1 hunks)supporting_scripts/playwright/startPlaywright.sh(1 hunks)supporting_scripts/playwright/startPlaywrightUI.sh(1 hunks)
🔇 Additional comments (2)
supporting_scripts/playwright/README.md (2)
19-23: LGTM! Improved code block formatting.
The addition of proper code block formatting for the setupUsers.sh command improves readability and follows Markdown best practices.
26-41: LGTM! Clear and well-structured documentation.
The documentation improvements effectively:
- Separate and explain different execution modes
- Provide clear distinctions between UI mode and headless testing
- Include helpful details about features available in each mode
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
supporting_scripts/playwright/runArtemisInDocker_linux.sh (1)
Line range hint
8-10: Add safety checks for Docker setupThe script modifies the Docker compose file without validation or backup.
Consider these improvements:
+# Check if Docker is running +if ! docker info >/dev/null 2>&1; then + echo "Error: Docker is not running" + exit 1 +fi + +# Get Docker group ID with validation +DOCKER_GID=$(getent group docker | cut -d: -f3) +if [ -z "$DOCKER_GID" ]; then + echo "Error: Could not determine Docker group ID" + exit 1 +fi + +# Create backup before modification +cp artemis-dev-local-vc-local-ci-mysql.yml artemis-dev-local-vc-local-ci-mysql.yml.bak + echo "Updating docker group ID in the docker compose file" -sed -i "s/999/$(getent group docker | cut -d: -f3)/g" artemis-dev-local-vc-local-ci-mysql.yml +sed -i "s/999/$DOCKER_GID/g" artemis-dev-local-vc-local-ci-mysql.ymlsupporting_scripts/playwright/runArtemisInDocker_macOS.sh (1)
Line range hint
8-9: Add Docker.app readiness checkThe script opens Docker.app but doesn't wait for it to be ready before proceeding.
Consider adding a wait loop:
cd "$artemis_path/docker" open -a Docker + +# Wait for Docker to be ready +echo "Waiting for Docker to start..." +for i in $(seq 1 30); do + if docker info >/dev/null 2>&1; then + echo "Docker is ready" + break + fi + if [ $i -eq 30 ]; then + echo "Error: Docker failed to start within 30 seconds" + exit 1 + fi + sleep 1 +done
🧹 Nitpick comments (2)
supporting_scripts/playwright/setupUsers.sh (1)
6-6: Consider improving path resolution portabilityWhile quoting
"$0"is a good improvement, the current path resolution approach has some portability concerns:
readlink -fis not available on macOS by default- No fallback if readlink fails
Consider this more portable alternative:
-artemis_path="$(readlink -f "$(dirname "$0")/../..")" +get_script_path() { + if command -v readlink >/dev/null && readlink -f "." >/dev/null 2>&1; then + readlink -f "$1" + else + # Fallback for systems without readlink -f (e.g., macOS) + local path="$1" + while [ -L "$path" ]; do + path="$(cd "$(dirname "$path")" && pwd -P)/$(basename "$path")" + done + echo "$(cd "$(dirname "$path")" && pwd -P)/$(basename "$path")" + fi +} +artemis_path="$(dirname "$(get_script_path "$0")")/../.."supporting_scripts/playwright/runArtemisInDocker_macOS.sh (1)
Line range hint
10-16: Improve Docker group ID handlingThe current implementation could be more robust in handling the Docker group ID lookup and file modification.
Consider these improvements:
+# Create backup before modification +cp artemis-dev-local-vc-local-ci-mysql.yml artemis-dev-local-vc-local-ci-mysql.yml.bak + echo "Updating docker group ID in the docker compose file" -PRIMARY_GROUP_ID=$(dscl . -read /Groups/docker PrimaryGroupID | awk '{print $2}') -if [ -n "$PRIMARY_GROUP_ID" ]; then - sed -i '' "s/999/$PRIMARY_GROUP_ID/g" artemis-dev-local-vc-local-ci-mysql.yml -else - echo "PrimaryGroupID not found, skipping replacement" -fi +get_docker_gid() { + local gid + if gid=$(dscl . -read /Groups/docker PrimaryGroupID 2>/dev/null | awk '{print $2}'); then + echo "$gid" + return 0 + fi + return 1 +} + +if ! PRIMARY_GROUP_ID=$(get_docker_gid); then + echo "Error: Could not determine Docker group ID" + exit 1 +fi + +if ! sed -i '' "s/999/$PRIMARY_GROUP_ID/g" artemis-dev-local-vc-local-ci-mysql.yml; then + echo "Error: Failed to update Docker group ID in compose file" + exit 1 +fi
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
supporting_scripts/playwright/runArtemisInDocker_linux.sh(1 hunks)supporting_scripts/playwright/runArtemisInDocker_macOS.sh(1 hunks)supporting_scripts/playwright/setupUsers.sh(1 hunks)supporting_scripts/playwright/startPlaywright.sh(2 hunks)supporting_scripts/playwright/startPlaywrightUI.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- supporting_scripts/playwright/startPlaywright.sh
- supporting_scripts/playwright/startPlaywrightUI.sh
🔇 Additional comments (2)
supporting_scripts/playwright/runArtemisInDocker_linux.sh (1)
5-5: Same path resolution concerns as setupUsers.shThe path resolution approach has the same portability concerns as discussed in setupUsers.sh.
supporting_scripts/playwright/runArtemisInDocker_macOS.sh (1)
5-5: Same path resolution concerns as setupUsers.shThe path resolution approach has the same portability concerns as discussed in setupUsers.sh.
Checklist
General
Motivation and Context
With the simplified playwright setup, the user should be able to run all to run all tests.
Description
Adds another script, to be able to run either all tests, or run Playwright in UI mode. Also extends the README explaining the two options.
Steps for Testing
Testserver States
Note
These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Click on the badges to get to the test servers.
Review Progress
Code Review
Manual Tests
Summary by CodeRabbit
New Features
startPlaywrightUI.sh).Bug Fixes
startPlaywright.shto run all Playwright tests instead of launching the UI.Documentation
Chores