test: improve the e2e-tests.sh script to make local functional tests faster #1375
Conversation
monsieurswag
force-pushed
the
improve/e2e_tests_script
branch
from
8c562d4 to
ca1d933
Compare
Minor correction Minor adjustment
monsieurswag
force-pushed
the
improve/e2e_tests_script
branch
from
ca1d933 to
aaad6e2
Compare
monsieurswag
requested review from
nas-tabchiche,
eric-intuitem and
Mohamed-Hacene
ab-smith
changed the title
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughThe pull request introduces updates to the project's development setup and testing infrastructure. The changes span across three files: Changes
Poem
🪧 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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
frontend/tests/e2e-tests.sh (1)
83-98: Improve Python script formatting and error handling.The Python script for port checking is more robust with the timeout, but the formatting could be improved.
-if python3 -c " -import socket -try : - s = socket.socket(socket.AF_INET, socket.SOCK_STREAM) - # Using python to check if the port is opened is safer for compatibility - # But it increase the execution time of the script with the timeout - # A local TCP connection should never reach a 1s delay - s.settimeout(1) - s.connect(('127.0.0.1', $BACKEND_PORT)) - port_already_in_used = True -except : - port_already_in_used = False -s.close() -# We use "not" because 0 is True and 1 is False in bash -exit(not port_already_in_used) -"; then +if python3 -c " +import socket +try: + s = socket.socket(socket.AF_INET, socket.SOCK_STREAM) + # Using python to check if the port is opened is safer for compatibility + # But it increases the execution time of the script with the timeout + # A local TCP connection should never reach a 1s delay + s.settimeout(1) + s.connect(('127.0.0.1', $BACKEND_PORT)) + port_already_in_use = True +except (socket.timeout, socket.error): + port_already_in_use = False +finally: + s.close() +# We use "not" because 0 is True and 1 is False in bash +exit(not port_already_in_use) +"; then🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 96-96: Word is of the form "A"B"C" (B indicated). Did you mean "ABC" or "A"B"C"?
(SC2140)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.md(1 hunks)frontend/.gitignore(1 hunks)frontend/tests/e2e-tests.sh(8 hunks)
🧰 Additional context used
🪛 Shellcheck (0.10.0)
frontend/tests/e2e-tests.sh
[warning] 96-96: Word is of the form "A"B"C" (B indicated). Did you mean "ABC" or "A"B"C"?
(SC2140)
[error] 159-159: Arrays implicitly concatenate in [[ ]]. Use a loop (or explicit * instead of @).
(SC2199)
[warning] 230-230: Use 'find .. -print0 | xargs -0 ..' or 'find .. -exec .. +' to allow non-alphanumeric filenames.
(SC2038)
[error] 238-238: Argument mixes string and array. Use * or separate argument.
(SC2145)
[error] 240-240: Argument mixes string and array. Use * or separate argument.
(SC2145)
🔇 Additional comments (5)
frontend/.gitignore (1)
4-6: LGTM! Appropriate test output directories and cache files added to .gitignore.The added entries properly exclude test results, reports, and build cache from version control.
frontend/tests/e2e-tests.sh (3)
2-2: Improve path resolution robustness.Using
realpathis a good improvement for resolving the application directory path, making it more reliable across different environments.
79-80: Good addition of root user warning.The warning about running with root privileges is important for preventing Playwright issues.
188-197: Good optimization with database snapshot feature.The database snapshot feature significantly improves startup time by avoiding unnecessary migrations.
README.md (1)
279-279: LGTM! Poetry version requirement updated.The update to poetry 2.0+ aligns with the changes in the testing infrastructure.
frontend/tests/e2e-tests.sh
Outdated
| FRONTEND_HASH_FILE="$APP_DIR/frontend/tests/.frontend_hash" | ||
| FRONTEND_HASH=$(find "$APP_DIR/frontend/src" -type f \( -name "*.ts" -o -name "*.svelte" \) | xargs -I {} md5sum {} | md5sum) | ||
|
|
||
| if ! cmp <(cat "$FRONTEND_HASH_FILE") <(echo "$FRONTEND_HASH") &>/dev/null; then | ||
| pnpm run build # Required for the "pnpm run preview" command of playwright.config.ts | ||
| echo "$FRONTEND_HASH" > "$FRONTEND_HASH_FILE" | ||
| fi |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix potential issues with filenames in frontend hash calculation.
The frontend hash calculation needs to handle filenames with spaces and special characters properly.
-FRONTEND_HASH=$(find "$APP_DIR/frontend/src" -type f \( -name "*.ts" -o -name "*.svelte" \) | xargs -I {} md5sum {} | md5sum)
+FRONTEND_HASH=$(find "$APP_DIR/frontend/src" -type f \( -name "*.ts" -o -name "*.svelte" \) -print0 | xargs -0 md5sum | md5sum)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| FRONTEND_HASH_FILE="$APP_DIR/frontend/tests/.frontend_hash" | |
| FRONTEND_HASH=$(find "$APP_DIR/frontend/src" -type f \( -name "*.ts" -o -name "*.svelte" \) | xargs -I {} md5sum {} | md5sum) | |
| if ! cmp <(cat "$FRONTEND_HASH_FILE") <(echo "$FRONTEND_HASH") &>/dev/null; then | |
| pnpm run build # Required for the "pnpm run preview" command of playwright.config.ts | |
| echo "$FRONTEND_HASH" > "$FRONTEND_HASH_FILE" | |
| fi | |
| FRONTEND_HASH_FILE="$APP_DIR/frontend/tests/.frontend_hash" | |
| FRONTEND_HASH=$(find "$APP_DIR/frontend/src" -type f \( -name "*.ts" -o -name "*.svelte" \) -print0 | xargs -0 md5sum | md5sum) | |
| if ! cmp <(cat "$FRONTEND_HASH_FILE") <(echo "$FRONTEND_HASH") &>/dev/null; then | |
| pnpm run build # Required for the "pnpm run preview" command of playwright.config.ts | |
| echo "$FRONTEND_HASH" > "$FRONTEND_HASH_FILE" | |
| fi |
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 230-230: Use 'find .. -print0 | xargs -0 ..' or 'find .. -exec .. +' to allow non-alphanumeric filenames.
(SC2038)
| if [[ -n "$QUICK_MODE_ACTIVATED" ]]; then | ||
| pnpm playwright test ./tests/functional/"${TEST_PATHS[@]}" --project=chromium "${SCRIPT_LONG_ARGS[@]}" "${SCRIPT_SHORT_ARGS[@]}" | ||
| else | ||
| npx playwright test ./tests/functional/"${TEST_PATHS[@]}" "${SCRIPT_LONG_ARGS[@]}" | ||
| pnpm playwright test ./tests/functional/"${TEST_PATHS[@]}" "${SCRIPT_LONG_ARGS[@]}" "${SCRIPT_SHORT_ARGS[@]}" |
There was a problem hiding this comment.
Fix array handling in test execution.
The test execution needs to handle arrays properly to avoid word splitting issues.
-if [[ -n "$QUICK_MODE_ACTIVATED" ]]; then
- pnpm playwright test ./tests/functional/"${TEST_PATHS[@]}" --project=chromium "${SCRIPT_LONG_ARGS[@]}" "${SCRIPT_SHORT_ARGS[@]}"
-else
- pnpm playwright test ./tests/functional/"${TEST_PATHS[@]}" "${SCRIPT_LONG_ARGS[@]}" "${SCRIPT_SHORT_ARGS[@]}"
+if [[ -n "$QUICK_MODE_ACTIVATED" ]]; then
+ pnpm playwright test ./tests/functional/"${TEST_PATHS[*]}" --project=chromium "${SCRIPT_LONG_ARGS[*]}" "${SCRIPT_SHORT_ARGS[*]}"
+else
+ pnpm playwright test ./tests/functional/"${TEST_PATHS[*]}" "${SCRIPT_LONG_ARGS[*]}" "${SCRIPT_SHORT_ARGS[*]}"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if [[ -n "$QUICK_MODE_ACTIVATED" ]]; then | |
| pnpm playwright test ./tests/functional/"${TEST_PATHS[@]}" --project=chromium "${SCRIPT_LONG_ARGS[@]}" "${SCRIPT_SHORT_ARGS[@]}" | |
| else | |
| npx playwright test ./tests/functional/"${TEST_PATHS[@]}" "${SCRIPT_LONG_ARGS[@]}" | |
| pnpm playwright test ./tests/functional/"${TEST_PATHS[@]}" "${SCRIPT_LONG_ARGS[@]}" "${SCRIPT_SHORT_ARGS[@]}" | |
| if [[ -n "$QUICK_MODE_ACTIVATED" ]]; then | |
| pnpm playwright test ./tests/functional/"${TEST_PATHS[*]}" --project=chromium "${SCRIPT_LONG_ARGS[*]}" "${SCRIPT_SHORT_ARGS[*]}" | |
| else | |
| pnpm playwright test ./tests/functional/"${TEST_PATHS[*]}" "${SCRIPT_LONG_ARGS[*]}" "${SCRIPT_SHORT_ARGS[*]}" |
🧰 Tools
🪛 Shellcheck (0.10.0)
[error] 238-238: Argument mixes string and array. Use * or separate argument.
(SC2145)
[error] 240-240: Argument mixes string and array. Use * or separate argument.
(SC2145)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
frontend/tests/e2e-tests.sh (3)
53-59: Improve help message formatting consistency.Consider aligning the option descriptions for better readability.
- -q Quick mode: execute only the tests 1 time with no retries and only 1 project - -k Keep a saved snapshot of the initial database and use it to avoid executing useless migrations. + -q Quick mode: execute only the tests 1 time with no retries and only 1 project + -k Keep a saved snapshot of the initial database and use it to avoid executing + useless migrations
79-80: Strengthen the root user warning.Consider making this warning more prominent and possibly exiting the script when run as root to prevent potential issues.
if [[ "$(id -u "$(whoami)")" -eq 0 ]]; then - echo "Running this script with a root account is highly discouraged as it can cause bugs with playwright." + echo "WARNING: Running this script as root is not recommended!" + echo "This can cause permission issues with Playwright and affect test reliability." + echo "Please run the script as a non-root user." + read -p "Do you want to continue anyway? (y/N) " -n 1 -r + echo + if [[ ! $REPLY =~ ^[Yy]$ ]]; then + exit 1 + fi fi
131-134: Add error handling for database snapshot operations.Consider adding error handling for the file operations to provide better feedback.
if [[ -z "$KEEP_DATABASE_SNAPSHOT" && -f "$DB_DIR/$DB_INIT_NAME" ]]; then - rm "$DB_DIR/$DB_INIT_NAME" - echo "| test initial database snapshot deleted" + if rm "$DB_DIR/$DB_INIT_NAME"; then + echo "| test initial database snapshot deleted" + else + echo "| warning: failed to delete test initial database snapshot" + fi fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/tests/e2e-tests.sh(8 hunks)
🧰 Additional context used
🪛 Shellcheck (0.10.0)
frontend/tests/e2e-tests.sh
[warning] 96-96: Word is of the form "A"B"C" (B indicated). Did you mean "ABC" or "A"B"C"?
(SC2140)
[error] 159-159: Arrays implicitly concatenate in [[ ]]. Use a loop (or explicit * instead of @).
(SC2199)
[error] 238-238: Argument mixes string and array. Use * or separate argument.
(SC2145)
[error] 240-240: Argument mixes string and array. Use * or separate argument.
(SC2145)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (4)
frontend/tests/e2e-tests.sh (4)
2-5: LGTM! Good use of realpath for reliable path resolution.Using
realpathensures consistent path resolution across different environments.
34-39: LGTM! Well-structured argument handling for new features.The new flags (-q, -v, -k) provide useful options for:
- Quick mode execution
- Backend output visibility
- Database snapshot management
229-235: LGTM! Properly handling special characters in filenames.The implementation now correctly handles filenames with spaces and special characters using
-print0andxargs -0.
237-240: Fix array handling in test execution.The test execution needs to handle arrays properly to avoid word splitting issues.
if [[ -n "$QUICK_MODE_ACTIVATED" ]]; then - pnpm playwright test ./tests/functional/"${TEST_PATHS[@]}" --project=chromium "${SCRIPT_LONG_ARGS[@]}" "${SCRIPT_SHORT_ARGS[@]}" + pnpm playwright test ./tests/functional/"${TEST_PATHS[*]}" --project=chromium "${SCRIPT_LONG_ARGS[*]}" "${SCRIPT_SHORT_ARGS[*]}" else - pnpm playwright test ./tests/functional/"${TEST_PATHS[@]}" "${SCRIPT_LONG_ARGS[@]}" "${SCRIPT_SHORT_ARGS[@]}" + pnpm playwright test ./tests/functional/"${TEST_PATHS[*]}" "${SCRIPT_LONG_ARGS[*]}" "${SCRIPT_SHORT_ARGS[*]}" fi🧰 Tools
🪛 Shellcheck (0.10.0)
[error] 238-238: Argument mixes string and array. Use * or separate argument.
(SC2145)
[error] 240-240: Argument mixes string and array. Use * or separate argument.
(SC2145)
| if python3 -c " | ||
| import socket | ||
| try : | ||
| s = socket.socket(socket.AF_INET, socket.SOCK_STREAM) | ||
| # Using python to check if the port is opened is safer for compatibility | ||
| # But it increase the execution time of the script with the timeout | ||
| # A local TCP connection should never reach a 1s delay | ||
| s.settimeout(1) | ||
| s.connect(('127.0.0.1', $BACKEND_PORT)) | ||
| port_already_in_used = True | ||
| except : | ||
| port_already_in_used = False | ||
| s.close() | ||
| # We use "not" because 0 is True and 1 is False in bash | ||
| exit(not port_already_in_used) | ||
| "; then |
There was a problem hiding this comment.
Fix socket resource handling in port check.
The socket is not properly closed in case of an exception, which could lead to resource leaks.
if python3 -c "
import socket
try:
s = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
s.settimeout(1)
s.connect(('127.0.0.1', $BACKEND_PORT))
port_already_in_used = True
+ s.close()
except:
port_already_in_used = False
-s.close()
+ try:
+ s.close()
+ except:
+ pass
exit(not port_already_in_used)
"; then📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if python3 -c " | |
| import socket | |
| try : | |
| s = socket.socket(socket.AF_INET, socket.SOCK_STREAM) | |
| # Using python to check if the port is opened is safer for compatibility | |
| # But it increase the execution time of the script with the timeout | |
| # A local TCP connection should never reach a 1s delay | |
| s.settimeout(1) | |
| s.connect(('127.0.0.1', $BACKEND_PORT)) | |
| port_already_in_used = True | |
| except : | |
| port_already_in_used = False | |
| s.close() | |
| # We use "not" because 0 is True and 1 is False in bash | |
| exit(not port_already_in_used) | |
| "; then | |
| if python3 -c " | |
| import socket | |
| try: | |
| s = socket.socket(socket.AF_INET, socket.SOCK_STREAM) | |
| # Using python to check if the port is opened is safer for compatibility | |
| # But it increase the execution time of the script with the timeout | |
| # A local TCP connection should never reach a 1s delay | |
| s.settimeout(1) | |
| s.connect(('127.0.0.1', $BACKEND_PORT)) | |
| port_already_in_used = True | |
| s.close() | |
| except: | |
| port_already_in_used = False | |
| try: | |
| s.close() | |
| except: | |
| pass | |
| exit(not port_already_in_used) | |
| "; then |
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 96-96: Word is of the form "A"B"C" (B indicated). Did you mean "ABC" or "A"B"C"?
(SC2140)
Summary by CodeRabbit
Documentation
poetry 2.0+as the required version for development setupChores
.gitignorewith new test-related entries