Fix skipped product tests #2457
Conversation
WalkthroughThe changes in this pull request involve significant updates to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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: 2
🧹 Outside diff range and nitpick comments (10)
tests/pw/tests/e2e/products.spec.ts (2)
66-66: Consider adding test.slow() for consistency.While activating this test aligns with the PR objectives, consider adding
test.slow()as subscription product creation typically involves multiple steps and API calls, similar to other subscription-related tests in this file.test('admin can add vendor subscription', { tag: ['@pro', '@admin'] }, async () => { + test.slow(); await admin.addDokanSubscription(data.product.vendorSubscription); });
The skipped test should be activated as it verifies important validation behavior
Looking at the implementation in
productsPage.ts, theaddProductWithoutRequiredFieldsmethod is fully implemented and tests a critical validation flow:
- It verifies that product title is required
- It verifies that product description is required
- It tests the error messages displayed to vendors
Since this test validates required field validation which is an important part of the product creation workflow, and the implementation is ready, this test should be unskipped.
- Update line 147-149 in
tests/pw/tests/e2e/products.spec.tsto remove the.skipmodifier🔗 Analysis chain
Line range hint
147-149: Verify if this test should remain skipped.While the PR objective is to fix skipped tests, this negative test case remains skipped. Please verify if this is intentional or if it should be activated as part of this PR.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if there are any related issues or PRs mentioning this specific test gh issue list --search "vendor can't add product without required fields" gh pr list --search "vendor can't add product without required fields"Length of output: 154
Script:
#!/bin/bash # Let's check the test file content to understand the context of the skipped test rg -B2 -A2 "vendor can't add product without required fields" tests/pw/tests/e2e/products.spec.ts # Also check for any skip/todo markers around this test rg -B2 -A2 "\.skip\(" tests/pw/tests/e2e/products.spec.tsLength of output: 568
Script:
#!/bin/bash # Let's check if the addProductWithoutRequiredFields method is implemented rg -B2 -A2 "addProductWithoutRequiredFields" --type ts # Let's also check if there are any other similar validation tests in the suite rg "required fields" --type tsLength of output: 1571
Script:
#!/bin/bash # Let's check the implementation of addProductWithoutRequiredFields to understand if it's ready for testing cat tests/pw/pages/productsPage.tsLength of output: 85508
tests/pw/tests/e2e/_env.setup.ts (1)
115-117: LGTM with suggestions for improvementThe implementation effectively disables the WooCommerce variable product tour, which is necessary for reliable product test execution. However, consider these improvements for better robustness:
- Use an environment variable or configuration constant for the admin user ID instead of hardcoding '1'
- Add a verification step to confirm the setting was applied successfully
Here's a suggested improvement:
- setup('disable woocommerce variable product tour', { tag: ['@lite'] }, async () => { - await dbUtils.setUserMeta('1','woocommerce_admin_variable_product_tour_shown', 'yes', false); - }); + setup('disable woocommerce variable product tour', { tag: ['@lite'] }, async () => { + const adminUserId = process.env.ADMIN_USER_ID || '1'; + await dbUtils.setUserMeta(adminUserId, 'woocommerce_admin_variable_product_tour_shown', 'yes', false); + + // Verify the setting was applied + const tourShown = await dbUtils.getUserMeta(adminUserId, 'woocommerce_admin_variable_product_tour_shown'); + expect(tourShown).toBe('yes'); + });tests/pw/feature-map/feature-map.yml (1)
87-91: Review security implications of enabled features.While enabling these features fixes skipped tests, please ensure:
- Admin-only features remain properly restricted from vendor access
- Subscription-related features have appropriate payment and validation checks
- Variable product capabilities maintain proper inventory controls
Consider implementing feature toggles in production to gradually roll out these capabilities if needed.
tests/pw/utils/testData.ts (1)
281-281: LGTM! Consider reducing code duplication.The implementation of
variationPricefor bothvariableandvariableSubscriptionproduct types is correct and consistent with other price generation methods in the file.Since both implementations are identical, consider extracting the common price generation logic into a shared helper function to reduce code duplication:
+ const generateVariationPrice = () => faker.finance.amount({ min: 100, max: 200, dec: faker.helpers.arrayElement([1, 2]) }).replace('.', ','); variable: { - variationPrice: () => faker.finance.amount({ min: 100, max: 200, dec: faker.helpers.arrayElement([1, 2]) }).replace('.', ','), + variationPrice: generateVariationPrice, // ... other properties }, variableSubscription: { - variationPrice: () => faker.finance.amount({ min: 100, max: 200, dec: faker.helpers.arrayElement([1, 2]) }).replace('.', ','), + variationPrice: generateVariationPrice, // ... other properties }Also applies to: 346-346
tests/pw/pages/productsPage.ts (4)
Line range hint
116-154: Consider abstracting common code in variable product methodsThe methods
addVariableProduct,addVariableSubscription, and others share similar logic for adding attributes and variations. Refactoring this common functionality into reusable methods can further enhance maintainability.For example, you could create methods like
addProductAttributesandaddProductVariationsto encapsulate this shared logic.
205-223: Repeated code in variation price assignmentThe code for adding variation prices in different methods is similar. Consider creating a method to handle variation price assignment to reduce duplication.
Example:
async addVariationPrice(price: string) { await this.click(productsAdmin.product.addVariationPrice); await this.type(productsAdmin.product.variationPriceInput, price); await this.clickAndWaitForResponse(data.subUrls.ajax, productsAdmin.product.addPrice); }Then, replace the repeated code with:
await this.addVariationPrice(product.variationPrice());
267-268: TODO comment left in codeThere's a
// todo: add commissioncomment left in theaddDokanSubscriptionmethod. Remember to implement this functionality or remove the comment if it's no longer needed.Would you like assistance in implementing the commission feature?
Line range hint
123-144: Inconsistent handling of new and existing attributesThe logic for adding new versus existing attributes in
addVariableProductmay benefit from clearer differentiation or consolidation to handle both cases more uniformly.Consider abstracting attribute addition logic into a single method that handles both new and existing attributes.
tests/pw/pages/selectors.ts (1)
3158-3160: Consider using more semantic names for these selectors.The names
addVariationPrice,variationPriceInput,addPricedon't clearly convey what UI elements they are selecting. More descriptive names likeaddVariationPriceButton,variationPriceInputField,addVariationPriceButtonwould improve readability and maintainability.-addVariationPrice: 'button.add_price_for_variations', +addVariationPriceButton: 'button.add_price_for_variations', -variationPriceInput: 'input.wc_input_variations_price', +variationPriceInputField: 'input.wc_input_variations_price', -addPrice: 'button.add_variations_price_button', +addVariationPriceButton: 'button.add_variations_price_button',
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
tests/pw/feature-map/feature-map.yml(1 hunks)tests/pw/pages/productsPage.ts(7 hunks)tests/pw/pages/selectors.ts(2 hunks)tests/pw/tests/e2e/_env.setup.ts(1 hunks)tests/pw/tests/e2e/products.spec.ts(1 hunks)tests/pw/utils/interfaces.ts(2 hunks)tests/pw/utils/testData.ts(2 hunks)
🔇 Additional comments (11)
tests/pw/tests/e2e/products.spec.ts (2)
57-58: LGTM! Complex test appropriately marked as slow.
The activation of this test is well-structured and the test.slow() addition is justified given the complexity of variable subscription products.
48-49: LGTM! Verify test execution time.
The activation of this test aligns with the PR objectives. The addition of test.slow() is appropriate for variable product creation which typically involves multiple steps.
✅ Verification successful
Based on the codebase analysis, I can now make a determination about the test.slow() usage for the variable product test:
The test.slow() addition is justified for variable product creation.
The codebase shows that variable product creation is more complex than other product types, requiring:
- Multiple steps for setup (attributes, variations)
- Additional API calls and data handling
- Similar tests for variable products also use
test.slow()
Evidence from the codebase:
- Other variable product tests use
test.slow()(e.g., "admin can add variable subscription product") - Variable product creation involves more setup than simple products
- The test involves
admin.addVariableProduct()which handles complex product configuration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the typical execution time of this test to confirm if test.slow() is necessary
npx playwright test tests/pw/tests/e2e/products.spec.ts --grep "admin can add variable product" --reporter=list
Length of output: 207
Script:
#!/bin/bash
# Let's analyze the test structure and its complexity to justify test.slow()
ast-grep --pattern 'test('\''admin can add variable product'\'', $_, async () => { $$$ })'
Length of output: 426
Script:
#!/bin/bash
# Search for other similar product creation tests to compare complexity
rg "test\('.*product" -A 5
Length of output: 138999
tests/pw/tests/e2e/_env.setup.ts (1)
Line range hint 139-152: Verify if these skipped tests should be addressed
While working on fixing skipped product tests, I noticed two skipped tests in the file:
disable germanized settings(lines 139-152)admin set dokan delivery time settings(lines 251-253)
Should these tests be reviewed and potentially un-skipped as part of this PR's objective to fix skipped product tests?
Let's check if these tests are related to the product tests being fixed:
Also applies to: 251-253
tests/pw/utils/interfaces.ts (2)
169-169: LGTM! Method signature added to variable interface.
The addition of variationPrice method with string return type is consistent with other price-related methods in the interface.
Let's verify the usage of this method:
✅ Verification successful
Method signature is correctly implemented and used in the codebase
The variationPrice method is properly implemented and actively used across multiple test files:
- Implemented in test data generation using faker (
testData.ts) - Used in product page interactions (
productsPage.ts) - Referenced in selectors for UI elements (
selectors.ts)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the variationPrice method is being used in test files
# Expect: Usage in test files that work with variable products
rg -A 2 "variationPrice" tests/
Length of output: 2632
234-234: LGTM! Method signature added to variableSubscription interface.
The addition of variationPrice method maintains consistency with the variable interface and follows the same return type pattern.
Let's verify if both interfaces use the method consistently:
tests/pw/feature-map/feature-map.yml (2)
87-89: Verify test coverage for newly enabled product features.
The changes enable admin capabilities for adding variable products and subscription products. This aligns with fixing skipped product tests.
Let's verify the corresponding test files exist:
✅ Verification successful
Test coverage exists for all newly enabled product features
The verification confirms that corresponding test implementations exist in tests/pw/tests/e2e/products.spec.ts for all the enabled features:
- Test for adding variable products
- Test for adding simple subscription products
- Test for adding variable subscription products
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for test files covering the newly enabled product features
# Search for test files related to variable products and subscriptions
echo "Checking for variable product test files..."
fd -e spec.ts -e test.ts | rg -i "variable.*product|subscription.*product"
# Search for specific test cases within test files
echo "Checking for specific test cases..."
rg -A 5 "admin can add variable product|admin can add.*subscription product" "tests/pw/tests"
Length of output: 1545
91-91: Ensure vendor subscription feature is properly integrated.
The addition of vendor subscription capability for admin role should be complemented by appropriate vendor-side permissions.
Let's verify the vendor subscription integration:
✅ Verification successful
Let me check the actual test implementations to ensure proper integration.
Vendor subscription feature is properly integrated and verified
The feature is comprehensively integrated with:
- Complete API endpoints for subscription management (GET, POST, PUT operations)
- Extensive test coverage for both admin and vendor capabilities:
- Admin can manage subscriptions, filter vendors, and assign subscription packs
- Vendors can view, buy, switch, and cancel subscriptions
- Product subscription management for vendors and customers
- Proper permission mappings in feature-map.yml aligning with implemented functionality
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify vendor subscription integration
# Check for vendor subscription related configurations and tests
echo "Checking vendor subscription configurations..."
rg "vendor.*subscription" tests/pw/feature-map/feature-map.yml
# Check for vendor subscription test implementations
echo "Checking vendor subscription test files..."
fd -e spec.ts -e test.ts | rg -i "vendor.*subscription"
Length of output: 1742
Script:
#!/bin/bash
# Check the content of vendor subscription test files
echo "Content of vendorSubscription.spec.ts:"
cat tests/pw/tests/api/vendorSubscription.spec.ts
echo -e "\nContent of vendorProductSubscription.spec.ts:"
cat tests/pw/tests/e2e/vendorProductSubscription.spec.ts
echo -e "\nContent of vendorSubscriptions.spec.ts:"
cat tests/pw/tests/e2e/vendorSubscriptions.spec.ts
Length of output: 15284
tests/pw/pages/productsPage.ts (4)
47-51: Method addProductNameAndType: Implementation looks good
The extraction of product name and type setting into a separate method enhances code reusability and readability.
58-61: Method assignVendorToProduct: Implementation looks good
The method correctly assigns the vendor to the product using the provided vendorName.
63-67: Method publishProduct: Implementation looks good
Encapsulating the product publishing logic in a separate method improves code organization.
Line range hint 74-86: Refactoring addSimpleProduct enhances maintainability
The refactoring to use the new methods (addProductNameAndType, assignCategoryToProduct, assignVendorToProduct) reduces code duplication and improves readability.
| // admin assign category to product | ||
| async assignCategoryToProduct(productName: string) { | ||
| await this.click(productsAdmin.product.category(productName)); | ||
| } |
There was a problem hiding this comment.
Parameter name misalignment in assignCategoryToProduct method
The parameter productName seems to represent a category name rather than a product name. To improve clarity, consider renaming it to categoryName.
Apply this diff to fix the parameter name:
- async assignCategoryToProduct(productName: string) {
+ async assignCategoryToProduct(categoryName: string) {
await this.click(productsAdmin.product.category(categoryName));
}📝 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.
| // admin assign category to product | |
| async assignCategoryToProduct(productName: string) { | |
| await this.click(productsAdmin.product.category(productName)); | |
| } | |
| // admin assign category to product | |
| async assignCategoryToProduct(categoryName: string) { | |
| await this.click(productsAdmin.product.category(categoryName)); | |
| } |
| @@ -3199,6 +3202,7 @@ export const selector = { | |||
| preview: '#post-preview', | |||
| publish: '#publishing-action #publish', | |||
| updatedSuccessMessage: '.updated.notice.notice-success p', | |||
| productPublishSuccessMessage: '//p[contains(.,"Product published. View Product")]', | |||
There was a problem hiding this comment.
💡 Codebase verification
The success message text in the selector doesn't match the test data
The selector expects "Product published. View Product" but the test data defines it as "Product published. " (with a space). This mismatch will cause test failures.
- In
tests/pw/pages/selectors.ts:productPublishSuccessMessage: '//p[contains(.,"Product published. View Product")]' - In
tests/pw/utils/testData.ts:publishSuccessMessage: 'Product published. '
🔗 Analysis chain
Verify the success message text in the codebase.
The success message selector assumes the text "Product published. View Product" will be displayed. However, the actual text may vary.
Run the following script to search for usages of this success message in the codebase and confirm the text is correct:
Let me search for any variations of this success message text to ensure we're using the correct one.
Let me search for any success message definitions in the codebase to verify the correct text.
Let me search more specifically for the success message text in the test data files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the product publish success message text.
# Test: Search for string "Product published. View Product" in JS/TS files.
# Expect: Occurrences in files that set this success message.
rg -t js -t ts "Product published. View Product"
Length of output: 178
Script:
#!/bin/bash
# Search for similar success message patterns related to product publishing
rg -i "product.*published" -g "!{package-lock.json,yarn.lock}"
# Also search for any toast/notification message definitions that might contain this text
rg -i "toast|notification|success.*message" -g "*.{ts,tsx,js,jsx}"
Length of output: 101209
Script:
#!/bin/bash
# Search for success message definitions in test data and utils files
rg -t ts -t js "success.*message|publish.*message" tests/pw/utils/
# Also search for any toast/notification message patterns related to product publishing
rg -t ts -t js "toast|notification.*product.*publish" tests/pw/
Length of output: 201
Script:
#!/bin/bash
# Search for success message text in test data and utils files
rg -t ts -t js "publishSuccessMessage|Product published" tests/pw/utils/testData.ts
# Also check for any message definitions in the page objects
rg -t ts -t js "publishSuccessMessage|Product published" tests/pw/pages/
Length of output: 1041
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
Release Notes
New Features
Bug Fixes
Enhancements