Development: Improve and simplify local playwright setup
#9796
Conversation
github-actions
bot
added
tests
documentation
config-change
|
There hasn't been any activity on this pull request recently. Therefore, this pull request has been automatically marked as stale and will be closed if no further activity occurs within seven days. Thank you for your contributions. |
SimonEntholzer
changed the title
Development: Improve and simplify local playwright setup
|
Warning Rate limit exceeded@SimonEntholzer has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 0 minutes and 5 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe pull request introduces updates to the Playwright setup documentation and scripts. Key changes include a revised 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: 8
🧹 Outside diff range and nitpick comments (8)
supporting_scripts/playwright/setupUsers.sh (1)
1-1: Use more specific shebang for Python environmentSince this script manages Python virtual environments, it's better to use bash explicitly.
-#!/bin/sh +#!/bin/bashsupporting_scripts/playwright/runArtemisInDocker_linux.sh (2)
20-21: Create backup when modifying Docker Compose fileThe sed command should create a backup file when modifying the Docker Compose configuration.
-sed -i "s/999/$(getent group docker | cut -d: -f3)/g" artemis-dev-local-vc-local-ci-mysql.yml +sed -i.bak "s/999/$(getent group docker | cut -d: -f3)/g" artemis-dev-local-vc-local-ci-mysql.yml
1-31: Consider adding cleanup functionalityThe script should handle cleanup of resources on failure or interruption.
Consider adding a trap for cleanup:
+cleanup() { + echo "Cleaning up..." + cd docker && docker compose -f artemis-dev-local-vc-local-ci-mysql.yml down + exit 1 +} + +trap cleanup INT TERMsupporting_scripts/playwright/runArtemisInDocker_macOS.sh (1)
34-37: Add npm and node version validationThe script should verify that the required versions of npm and node are installed before proceeding with npm operations.
Add these checks before npm operations:
+# Check for required node version +required_node_version="16.0.0" +if ! command -v node >/dev/null 2>&1; then + echo "Error: node is not installed" >&2 + exit 1 +fi +if ! node -v | grep -q "v$required_node_version"; then + echo "Error: node version $required_node_version or higher is required" >&2 + exit 1 +fi + echo "Installing Artemis npm dependencies and start Artemis client" npm install npm run startsupporting_scripts/playwright/README.md (2)
14-14: Fix grammatical issuesThere are two grammatical issues in the text:
- Line 14: "you are be able" should be "you are able"
- Line 20: "it's" should be "its" (possessive form)
Apply these corrections:
-After this step, you are be able to access Artemis locally as you usually would be. +After this step, you are able to access Artemis locally as you usually would be. -Playwright needs users for it's tests. +Playwright needs users for its tests.Also applies to: 20-20
🧰 Tools
🪛 LanguageTool
[grammar] ~14-~14: Consider using either the present participle “are being” here.
Context: ...he client via npm. After this step, you are be able to access Artemis locally as you u...(BEEN_PART_AGREEMENT)
1-27: Enhance documentation with prerequisites and troubleshootingWhile the documentation is well-structured, it would benefit from additional sections covering prerequisites and common issues.
Consider adding these sections:
# Prerequisites - Docker Desktop installed and running - Node.js version 16.0.0 or higher - npm version 8.0.0 or higher - At least 4GB of free RAM - At least 10GB of free disk space # Troubleshooting ## Common Issues 1. Docker fails to start - Ensure Docker Desktop is running - Check system resources 2. npm install fails - Clear npm cache: `npm cache clean --force` - Delete node_modules and package-lock.json 3. Playwright tests fail - Verify all users are created successfully - Check Artemis server logs for errors🧰 Tools
🪛 LanguageTool
[grammar] ~14-~14: Consider using either the present participle “are being” here.
Context: ...he client via npm. After this step, you are be able to access Artemis locally as you u...(BEEN_PART_AGREEMENT)
[uncategorized] ~20-~20: Did you mean “its” (the possessive pronoun)?
Context: ...Setup users Playwright needs users for it's tests. If you do not have users set up,...(ITS_PREMIUM)
docs/dev/playwright.rst (2)
43-44: Document the rationale for timeout valuesNew timeout configurations have been added:
- FAST_TEST_TIMEOUT_SECONDS=45
- SLOW_TEST_TIMEOUT_SECONDS=180
Consider adding a comment explaining:
- What determines a "fast" vs "slow" test
- Why these specific values were chosen
- Which types of tests should use which timeout
52-62: Enhance user setup documentationThe user setup section needs several improvements:
- Add example output or expected results from running
setupUsers.sh- Explain what roles/permissions the generated users will have
- Clarify the limitation regarding the template string changes
Consider adding this content:
setupUsers.sh +The script will generate the following test users: +- artemis_test_user_1 (student role) +- artemis_test_user_2 (tutor role) +... + +Note: The script currently only supports the default template format due to [specific reason]. +If you need custom usernames, please modify the script accordingly.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
src/main/resources/config/application-local-template.ymlis excluded by!**/*.yml
📒 Files selected for processing (10)
docker/artemis/config/dev-local-vc-local-ci.env(1 hunks)docs/dev/playwright.rst(1 hunks)src/test/playwright/init/ImportUsers.spec.ts(0 hunks)src/test/playwright/package.json(1 hunks)src/test/playwright/playwright.env(1 hunks)supporting_scripts/playwright/README.md(1 hunks)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(1 hunks)
💤 Files with no reviewable changes (1)
- src/test/playwright/init/ImportUsers.spec.ts
✅ Files skipped from review due to trivial changes (2)
- src/test/playwright/playwright.env
- supporting_scripts/playwright/startPlaywright.sh
🧰 Additional context used
🪛 Shellcheck (0.10.0)
supporting_scripts/playwright/setupUsers.sh
[warning] 13-13: In POSIX sh, 'source' in place of '.' is undefined.
(SC3046)
🪛 LanguageTool
supporting_scripts/playwright/README.md
[grammar] ~14-~14: Consider using either the present participle “are being” here.
Context: ...he client via npm. After this step, you are be able to access Artemis locally as you u...
(BEEN_PART_AGREEMENT)
[uncategorized] ~20-~20: Did you mean “its” (the possessive pronoun)?
Context: ...Setup users Playwright needs users for it's tests. If you do not have users set up,...
(ITS_PREMIUM)
🔇 Additional comments (3)
docker/artemis/config/dev-local-vc-local-ci.env (1)
5-5: Verify the impact of adding the 'local' profile
The addition of the 'local' profile to SPRING_PROFILES_ACTIVE might affect Spring configuration loading order and override behavior.
✅ Verification successful
Let me gather more specific information about the local profile usage.
Adding 'local' profile is consistent with existing configuration
The 'local' profile addition is safe and aligns with the existing development setup:
- The application already uses multiple local-related profiles (localci, localvc) with well-defined configurations
- Found application-local configuration files contain development-specific settings
- Java classes using @Profile("local") are primarily test and development-focused services
- Profile ordering in SPRING_PROFILES_ACTIVE follows the standard pattern with development profiles at the end
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any local profile configurations that might conflict
rg -l "application-local\." --glob '!*.env'
# Search for profile-specific beans or configurations
ast-grep --pattern 'class $_ {
$$$
@Profile("local")
$$$
}'
Length of output: 721
Script:
#!/bin/bash
# Check content of application-local properties/yml files
fd -e yml -e yaml -e properties 'application-local' -x cat {}
# Search for local profile references in configuration files
rg -A 3 '"local"' --type yaml --type properties
# Search for Spring configuration classes with local profile
rg -l '@Profile.*local' --type java
Length of output: 4840
docs/dev/playwright.rst (2)
32-33: Verify template changes across the codebase
The username and password templates have been simplified by removing the USERID suffix. This change needs to be consistent with the actual implementation.
✅ Verification successful
Template changes in documentation match the implementation
The documentation accurately reflects the current implementation:
- The environment file (
playwright.env) uses the same template format:artemis_test_user_ - The implementation in
users.tscorrectly uses these environment variables with fallback values - The templates are used as prefixes and combined with a user ID selector in the actual code
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the template changes are consistent across the codebase
# Check the actual environment file
rg "PLAYWRIGHT_USERNAME_TEMPLATE|PLAYWRIGHT_PASSWORD_TEMPLATE" src/test/playwright/playwright.env
# Check for any hardcoded references to the old format
rg "artemis_test_user_\d+" src/test/playwright/
Length of output: 245
Script:
#!/bin/bash
# Let's check for any test files or configuration that might be using these templates
rg -A 2 "PLAYWRIGHT_USERNAME_TEMPLATE|PLAYWRIGHT_PASSWORD_TEMPLATE" src/test/playwright/
# Also check for any dynamic user generation or related patterns
rg -A 2 "generateUser|createUser|setupUser" src/test/playwright/
Length of output: 1263
66-67: 🛠️ Refactor suggestion
Improve OS-specific installation instructions
The documentation mentions potential OS-specific installation issues but doesn't provide details.
Please add:
- List of affected operating systems
- Package manager commands for manual installation
- Troubleshooting steps for common installation issues
Example addition:
npm run playwright:install
+Known OS-specific requirements:
+- Ubuntu/Debian: Additional dependencies needed (libwebkit2gtk-4.0-dev, etc.)
+- macOS: No additional steps required
+- Windows: May require Microsoft Visual C++ RedistributableAlso applies to: 71-72
✅ Verification successful
Based on the search results, I can see that the project uses cross-platform tools like cross-env and has Docker-based test infrastructure, but there's no explicit documentation about OS-specific installation requirements for Playwright. Let me check the package.json and any setup scripts to understand the installation process better.
Let me check if there are any specific OS-related dependencies or requirements mentioned in the Playwright documentation or configuration files.
Based on the search results and package.json content, I can now generate a final response. The playwright:install command uses npx playwright install --with-deps chromium which includes system dependencies, and there's evidence of cross-platform support through cross-env usage.
Add specific OS installation instructions
Please add the following OS-specific details to the documentation:
- Ubuntu/Debian:
npm run playwright:install
+## Operating System Requirements
+
+### Ubuntu/Debian
+```bash
+# Install system dependencies
+sudo apt-get update
+sudo apt-get install -y libwebkit2gtk-4.0-0 libgtk-3-0 libgbm1
+```
+
+### Windows
+- No additional system dependencies required
+- Ensure Microsoft Visual C++ Redistributable is installed (usually present by default)
+
+### macOS
+- No additional system dependencies required
+
+## Troubleshooting
+If the automatic installation fails:
+1. Check system requirements above for your OS
+2. Run `npx playwright install-deps chromium` to install dependencies separately
+3. Run `npx playwright install chromium` to install browser binaries🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the npm command exists in package.json
# Check if the command exists in package.json
rg '"playwright:install"' src/test/playwright/package.json
Length of output: 137
Script:
#!/bin/bash
# Let's check the Playwright documentation and test files for OS-specific information
rg -i "os|linux|ubuntu|debian|windows|macos" docs/dev/playwright.rst
# Check test files for OS-specific configurations or imports
rg -i "os|platform|process\.platform" src/test/playwright/ -A 2
# Look for any existing troubleshooting or installation guides
fd README.md src/test/playwright/ -x cat {}
Length of output: 67193
Script:
#!/bin/bash
# Check package.json for installation scripts and dependencies
cat src/test/playwright/package.json
# Look for any setup or installation scripts
fd -e sh -e bash setup install src/test/playwright/
# Check for any documentation about installation
rg -i "install|setup|prerequisite" docs/dev/playwright.rst
Length of output: 2380
Script:
#!/bin/bash
# Check Playwright's config and environment files
cat src/test/playwright/playwright.config.ts
cat src/test/playwright/playwright.env
# Look for any OS-specific dependencies in the documentation
rg -i "linux|ubuntu|debian|windows|macos|dependencies" docs/dev/ -A 2
Length of output: 96623
|
Left a small comment. Also I added this branch manually to bamboo, as it wasn't created for some reason 😅 |
SimonEntholzer
dismissed stale reviews from muradium, BBesrour, az108, and coderabbitai[bot]
via
b0e9a78
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (6)
supporting_scripts/playwright/README.md (5)
12-12: Fix grammar in the sentenceThe sentence contains a grammatical error.
-After this step, you are be able to access Artemis locally as you usually would be. +After this step, you will be able to access Artemis locally as you usually would be.🧰 Tools
🪛 LanguageTool
[grammar] ~12-~12: Consider using either the present participle “are being” here.
Context: ...he client via npm. After this step, you are be able to access Artemis locally as you u...(BEEN_PART_AGREEMENT)
10-11: Consider adding supported OS version informationTo prevent potential compatibility issues, it would be helpful to specify which OS versions are supported for both macOS and Linux scripts.
18-18: Fix possessive pronoun usageThe word "it's" (contraction of "it is") should be "its" (possessive pronoun).
-Playwright needs users for it's tests. +Playwright needs users for its tests.🧰 Tools
🪛 LanguageTool
[uncategorized] ~18-~18: Did you mean “its” (the possessive pronoun)?
Context: ...Setup users Playwright needs users for it's tests. If you do not have users set up,...(ITS_PREMIUM)
18-20: Add information about prerequisitesThe section should mention any prerequisites required for running the
setupUsers.shscript (e.g., Python environment setup).🧰 Tools
🪛 LanguageTool
[uncategorized] ~18-~18: Did you mean “its” (the possessive pronoun)?
Context: ...Setup users Playwright needs users for it's tests. If you do not have users set up,...(ITS_PREMIUM)
24-25: Enhance setup instructions clarityConsider adding:
- An explanation of what "UI mode" means and its benefits
- The exact location where these scripts can be found
- The expected outcome when running in UI mode
supporting_scripts/playwright/setupUsers.sh (1)
1-5: Enhance script robustness and documentationConsider adding these improvements:
- Additional error handling flags
- Script description and usage instructions
#!/bin/sh -# We use the supporting scripts to create users +# Script: setupUsers.sh +# Description: Creates test users for Artemis Playwright testing environment +# Usage: ./setupUsers.sh -set -e +set -euo pipefail + +# Ensure script is sourced from the correct location +if [ "$(basename "$0")" = "setupUsers.sh" ]; then + cd "$(dirname "$0")" || exit 1 +fi
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
supporting_scripts/playwright/README.md(1 hunks)supporting_scripts/playwright/runArtemisInDocker_linux.sh(1 hunks)supporting_scripts/playwright/runArtemisInDocker_macOS.sh(1 hunks)supporting_scripts/playwright/setupUsers.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- supporting_scripts/playwright/runArtemisInDocker_macOS.sh
- supporting_scripts/playwright/runArtemisInDocker_linux.sh
🧰 Additional context used
🪛 LanguageTool
supporting_scripts/playwright/README.md
[grammar] ~12-~12: Consider using either the present participle “are being” here.
Context: ...he client via npm. After this step, you are be able to access Artemis locally as you u...
(BEEN_PART_AGREEMENT)
[uncategorized] ~18-~18: Did you mean “its” (the possessive pronoun)?
Context: ...Setup users Playwright needs users for it's tests. If you do not have users set up,...
(ITS_PREMIUM)
🪛 Shellcheck (0.10.0)
supporting_scripts/playwright/setupUsers.sh
[warning] 14-14: In POSIX sh, 'source' in place of '.' is undefined.
(SC3046)
🔇 Additional comments (5)
supporting_scripts/playwright/README.md (2)
1-7: LGTM!
The introduction provides a clear and concise overview of the setup process.
10-10: Verify the existence of referenced scripts
Let's ensure all referenced scripts exist in the repository.
Also applies to: 19-19, 24-24
✅ Verification successful
All referenced scripts are present in the repository
All shell scripts mentioned in the README.md (runArtemisInDocker_macOS.sh, runArtemisInDocker_linux.sh, setupUsers.sh, and startPlaywright.sh) exist in the supporting_scripts/playwright/ directory.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence of all referenced scripts
echo "Checking for referenced scripts..."
for script in "runArtemisInDocker_macOS.sh" "runArtemisInDocker_linux.sh" "setupUsers.sh" "startPlaywright.sh"; do
if fd "$script" .; then
echo "✓ Found: $script"
else
echo "✗ Missing: $script"
fi
done
Length of output: 1150
supporting_scripts/playwright/setupUsers.sh (3)
10-14: Replace 'source' with POSIX-compliant '.' command
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 14-14: In POSIX sh, 'source' in place of '.' is undefined.
(SC3046)
1-19: Verify script integration with Playwright setup
Let's verify how this script integrates with other Playwright setup scripts.
✅ Verification successful
Script is properly integrated with Playwright setup
The script is correctly integrated into the Playwright testing workflow as evidenced by:
- It's documented in
docs/dev/playwright.rstas a required step (Step 3) for configuring test users before running Playwright tests - Works in conjunction with other scripts in the
supporting_scripts/playwrightdirectory:runArtemisInDocker_linux.sh/runArtemisInDocker_macOS.shset up the Docker environmentsetupUsers.shcreates the required test usersstartPlaywright.shhandles the Playwright installation and execution
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for related setup scripts and their execution order
echo "Checking for related Playwright setup scripts..."
fd -t f -e sh . supporting_scripts/playwright
# Check if this script is referenced in documentation
echo -e "\nChecking for script references in documentation..."
rg -l "setupUsers.sh" docs/
Length of output: 532
Script:
#!/bin/bash
# Check the content of the Playwright documentation
echo "Checking Playwright documentation..."
cat docs/dev/playwright.rst
# Check the content of related Playwright scripts
echo -e "\nChecking startPlaywright.sh..."
cat supporting_scripts/playwright/startPlaywright.sh
echo -e "\nChecking Docker setup scripts..."
cat supporting_scripts/playwright/runArtemisInDocker_linux.sh
cat supporting_scripts/playwright/runArtemisInDocker_macOS.sh
Length of output: 11924
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 14-14: In POSIX sh, 'source' in place of '.' is undefined.
(SC3046)
16-19: 🛠️ Refactor suggestion
Add error handling and success verification
The script should verify successful execution of critical commands and provide meaningful error messages.
-cd "$artemis_path/supporting_scripts/course-scripts/quick-course-setup"
+cd "$artemis_path/supporting_scripts/course-scripts/quick-course-setup" || {
+ echo "Error: Failed to change to quick-course-setup directory" >&2
+ exit 1
+}
-python3 -m pip install -r requirements.txt
-python3 create_users.py
+if [ ! -f "requirements.txt" ]; then
+ echo "Error: requirements.txt not found" >&2
+ exit 1
+fi
+
+echo "Installing dependencies..."
+if ! python3 -m pip install -r requirements.txt; then
+ echo "Error: Failed to install requirements" >&2
+ exit 1
+fi
+
+echo "Creating users..."
+if ! python3 create_users.py; then
+ echo "Error: Failed to create users" >&2
+ exit 1
+fi
+
+echo "Successfully created users"Likely invalid or redundant comment.
…simplify-setup' into feature/playwright/simplify-setup
Checklist
General
Motivation and Context
Setting up playwright initially is quite cumbersome, as several steps need to be followed before it can be used.
Description
This PR simplifies the setup by providing three simple scripts, the user can execute to run playwright fast.
Steps for Testing
checkout feature/playwright/simplify-setup)supporting_scripts/playwrightReview Progress
Code Review
Manual Tests
Summary by CodeRabbit
Release Notes
New Features
Improvements
Documentation