feat: add solution for day 26 #295
Conversation
WalkthroughA new test file has been added to verify the functionality of adding items in a market simulation game using Playwright. The test validates the market page by confirming the title, iterates through table rows to add items with proper error handling for null values and pagination, and finally verifies a successful operation via a dialog handler. A helper function, Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Runner
participant Market as Market Page
participant Table as Food Items Table
participant Dialog as Dialog Handler
Test->>Market: Navigate to market page
Market-->>Test: Display page with title and items table
Test->>Table: Retrieve list of food items
alt Item visible
Table-->>Test: Return item details
Test->>Market: Add item to cart
else Item not visible
Test->>Market: Navigate to next page (max attempts)
end
Test->>Dialog: Click result check button
Dialog-->>Test: Show success message
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Pro Plan! Simply use the command ✨ 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: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
daily-challenges/2024-10/26/hunghoang3110.spec.ts(1 hunks)
🔇 Additional comments (2)
daily-challenges/2024-10/26/hunghoang3110.spec.ts (2)
6-9: Good use of step organization and page verification.The navigation step is well-structured with appropriate page verification. Using
test.stepimproves test readability and debugging.
71-74: Good helper function implementation.The
getCellTexthelper function is well-typed and properly encapsulates the cell text extraction logic.
| await test.step('Check the result', async () => { | ||
| // Set up dialog handler first | ||
| page.on('dialog', dialog => { | ||
| expect(dialog.message()).toBe('Bạn đã đi chợ chính xác!'); | ||
| dialog.accept(); | ||
| }); | ||
| await page.getByRole('button', {name: 'Kiểm tra kết quả'}) | ||
| .click(); | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Verify dialog appears with a timeout.
The code sets up a dialog handler but doesn't verify the dialog actually appears. If the dialog never shows up, the test will pass silently.
await test.step('Check the result', async () => {
// Set up dialog handler first
+ let dialogShown = false;
page.on('dialog', dialog => {
expect(dialog.message()).toBe('Bạn đã đi chợ chính xác!');
+ dialogShown = true;
dialog.accept();
});
await page.getByRole('button', {name: 'Kiểm tra kết quả'})
.click();
+
+ // Wait for dialog to appear with timeout
+ await expect.poll(() => dialogShown, {
+ message: 'Dialog did not appear',
+ timeout: 5000
+ }).toBe(true);
})📝 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 test.step('Check the result', async () => { | |
| // Set up dialog handler first | |
| page.on('dialog', dialog => { | |
| expect(dialog.message()).toBe('Bạn đã đi chợ chính xác!'); | |
| dialog.accept(); | |
| }); | |
| await page.getByRole('button', {name: 'Kiểm tra kết quả'}) | |
| .click(); | |
| }) | |
| await test.step('Check the result', async () => { | |
| // Set up dialog handler first | |
| let dialogShown = false; | |
| page.on('dialog', dialog => { | |
| expect(dialog.message()).toBe('Bạn đã đi chợ chính xác!'); | |
| dialogShown = true; | |
| dialog.accept(); | |
| }); | |
| await page.getByRole('button', {name: 'Kiểm tra kết quả'}) | |
| .click(); | |
| // Wait for dialog to appear with timeout | |
| await expect.poll(() => dialogShown, { | |
| message: 'Dialog did not appear', | |
| timeout: 5000 | |
| }).toBe(true); | |
| }) |
| for (let i = 1; i <= maxAttempts; i++) { | ||
| const isVisible = await page.locator('.product-item') | ||
| .filter({ has: page.getByText(nameOfFood, {exact: true}) }) | ||
| .isVisible(); | ||
| if (isVisible) { | ||
| for (let index = 0; index < Number(quantity); index++) { | ||
| await page.locator('.product-item') | ||
| .filter({ has: page.getByText(nameOfFood) }) | ||
| .getByRole('button', { name: '+' }) | ||
| .click(); | ||
| } | ||
| break; | ||
| } else { | ||
| // Move to the next page if the item cannot be found in the current page | ||
| console.log(`Not found on attempt ${i + 1}`); | ||
| await page.getByRole('button', { name: 'Trang sau' }) | ||
| .click(); | ||
|
|
||
| } | ||
| } | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add handling for items not found after maximum attempts.
The code doesn't handle the case where an item isn't found after the maximum number of attempts. This could lead to silent failures.
for (let i = 1; i <= maxAttempts; i++) {
const isVisible = await page.locator('.product-item')
.filter({ has: page.getByText(nameOfFood, {exact: true}) })
.isVisible();
if (isVisible) {
for (let index = 0; index < Number(quantity); index++) {
await page.locator('.product-item')
.filter({ has: page.getByText(nameOfFood) })
.getByRole('button', { name: '+' })
.click();
}
break;
} else {
// Move to the next page if the item cannot be found in the current page
console.log(`Not found on attempt ${i + 1}`);
await page.getByRole('button', { name: 'Trang sau' })
.click();
}
+ // Add handling for when item is not found after max attempts
+ if (i === maxAttempts) {
+ console.error(`Failed to find item "${nameOfFood}" after ${maxAttempts} attempts`);
+ // Consider whether to fail the test or continue with other items
+ // throw new Error(`Item "${nameOfFood}" not found after ${maxAttempts} page attempts`);
+ }
}📝 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.
| for (let i = 1; i <= maxAttempts; i++) { | |
| const isVisible = await page.locator('.product-item') | |
| .filter({ has: page.getByText(nameOfFood, {exact: true}) }) | |
| .isVisible(); | |
| if (isVisible) { | |
| for (let index = 0; index < Number(quantity); index++) { | |
| await page.locator('.product-item') | |
| .filter({ has: page.getByText(nameOfFood) }) | |
| .getByRole('button', { name: '+' }) | |
| .click(); | |
| } | |
| break; | |
| } else { | |
| // Move to the next page if the item cannot be found in the current page | |
| console.log(`Not found on attempt ${i + 1}`); | |
| await page.getByRole('button', { name: 'Trang sau' }) | |
| .click(); | |
| } | |
| } | |
| }) | |
| for (let i = 1; i <= maxAttempts; i++) { | |
| const isVisible = await page.locator('.product-item') | |
| .filter({ has: page.getByText(nameOfFood, {exact: true}) }) | |
| .isVisible(); | |
| if (isVisible) { | |
| for (let index = 0; index < Number(quantity); index++) { | |
| await page.locator('.product-item') | |
| .filter({ has: page.getByText(nameOfFood) }) | |
| .getByRole('button', { name: '+' }) | |
| .click(); | |
| } | |
| break; | |
| } else { | |
| // Move to the next page if the item cannot be found in the current page | |
| console.log(`Not found on attempt ${i + 1}`); | |
| await page.getByRole('button', { name: 'Trang sau' }) | |
| .click(); | |
| } | |
| // Add handling for when item is not found after max attempts | |
| if (i === maxAttempts) { | |
| console.error(`Failed to find item "${nameOfFood}" after ${maxAttempts} attempts`); | |
| // Consider whether to fail the test or continue with other items | |
| // throw new Error(`Item "${nameOfFood}" not found after ${maxAttempts} page attempts`); | |
| } | |
| } | |
| }) |
| const numberOfRows = await listMarket.getByRole('row').count() - 1; | ||
| for (let index = 1; index <= numberOfRows; index++) { | ||
| const orderRow = listMarket.getByRole('row').nth(index); | ||
| const nameOfFood = await getCellText(orderRow, 0); | ||
| const quantity = await getCellText(orderRow, 1); | ||
| if (nameOfFood === null || quantity === null) { | ||
| throw new Error('Cell text is null'); | ||
| } | ||
| let currentPage = await page.locator('#page-number').textContent(); | ||
| if (currentPage === null) { | ||
| throw new Error('Cell text is null'); | ||
| } | ||
| // First, go back to the first page | ||
| while (Number(currentPage) > 1) { | ||
| await page.getByRole('button', { name: 'Trang trước' }) | ||
| .click(); | ||
| currentPage = await page.locator('#page-number').textContent(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve navigation efficiency by not resetting to page 1 for every item.
The current approach resets to page 1 for each food item, which is inefficient. Consider processing all items on the current page before moving to the next page.
-const numberOfRows = await listMarket.getByRole('row').count() - 1;
-for (let index = 1; index <= numberOfRows; index++) {
- const orderRow = listMarket.getByRole('row').nth(index);
- const nameOfFood = await getCellText(orderRow, 0);
- const quantity = await getCellText(orderRow, 1);
- if (nameOfFood === null || quantity === null) {
- throw new Error('Cell text is null');
- }
- let currentPage = await page.locator('#page-number').textContent();
- if (currentPage === null) {
- throw new Error('Cell text is null');
- }
- // First, go back to the first page
- while (Number(currentPage) > 1) {
- await page.getByRole('button', { name: 'Trang trước' })
- .click();
- currentPage = await page.locator('#page-number').textContent();
- }
+// Extract all food items and quantities from the table
+const foodItems = [];
+const numberOfRows = await listMarket.getByRole('row').count() - 1;
+for (let index = 1; index <= numberOfRows; index++) {
+ const orderRow = listMarket.getByRole('row').nth(index);
+ const nameOfFood = await getCellText(orderRow, 0);
+ const quantity = await getCellText(orderRow, 1);
+ if (nameOfFood === null || quantity === null) {
+ throw new Error('Cell text is null');
+ }
+ foodItems.push({ name: nameOfFood, quantity: Number(quantity) });
+}
+
+// Reset to page 1 once before processing all items
+let currentPage = await page.locator('#page-number').textContent();
+if (currentPage === null) {
+ throw new Error('Page number is null');
+}
+
+while (Number(currentPage) > 1) {
+ await page.getByRole('button', { name: 'Trang trước' })
+ .click();
+ currentPage = await page.locator('#page-number').textContent();
+}
+
+// Now process all food items
+for (const { name: nameOfFood, quantity } of foodItems) {📝 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.
| const numberOfRows = await listMarket.getByRole('row').count() - 1; | |
| for (let index = 1; index <= numberOfRows; index++) { | |
| const orderRow = listMarket.getByRole('row').nth(index); | |
| const nameOfFood = await getCellText(orderRow, 0); | |
| const quantity = await getCellText(orderRow, 1); | |
| if (nameOfFood === null || quantity === null) { | |
| throw new Error('Cell text is null'); | |
| } | |
| let currentPage = await page.locator('#page-number').textContent(); | |
| if (currentPage === null) { | |
| throw new Error('Cell text is null'); | |
| } | |
| // First, go back to the first page | |
| while (Number(currentPage) > 1) { | |
| await page.getByRole('button', { name: 'Trang trước' }) | |
| .click(); | |
| currentPage = await page.locator('#page-number').textContent(); | |
| } | |
| // Extract all food items and quantities from the table | |
| const foodItems = []; | |
| const numberOfRows = await listMarket.getByRole('row').count() - 1; | |
| for (let index = 1; index <= numberOfRows; index++) { | |
| const orderRow = listMarket.getByRole('row').nth(index); | |
| const nameOfFood = await getCellText(orderRow, 0); | |
| const quantity = await getCellText(orderRow, 1); | |
| if (nameOfFood === null || quantity === null) { | |
| throw new Error('Cell text is null'); | |
| } | |
| foodItems.push({ name: nameOfFood, quantity: Number(quantity) }); | |
| } | |
| // Reset to page 1 once before processing all items | |
| let currentPage = await page.locator('#page-number').textContent(); | |
| if (currentPage === null) { | |
| throw new Error('Page number is null'); | |
| } | |
| while (Number(currentPage) > 1) { | |
| await page.getByRole('button', { name: 'Trang trước' }) | |
| .click(); | |
| currentPage = await page.locator('#page-number').textContent(); | |
| } | |
| // Now process all food items | |
| for (const { name: nameOfFood, quantity } of foodItems) { |
| test('Check the Add item function', async ({ page }) => { | ||
| const maxAttempts = 3; | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
LGTM. Consider defining constants closer to their usage.
The test structure is well-organized with a clear title. The maxAttempts constant is defined early but only used much later at line 36, making it harder to understand the connection.
-const maxAttempts = 3;
+// Other code...
+
+await test.step('Add the quantity of item', async () => {
+ const maxAttempts = 3;Committable suggestion skipped: line range outside the PR's diff.
Summary by CodeRabbit