feature: add solution on daily 10/26 KT #277
Conversation
WalkthroughThe pull request introduces a Playwright test specification for a product purchasing functionality. The code defines an interface Changes
Sequence DiagramsequenceDiagram
participant Test as Test Case
participant Page as Playwright Page
participant BuyProduct as buyProduct Function
Test->>Page: Navigate to URL
Test->>Page: Extract product list
Test->>BuyProduct: Call with product list and page
loop For each product
BuyProduct->>Page: Check product visibility
alt Product visible
BuyProduct->>Page: Click purchase button
else Product not visible
BuyProduct->>Page: Add to secondary list
BuyProduct->>Page: Click pagination
BuyProduct->>BuyProduct: Recursive call
end
end
Test->>Page: Handle dialog events
Test->>Page: Verify purchase result
Possibly related PRs
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: 8
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
daily-challenges/2024-10/26/khanh5923_26.spec.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
daily-challenges/2024-10/26/khanh5923_26.spec.ts
[error] 11-11: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 7-7: This let declares a variable that is only assigned once.
'productSecondtList' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 33-33: This let declares a variable that is only assigned once.
'productsList' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
| for (let i = 1; i <= Number(product.qnt); i++) { | ||
| await page.waitForTimeout(10); | ||
| await page.locator(`//div[contains(text(),'${product.name}')]/following-sibling::div//button[2]`).click(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize the quantity-based click loop
Regarding your concern about the for loop, there are several ways to improve this:
- Use Playwright's built-in
click({ clickCount: n })instead of multiple clicks - Consider using
evaluate()to directly set the quantity value if there's an input field
// Option 1: Use clickCount
await page.locator(`//div[contains(text(),'${product.name}')]/following-sibling::div//button[2]`)
.click({ clickCount: Number(product.qnt) });
// Option 2: If there's an input field, you could try:
await page.evaluate((selector, value) => {
const input = document.querySelector(selector);
if (input) {
Object.defineProperty(input, 'readonly', { value: false });
input.value = value;
}
}, 'quantity-input-selector', product.qnt);| try { | ||
| await expect(page.locator(`//div[contains(text(),'${product.name}')]`)).toHaveText(product.name!); | ||
| visible = true; | ||
| } catch { | ||
| productSecondtList.push(product); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and selector reliability
The current error handling silently catches all errors and the XPath selectors are brittle.
- Use more reliable selectors (data-testid, role, etc.)
- Add specific error handling
- Add logging for debugging
try {
const productElement = page.getByTestId(`product-${product.name}`);
await expect(productElement).toBeVisible({ timeout: 5000 });
visible = true;
} catch (error) {
console.log(`Product ${product.name} not visible on current page: ${error.message}`);
productSecondList.push(product);
}🧰 Tools
🪛 Biome (1.9.4)
[error] 11-11: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
| for (let i = 1; i <= 5; i++) { | ||
| productsList.push({ | ||
| name: await page.locator(`//tbody[@id="wishlist"]//tr[${i}]//td[1]`).textContent(), | ||
| qnt: await page.locator(`//tbody[@id="wishlist"]//tr[${i}]//td[2]`).textContent(), | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove hardcoded product count
The hardcoded loop count of 5 makes the test brittle. Instead, get the actual row count dynamically.
const rows = await page.locator('//tbody[@id="wishlist"]//tr').count();
for (let i = 1; i <= rows; i++) {
// ... rest of the code
}| page.on("dialog", async (dialog) => { | ||
| const messageResult = dialog.message(); | ||
| await dialog.accept(); | ||
| expect(messageResult).toContain('Bạn đã đi chợ chính xác!'); | ||
| }); | ||
| await page.click(`//button[@class='check-result']`); |
There was a problem hiding this comment.
Move dialog handler setup before triggering actions
The dialog handler should be set up before any actions that might trigger dialogs to ensure no dialogs are missed.
Move this block before the buyProduct call:
// Move this before line 40
page.on("dialog", async (dialog) => {
const messageResult = dialog.message();
await dialog.accept();
expect(messageResult).toContain('Bạn đã đi chợ chính xác!');
});| import { test, expect, Page } from "@playwright/test"; | ||
| interface Product { | ||
| name: string | null; | ||
| qnt: string | null; | ||
| } | ||
| async function buyProduct(productsList: Product[], page: Page): Promise<void> { | ||
| let productSecondtList:Product[] = []; | ||
| for (const product of productsList) { | ||
| let visible = false; | ||
| try { | ||
| await expect(page.locator(`//div[contains(text(),'${product.name}')]`)).toHaveText(product.name!); | ||
| visible = true; | ||
| } catch { | ||
| productSecondtList.push(product); | ||
| } | ||
| if (visible) { | ||
| for (let i = 1; i <= Number(product.qnt); i++) { | ||
| await page.waitForTimeout(10); | ||
| await page.locator(`//div[contains(text(),'${product.name}')]/following-sibling::div//button[2]`).click(); | ||
| } | ||
| } | ||
| } | ||
| if (productSecondtList.length > 0) { | ||
| await page.click(`//div[@class="pagination"]//button[2]`); | ||
| await buyProduct(productSecondtList, page); | ||
| } | ||
| else return; | ||
| } | ||
|
|
||
| test.setTimeout(60000); | ||
| test("khanh5923_26", async ({ page }) => { | ||
| await page.goto(`https://material.playwrightvn.com/games/001-di-cho.html`); | ||
| let productsList:Product[] = []; | ||
| for (let i = 1; i <= 5; i++) { | ||
| productsList.push({ | ||
| name: await page.locator(`//tbody[@id="wishlist"]//tr[${i}]//td[1]`).textContent(), | ||
| qnt: await page.locator(`//tbody[@id="wishlist"]//tr[${i}]//td[2]`).textContent(), | ||
| }); | ||
| } | ||
| await buyProduct(productsList, page); | ||
| page.on("dialog", async (dialog) => { | ||
| const messageResult = dialog.message(); | ||
| await dialog.accept(); | ||
| expect(messageResult).toContain('Bạn đã đi chợ chính xác!'); | ||
| }); | ||
| await page.click(`//button[@class='check-result']`); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider implementing Page Object Model pattern
To improve maintainability and reusability, consider:
- Implementing the Page Object Model pattern
- Moving selectors to a separate configuration file
- Creating reusable test utilities for common operations
Would you like me to provide an example of how to restructure this using the Page Object Model pattern?
🧰 Tools
🪛 Biome (1.9.4)
[error] 11-11: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 7-7: This let declares a variable that is only assigned once.
'productSecondtList' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 33-33: This let declares a variable that is only assigned once.
'productsList' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
| interface Product { | ||
| name: string | null; | ||
| qnt: string | null; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider strengthening the type definitions
The nullable fields in the Product interface could lead to runtime errors. Consider making these fields required since they appear to be essential for the functionality.
interface Product {
- name: string | null;
- qnt: string | null;
+ name: string;
+ qnt: string;
}📝 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.
| interface Product { | |
| name: string | null; | |
| qnt: string | null; | |
| } | |
| interface Product { | |
| name: string; | |
| qnt: string; | |
| } |
| qnt: string | null; | ||
| } | ||
| async function buyProduct(productsList: Product[], page: Page): Promise<void> { | ||
| let productSecondtList:Product[] = []; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Fix typo in variable name
The variable name contains a typo: productSecondtList should be productSecondList.
- let productSecondtList:Product[] = [];
+ const productSecondList: Product[] = [];📝 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.
| let productSecondtList:Product[] = []; | |
| const productSecondList: Product[] = []; |
🧰 Tools
🪛 Biome (1.9.4)
[error] 7-7: This let declares a variable that is only assigned once.
'productSecondtList' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
| } | ||
| if (visible) { | ||
| for (let i = 1; i <= Number(product.qnt); i++) { | ||
| await page.waitForTimeout(10); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove hardcoded timeout
Hardcoded timeouts are unreliable and can make tests flaky. Instead, wait for specific conditions.
- await page.waitForTimeout(10);
+ await page.waitForSelector(`//div[contains(text(),'${product.name}')]/following-sibling::div//button[2]`, { state: 'attached' });📝 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(10); | |
| await page.waitForSelector(`//div[contains(text(),'${product.name}')]/following-sibling::div//button[2]`, { state: 'attached' }); |
Hi, mọi người xem giúp em bài này nhé. Vòng lặp for ở dòng 17 em thấy nó không được hay lắm, em có thử dùng evaluate() để bỏ thuộc tính "readonly" rồi điền giá trị vào mà không được :)))))
----------- GÓC HỎI NHỎ :)))))) -----------
hú anh @minhphong306 và mọi người, em định làm một project cá nhân nhỏ để bỏ vô CV cho hay, thì em tính viết một bộ testcase như manual test í rồi từ mấy testcase đó viết script cho automation. Thì hong biết em cần thêm gì cho hay hơn không anh nhỉ, mọi người với anh cho em thêm ý kiến nhé. Em cảm ơn ạ.
Summary by CodeRabbit