test: write functional tests for backup restore #1477
Conversation
WalkthroughA new functional test is added in the backup-restore module to validate the database export and subsequent import functionality. The test dismisses any obstructive modal dialogs, navigates to the backup/restore section, and verifies the URL and visibility of the export button. It then initiates the export process, waits for a download event, verifies that the downloaded file has a .bak extension and contains data, saves the file locally, and finally tests that importing the file redirects the user to the login page. Changes
Suggested reviewers
Poem
✨ Finishing Touches
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
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: 3
🧹 Nitpick comments (3)
frontend/tests/functional/backup-restore.test.ts (3)
1-3: Remove unused imports and fixtures.The test imports
setHttpResponsesListenerbut never uses it. Similarly, thelogedPagefixture is received but not used in the test.-import { expect, setHttpResponsesListener, test } from '../utils/test-utils.js'; +import { expect, test } from '../utils/test-utils.js'; -test('Database export should generate valid backup file', async ({ logedPage, page }) => { +test('Database export should generate valid backup file', async ({ page }) => {
8-21: Refactor modal dismissal logic for better reliability.The current approach of dismissing modals could be more robust. Consider creating a reusable helper function that systematically checks and dismisses all possible modals.
async function dismissAllModals(page) { const modalSelectors = [ { testId: 'modal-backdrop', action: 'Escape' }, { selector: '#driver-dummy-element', closeSelector: '.driver-popover-close-btn' } ]; for (const { testId, selector, action, closeSelector } of modalSelectors) { const element = testId ? page.getByTestId(testId) : page.locator(selector); if (await element.isVisible()) { if (action) { await element.press(action); } else if (closeSelector) { await page.locator(closeSelector).first().click(); } await expect(element).not.toBeVisible(); } } }
36-39: Enhance filename validation.The current validation only checks the file extension. Consider adding more thorough validation of the filename pattern.
-expect(fileName.endsWith('.bak')).toBeTruthy(); +expect(fileName).toMatch(/^\d{8}_\d{6}\.bak$/); // Assuming format: YYYYMMDD_HHMMSS.bak +const [date, time] = fileName.split('.')[0].split('_'); +expect(date).toMatch(/^\d{8}$/); +expect(time).toMatch(/^\d{6}$/);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/tests/functional/backup-restore.test.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: migrations-check (3.12)
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: startup-docker-compose-test
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
frontend/tests/functional/backup-restore.test.ts (1)
22-27: LGTM! Navigation steps are well-structured.The navigation logic is clear and includes appropriate assertions for both URL and UI state.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
frontend/tests/functional/backup-restore.test.ts (2)
32-42: Consider adding timeout for download event.While the test logic is correct, downloads might take longer in slower environments.
-const downloadPromise = page.waitForEvent('download'); +const downloadPromise = page.waitForEvent('download', { timeout: 10000 });
44-71: Enhance file content validation.The test verifies basic JSON structure but could be more thorough.
expect(jsonData).toBeDefined(); -// expect(jsonData).toHaveProperty('meta'); +expect(jsonData).toHaveProperty('version'); +expect(jsonData).toHaveProperty('data'); +expect(jsonData.data).toBeInstanceOf(Object);frontend/tests/e2e-tests.sh (2)
174-175: Fix potential return value masking.The current implementation might mask git command failures.
-export CISO_ASSISTANT_VERSION=$(git describe --tags --always) -export CISO_ASSISTANT_BUILD=$(git rev-parse --short HEAD) +CISO_ASSISTANT_VERSION=$(git describe --tags --always) +export CISO_ASSISTANT_VERSION +CISO_ASSISTANT_BUILD=$(git rev-parse --short HEAD) +export CISO_ASSISTANT_BUILD🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 174-174: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 175-175: Declare and assign separately to avoid masking return values.
(SC2155)
238-241: Fix array argument handling in playwright test commands.The current implementation might cause issues with argument parsing.
- 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[*]}" - 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[*]}"🧰 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)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/functional-tests.yml(1 hunks)frontend/tests/e2e-tests.sh(4 hunks)frontend/tests/functional/backup-restore.test.ts(1 hunks)
🧰 Additional context used
🪛 Shellcheck (0.10.0)
frontend/tests/e2e-tests.sh
[error] 49-49: Arrays implicitly concatenate in [[ ]]. Use a loop (or explicit * instead of @).
(SC2199)
[error] 49-49: Arrays implicitly concatenate in [[ ]]. Use a loop (or explicit * instead of @).
(SC2199)
[error] 106-106: Arrays implicitly concatenate in [[ ]]. Use a loop (or explicit * instead of @).
(SC2199)
[error] 111-111: Arrays implicitly concatenate in [[ ]]. Use a loop (or explicit * instead of @).
(SC2199)
[error] 157-157: Arrays implicitly concatenate in [[ ]]. Use a loop (or explicit * instead of @).
(SC2199)
[warning] 174-174: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 175-175: Declare and assign separately to avoid masking return values.
(SC2155)
[error] 220-220: Argument mixes string and array. Use * or separate argument.
(SC2145)
[error] 225-225: Argument mixes string and array. Use * or separate argument.
(SC2145)
[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 (6)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: startup-docker-compose-test
- GitHub Check: enterprise-functional-tests (3.12, chromium)
🔇 Additional comments (3)
frontend/tests/functional/backup-restore.test.ts (2)
11-24: LGTM! Thorough modal handling.The code properly handles both standard modals and driver tour modals, ensuring a clean test environment.
77-87: LGTM! Comprehensive import verification.The test properly handles the file upload, confirmation dialog, and verifies the expected redirect.
.github/workflows/functional-tests.yml (1)
97-98: LGTM! Good addition of version tracking.Adding version and build information to the environment variables improves traceability.
nas-tabchiche
force-pushed
the
test/backup-restore-regression-tests
branch
3 times, most recently
from
ba8f027 to
9a8e2ce
Compare
nas-tabchiche
force-pushed
the
test/backup-restore-regression-tests
branch
from
9a8e2ce to
57d83a2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
backend/serdes/views.py (4)
66-88: Consider leveraging a transactional approach
While the in-memory backup logic is functional, wrapping it in a database transaction (e.g., Django'satomic) could prevent data inconsistencies if multiple backup requests occur simultaneously.
113-129: Perform restore in a single transaction
In the exception handler, you flush and restore the database on error. Using a transaction-based approach (e.g.,atomic) would prevent partial data loads if an error occurs or if the process is interrupted midway.
133-133: Clarify the version mismatch error message
Currently, the code always returns{"error": "LowerBackupVersion"}regardless of whether the backup version is higher or lower. Consider a more precise message to reflect any mismatch.Example adjustment:
- {"error": "LowerBackupVersion"}, status=status.HTTP_400_BAD_REQUEST + { + "error": f"IncompatibleBackupVersion - Backup version {backup_version} does not match current version {current_version}" + }, + status=status.HTTP_400_BAD_REQUEST
170-170: Avoid re-encoding JSON for large backups
Re-serializing the backup data could be memory-intensive if handling large datasets. Consider streaming or chunk-based solutions to improve performance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/functional-tests.yml(2 hunks)backend/serdes/views.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/functional-tests.yml
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: test (3.12)
🔇 Additional comments (4)
backend/serdes/views.py (4)
7-7: Use structured logging
The addition ofstructlogis a beneficial step for better logging clarity and correlation.
9-9: Approved import
Bringing indumpdatais appropriate for the in-memory backup approach below.
16-16: Confirmed usage of version constants
ImportingSCHEMA_VERSIONandVERSIONaligns well with the version checks in the code.
168-168: Verify schema version checks
compare_schema_versions(int(schema_version), backup_version)is called but its outcome isn't handled here. Confirm that any mismatch or error raised by this function is appropriately captured.
| try: | ||
| # Here we load the data from stdin | ||
| management.call_command("flush", interactive=False) | ||
| raise Exception("Test") |
There was a problem hiding this comment.
Remove leftover debug exception
Line 97 forcibly raises an exception labeled "Test," causing every restore attempt to fail. This appears to be a leftover debug statement.
Apply this diff to remove the forced exception:
- raise Exception("Test")📝 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.
| try: | |
| # Here we load the data from stdin | |
| management.call_command("flush", interactive=False) | |
| raise Exception("Test") | |
| try: | |
| management.call_command("flush", interactive=False) |
Summary by CodeRabbit