🧪test-update: fix/auth-setup/authenticate-vendor #2814
Conversation
…nd fixes auth test
…ESS and change its type to boolean
WalkthroughThe changes update Playwright test configurations to explicitly handle the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error Exit handler never called! Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
✨ 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 (
|
dev-shahed
changed the title
dev-shahed
changed the title
dev-shahed
changed the title
dev-shahed
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
tests/pw/tests/e2e/_auth.setup.ts (1)
90-102: Improve nonce extraction robustness.The nonce extraction logic is good, but there are opportunities for improvement:
Consider these enhancements:
let nonce = ''; if (addProductHref) { - const match = addProductHref.match(/_dokan_edit_product_nonce=([a-zA-Z0-9]+)/); + const match = addProductHref.match(/_dokan_edit_product_nonce=([a-fA-F0-9]+)/); if (match && match[1]) { nonce = match[1]; console.log(`✅ Product edit nonce found: ${nonce}`); + } else { + console.log(`⚠️ Nonce pattern not found in URL: ${addProductHref}`); } + } else { + console.log('⚠️ Add product button href is null or empty'); }The regex change assumes nonces are hexadecimal (WordPress nonces typically are), which is more restrictive and accurate than
[a-zA-Z0-9]+.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
tests/pw/e2e.config.ts(1 hunks)tests/pw/pages/selectors.ts(1 hunks)tests/pw/playwright.config.ts(1 hunks)tests/pw/tests/e2e/_auth.setup.ts(2 hunks)tests/pw/types/environment.d.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/pw/tests/e2e/_auth.setup.ts (3)
tests/pw/utils/testData.ts (1)
data(63-2914)tests/pw/pages/selectors.ts (1)
selector(3-8231)tests/pw/utils/helpers.ts (1)
helpers(7-547)
🔇 Additional comments (4)
tests/pw/types/environment.d.ts (1)
59-59: Good type system improvement!Changing
NON_HEADLESSfromstringtobooleanaligns with its actual usage pattern and creates consistency with other boolean environment variables in the interface likeLOCAL,CI, andDOKAN_PRO.tests/pw/tests/e2e/_auth.setup.ts (3)
8-8: LGTM - Clean import change.The import change from
ProductsPagetoselectoraligns well with the refactored approach of using direct selectors instead of page object methods.
103-107: LGTM - Good error handling approach.The try-catch block with fallback nonce handling is well-implemented. Using different fallback values ('fallback_nonce' vs 'error_fallback_nonce') is helpful for debugging.
80-84: Validation Completed: Navigation Timing & URL Consistency
- The
data.subUrls.frontend.vDashboard.productsendpoint is used uniformly across all E2E tests and page-object methods.- Each navigation (
page.goto) is immediately followed byawait page.waitForLoadState('networkidle'), ensuring the dashboard is fully loaded before further actions.- The existing
try { … }wrapper covers unexpected navigation failures—no additional error-handling changes are required at this location.No further action needed.
tests/pw/e2e.config.ts
Outdated
| @@ -81,7 +81,7 @@ export default defineConfig({ | |||
| /* Emulates 'prefers-colors-scheme' media feature, supported values are 'light', 'dark', 'no-preference' */ | |||
| // colorScheme: 'dark' , | |||
| /* Whether to run tests on headless or non-headless mode */ | |||
| headless: !NON_HEADLESS, | |||
| headless: Boolean(NON_HEADLESS), // Because NON_HEADLESS is a string, we convert it to boolean | |||
There was a problem hiding this comment.
Critical logic error: headless behavior is inverted and comment is outdated.
The change from !NON_HEADLESS to Boolean(NON_HEADLESS) inverts the intended behavior:
- Previous behavior: headless = false when NON_HEADLESS is truthy (correct)
- Current behavior: headless = true when NON_HEADLESS is true (incorrect)
The variable name NON_HEADLESS suggests it should enable non-headless mode, but now it directly enables headless mode.
Apply this diff to fix the logic and update the comment:
- headless: Boolean(NON_HEADLESS), // Because NON_HEADLESS is a string, we convert it to boolean
+ headless: !NON_HEADLESS, // NON_HEADLESS=true means run in non-headless mode, so headless should be false📝 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.
| headless: Boolean(NON_HEADLESS), // Because NON_HEADLESS is a string, we convert it to boolean | |
| headless: !NON_HEADLESS, // NON_HEADLESS=true means run in non-headless mode, so headless should be false |
🤖 Prompt for AI Agents
In tests/pw/e2e.config.ts at line 84, the headless flag logic is inverted and
the comment is outdated. Change the assignment to headless:
!Boolean(NON_HEADLESS) to ensure headless mode is false when NON_HEADLESS is
truthy, matching the intended behavior. Also update the comment to clearly
explain that NON_HEADLESS enables non-headless mode, so headless is the negation
of NON_HEADLESS.
tests/pw/playwright.config.ts
Outdated
| @@ -74,7 +74,8 @@ export default defineConfig({ | |||
| /* Emulates 'prefers-colors-scheme' media feature, supported values are 'light', 'dark', 'no-preference' */ | |||
| // colorScheme: 'dark' , | |||
| /* Whether to run tests on headless or non-headless mode */ | |||
| headless: !NON_HEADLESS, | |||
|
|
|||
| headless: Boolean(NON_HEADLESS), // Because NON_HEADLESS is a string, we convert it to boolean | |||
There was a problem hiding this comment.
Fix the headless mode logic - current implementation is incorrect.
The current logic Boolean(NON_HEADLESS) will always return true for any non-empty string, including "false". This means setting NON_HEADLESS="false" would still result in headless=true, which is counterintuitive.
Apply this fix to correctly handle the NON_HEADLESS environment variable:
- headless: Boolean(NON_HEADLESS), // Because NON_HEADLESS is a string, we convert it to boolean
+ headless: !NON_HEADLESS || NON_HEADLESS.toLowerCase() === 'false',Or alternatively, use a more explicit approach:
- headless: Boolean(NON_HEADLESS), // Because NON_HEADLESS is a string, we convert it to boolean
+ headless: NON_HEADLESS !== 'true',🤖 Prompt for AI Agents
In tests/pw/playwright.config.ts at line 78, the headless mode is incorrectly
set using Boolean(NON_HEADLESS), which treats any non-empty string as true. To
fix this, update the logic to explicitly check if NON_HEADLESS equals the string
"true" or "1" (case-insensitive) and set headless to the inverse of that, so
that setting NON_HEADLESS="false" correctly results in headless=true. Replace
the current Boolean conversion with this explicit string comparison to properly
handle the environment variable.
…s a string and adjust product selector paths
…orts in config files
All Submissions:
Changes proposed in this Pull Request:
Related Pull Request(s)
Closes
How to test the changes in this Pull Request:
Changelog entry
Title
Detailed Description of the pull request. What was previous behaviour
and what will be changed in this PR.
Before Changes
Describe the issue before changes with screenshots(s).
After Changes
Describe the issue after changes with screenshot(s).
Feature Video (optional)
Link of detailed video if this PR is for a feature.
PR Self Review Checklist:
FOR PR REVIEWER ONLY:
Summary by CodeRabbit