Refactor: Optimize module management with local caching#2815
Refactor: Optimize module management with local caching#2815
Conversation
WalkthroughThis update introduces local caching for module activation state in API utilities, modifies navigation and selectors for product page interactions, adjusts authentication test setup to disable email verification, and changes how storage state file paths are resolved in test data. No new public API signatures are introduced, but several method implementations are updated. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Script
participant ApiUtils as ApiUtils
participant FileSystem as Local JSON File
participant Server as API Server
Test->>ApiUtils: activateModules(moduleIds)
ApiUtils->>FileSystem: getAllModulesFromLocalJson()
FileSystem-->>ApiUtils: Module list (cached)
alt All modules already active
ApiUtils-->>Test: [null, null]
else Some modules inactive
ApiUtils->>Server: Activate modules API call
Server-->>ApiUtils: API response
ApiUtils->>Server: getAllModules()
Server-->>ApiUtils: Updated module list
ApiUtils->>FileSystem: Save modules.json
ApiUtils-->>Test: [APIResponse, responseBody]
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Possibly related PRs
Suggested labels
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (4)
tests/pw/pages/selectors.ts (1)
3878-3878: Consider re-scoping the XPath to avoid accidental matchesThe new selector is more tolerant of the optional “+” prefix, which is great.
However, by starting with//afrom the document root it now matches any anchor containing “Add new product”. If the phrase ever appears outside the products widget, the test might interact with the wrong element.- addNewProduct: '//a[contains(normalize-space(.), "Add new product")]', + addNewProduct: '//div[@class="dashboard-widget products"]//a[contains(normalize-space(.), "Add new product")]',Scoping the search to the
dashboard-widget productscontainer keeps the flexibility while preventing false positives.tests/pw/utils/testData.ts (1)
85-102: Consolidate path resolution to avoid multiplerequirecallsEach call to
require('path').resolveincurs an unnecessary module lookup and clutters the object literal. Cache the resolved auth directory once and reuse it – this improves readability and keeps the file DRY.- storageState: require('path').resolve(__dirname, '../playwright/.auth/adminStorageState.json'), + storageState: `${AUTH_DIR}/adminStorageState.json`, ... - storageState: require('path').resolve(__dirname, '../playwright/.auth/vendorStorageState.json'), + storageState: `${AUTH_DIR}/vendorStorageState.json`, ... - storageState: require('path').resolve(__dirname, '../playwright/.auth/vendor2StorageState.json'), + storageState: `${AUTH_DIR}/vendor2StorageState.json`, ... - storageState: require('path').resolve(__dirname, '../playwright/.auth/customerStorageState.json'), + storageState: `${AUTH_DIR}/customerStorageState.json`, ... - storageState: require('path').resolve(__dirname, '../playwright/.auth/customer2StorageState.json'), + storageState: `${AUTH_DIR}/customer2StorageState.json`,Add once near the top of the file (outside the diff context):
import path from 'node:path'; const AUTH_DIR = path.resolve(__dirname, '../playwright/.auth');This single constant is cheap, clear and eliminates five duplicate
requireinvocations.tests/pw/utils/apiUtils.ts (2)
858-863: Consider async file operations and error handling.The local caching implementation is good, but consider these improvements:
Use async file operations:
fs.writeFileSyncblocks the event loop. Consider usingfs.promises.writeFileorfs.writeFilewith proper async/await.Add error handling: File operations can fail due to permissions, disk space, etc.
Path robustness: The relative path
../playwright/modules.jsonassumes a specific directory structure that could be fragile.- // Save modules to local JSON file - fs.writeFileSync( - path.resolve(__dirname, '../playwright/modules.json'), - JSON.stringify(responseBody, null, 2) - ); + // Save modules to local JSON file + try { + await fs.promises.writeFile( + path.resolve(__dirname, '../playwright/modules.json'), + JSON.stringify(responseBody, null, 2) + ); + } catch (error) { + console.warn('Failed to save modules to local cache:', error.message); + }
875-883: Good error handling, consider async operations.The method correctly handles file read errors by returning an empty array. However, consider using async file operations for consistency and better performance.
- async getAllModulesFromLocalJson(): Promise<{ id: string; active: boolean }[]> { - try { - const allModules = JSON.parse(fs.readFileSync(path.resolve(__dirname, '../playwright/modules.json'), 'utf8')); - return allModules; - } catch (error) { - // Return empty array if file doesn't exist or is invalid - return []; - } - } + async getAllModulesFromLocalJson(): Promise<{ id: string; active: boolean }[]> { + try { + const data = await fs.promises.readFile(path.resolve(__dirname, '../playwright/modules.json'), 'utf8'); + return JSON.parse(data); + } catch (error) { + // Return empty array if file doesn't exist or is invalid + return []; + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
tests/pw/pages/productsPage.ts(1 hunks)tests/pw/pages/selectors.ts(1 hunks)tests/pw/tests/e2e/_auth.setup.ts(3 hunks)tests/pw/utils/apiUtils.ts(3 hunks)tests/pw/utils/testData.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/pw/pages/productsPage.ts (1)
tests/pw/utils/testData.ts (1)
data(63-2914)
🔇 Additional comments (6)
tests/pw/tests/e2e/_auth.setup.ts (1)
9-9: LGTM!The import is correctly added to support the email verification disabling functionality used later in the file.
tests/pw/pages/productsPage.ts (1)
617-617: LGTM! Navigation optimization improves efficiency.The change from navigating to the vendor dashboard home page to directly navigating to the products page is a logical optimization. Since the method needs to extract the product edit nonce from the "add new product" link, going directly to the products page (
dashboard/products) eliminates an unnecessary navigation step compared to the previous dashboard home page (dashboard) approach.This aligns well with the PR objectives of optimizing performance and reducing unnecessary operations.
tests/pw/utils/apiUtils.ts (4)
6-7: LGTM! Appropriate imports for file operations.The fs and path imports are necessary for the local caching functionality being implemented.
887-892: LGTM! Clean implementation with proper TypeScript patterns.The method correctly uses array.find() with optional chaining and nullish coalescing to safely check module activation status, defaulting to false when the module isn't found.
920-942: LGTM! Consistent implementation with activateModules.The deactivateModules method follows the same optimization pattern as activateModules, maintaining consistency in the codebase. The logic correctly checks for already inactive modules and avoids redundant API calls.
The same cache staleness considerations mentioned for
activateModulesapply here as well.
895-917: activateModules: cache-first logic verified, no bypass calls detectedThe
rgsearch across.ts/.jsfiles confirmed that all module activation/deactivation operations go through theseapiUtilsmethods—no other code paths modify module state without hitting the same cache-checking logic. Your cache-first optimization stands solid.• No direct
activateModule/deactivateModulecalls found outside ofapiUtils.
• All E2E and API tests invokeactivateModules/deactivateModules, followed by the local refresh viagetAllModules.Recommendation: to guard against rare cache staleness (e.g. external modifications), consider adding a configurable TTL or on-demand cache refresh before the local-check step.
|
|
||
| setup.beforeAll(async () => { | ||
| apiUtils = new ApiUtils(await request.newContext()); | ||
| await dbUtils.setOptionValue(dbData.dokan.optionName.emailVerification, { ...dbData.dokan.emailVerificationSettings, enabled: 'off' }); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify email verification setting doesn't affect other test suites.
The implementation correctly disables email verification for test automation, which is a good practice to avoid complications during user creation.
Please verify that this database option change doesn't persist across different test suites or affect parallel test execution:
🏁 Script executed:
#!/bin/bash
# Description: Check if other test files rely on email verification being enabled
# and verify test isolation
# Search for other references to email verification in tests
rg -A 3 -B 3 "emailVerification|email.*verification" --type ts --type js tests/
# Check if there are test cleanup or reset mechanisms
rg -A 5 "afterAll|teardown|cleanup" tests/pw/tests/e2e/Length of output: 54736
We’ve confirmed that _auth.setup.ts disables email verification in beforeAll but never resets it in afterAll. This will leave email verification off for all subsequent tests that hit the same database.
Action Required: reset email verification in _auth.setup.ts teardown
- In
tests/pw/tests/e2e/_auth.setup.ts, add a call to restore the original settings inafterAll.
Suggested patch:
--- tests/pw/tests/e2e/_auth.setup.ts
@@ setup.afterAll(async () => {
- await apiUtils.dispose();
+ // restore email verification to default
+ await dbUtils.setOptionValue(
+ dbData.dokan.optionName.emailVerification,
+ dbData.dokan.emailVerificationSettings
+ );
+ await apiUtils.dispose();
});This ensures isolation and prevents cross-test pollution.
🤖 Prompt for AI Agents
In tests/pw/tests/e2e/_auth.setup.ts at line 18, the code disables email
verification in beforeAll but does not reset it in afterAll, causing potential
test pollution. To fix this, add an afterAll block that calls
dbUtils.setOptionValue with the original email verification settings to restore
them after tests complete, ensuring test isolation.
| setup('authenticate vendor', { tag: ['@lite'] }, async ({ page }) => { | ||
| const loginPage = new LoginPage(page); | ||
| await loginPage.login(data.vendor, data.auth.vendorAuthFile); | ||
| await page.waitForTimeout(500); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace hard-coded wait with proper element/state waiting.
Hard-coded waitForTimeout(500) is brittle and makes tests unnecessarily slow. This indicates a race condition that should be addressed with proper waiting mechanisms.
Consider replacing with one of these approaches:
- await page.waitForTimeout(500);
+ // Wait for navigation to complete
+ await page.waitForLoadState('networkidle');Or wait for specific elements that need to be ready:
- await page.waitForTimeout(500);
+ // Wait for the element needed by getProductEditNonce
+ await page.waitForSelector('[data-nonce], .wp-admin, #wpadminbar', { state: 'visible' });Or if waiting for URL navigation:
- await page.waitForTimeout(500);
+ // Wait for navigation to vendor dashboard/products page
+ await page.waitForURL('**/wp-admin/**');📝 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.
| await page.waitForTimeout(500); | |
| // Wait for navigation to complete | |
| await page.waitForLoadState('networkidle'); |
| await page.waitForTimeout(500); | |
| // Wait for the element needed by getProductEditNonce | |
| await page.waitForSelector('[data-nonce], .wp-admin, #wpadminbar', { state: 'visible' }); |
| await page.waitForTimeout(500); | |
| // Wait for navigation to vendor dashboard/products page | |
| await page.waitForURL('**/wp-admin/**'); |
🤖 Prompt for AI Agents
In tests/pw/tests/e2e/_auth.setup.ts at line 82, replace the hard-coded
waitForTimeout(500) with a more reliable waiting method such as waiting for a
specific element to appear or a particular state to be reached. Identify the
element or condition that indicates readiness and use page.waitForSelector or
page.waitForFunction accordingly to avoid brittle and slow tests caused by fixed
delays.
1 participant
Summary
Refactors module management functionality to improve performance by implementing local caching and preventing unnecessary API calls.
Changes
getAllModulesto automatically save modules locallycheckIsModuleActiveFromLocalmethod for single module status checkingactivateModulesanddeactivateModulesto check local state before API callsgetAllModuleIdsby removing duplicate file writing logicBenefits
Files Changed
tests/pw/utils/apiUtils.tsSummary by CodeRabbit
Bug Fixes
New Features
Chores