Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add manager tests #4

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

DianaKhala
Copy link

No description provided.

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job on implementing the Playwright test project for Wizard Bank! 🎉 Your code effectively meets the task requirements and constraints, covering various test scenarios for bank manager functionalities. While there are some minor issues related to assumptions about table row order, these do not critically impact the functionality. Keep up the good work, and consider refining these areas for even more robust tests in the future. Happy coding! 😊

✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨

Comment on lines +44 to +45
async assertCustomersTableContainsFirstName(value) {
await expect(this.lastRow).toContainText(value);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The method assertCustomersTableContainsFirstName assumes that the last row in the table is the newly added customer. This might not be reliable if the table is sorted or updated dynamically. Consider searching for the specific row containing the expected first name.

Comment on lines +48 to +49
async assertCustomersTableContainsLastName(value) {
await expect(this.lastRow).toContainText(value);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to the previous comment, assertCustomersTableContainsLastName assumes the last row is the newly added customer. Ensure that the method accurately identifies the correct row.

Comment on lines +52 to +53
async assertCustomersTableContainsPostalCode(value) {
await expect(this.lastRow).toContainText(value);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The method assertCustomersTableContainsPostalCode also relies on the last row assumption. Consider implementing a more robust way to verify the postal code in the table.

Comment on lines +6 to +7
this.lastRow = page.getByRole('row').last();
this.accountNumberCell = this.lastRow.getByRole('cell').nth(3);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The use of lastRow to access the account number cell assumes that the last row is always relevant for the operation. This might not be reliable if the table is sorted or updated dynamically. Consider searching for the specific row containing the expected account number.

Comment on lines +10 to +15
this.firstRow = page.getByRole('row').nth(1);
this.firstNameCell = this.firstRow.getByRole('cell').nth(0);

this.lastNameCell = this.firstRow.getByRole('cell').nth(1);

this.postCodeCell = this.firstRow.getByRole('cell').nth(2);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The use of firstRow to access the first name, last name, and post code cells assumes that the first row is always the correct one to check. This might not be reliable if the table is sorted or filtered. Consider implementing a more robust way to verify the customer details.

Comment on lines +11 to +12
this.lastRow = page.getByRole('row').last();
this.accountNumberCell = this.lastRow.getByRole('cell').nth(3);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The use of lastRow to access the account number cell assumes that the last row is always relevant for the operation. This might not be reliable if the table is sorted or updated dynamically. Consider searching for the specific row containing the expected account number.

Comment on lines +43 to +45
await addCustomerPage.assertCustomersTableContainsFirstName(firstName);
await addCustomerPage.assertCustomersTableContainsLastName(lastName);
await addCustomerPage.assertCustomersTableContainsPostalCode(postCode);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The assertions for checking the first name, last name, and postal code rely on the last row being the newly added customer. This might not be reliable if the table is sorted or updated dynamically. Consider implementing a more robust way to verify the customer details.

@@ -22,6 +35,12 @@ Test:
4. Reload the page.
5. Assert customer row is not present in the table.
*/
const customersListPage = new CustomersListPage(page);
const fullName = firstName + ' ' + lastName + ' ' + postCode;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fullName variable includes the postal code, which might not match the row's name attribute if it only contains the first and last name. Verify that the format of fullName matches the format used in the customer table for the row's name attribute.

Comment on lines +59 to +61
const fullName = firstName + ' ' + lastName + ' ' + postCode;

await customersListPage.assertCustomerRowHasAccountNumber(fullName);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fullName variable includes the postal code, which might not match the row's name attribute if it only contains the first and last name. Verify that the format of fullName matches the format used in the customer table for the row's name attribute.

Comment on lines +24 to +26
firstName = faker.person.firstName();
lastName = faker.person.lastName();
postalCode = faker.location.zipCode();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The variables firstName, lastName, and postalCode are initialized twice. The second initialization is redundant and should be removed to clean up the code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants