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

added tests #299

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

added tests #299

wants to merge 2 commits into from

Conversation

helga16
Copy link

@helga16 helga16 commented Nov 4, 2024

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.

Thank you for your hard work on this task! 😊 There are a few critical issues that need to be addressed before we can approve the pull request. These include typos in method names like assertPurcaseInfo and assertAllert, which should be corrected to assertPurchaseInfo and assertAlert respectively. Additionally, there's a syntax error in the import statement that needs fixing. Please address these issues to ensure the code functions correctly. Once these changes are made, your solution will be ready for approval! Keep up the great work! 🚀

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

🔄 Fix the issues, commit the changes, and then re-request my review using the Re-request review button in the Reviewers tab.

@@ -1,11 +1,47 @@
import HomeAndCataloguePageObject

Choose a reason for hiding this comment

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

There is a syntax error in the import statement. The correct syntax should be import HomeAndCataloguePageObject from '../support/pages/homeCatalogue.pageObject';.

homePage.clickOnCategory(testData.categoryName);
homePage.clickOnProduct(testData.product);
productPage.addToCart();
productPage.assertAllert(testData.message);

Choose a reason for hiding this comment

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

The method assertAllert seems to have a typo. It should likely be assertAlert to correctly reflect the word 'alert'. Ensure that the method name matches the implementation in the ProdPagePageObject class.

testData.cardMonth,
testData.cardYear
);
cartPage.assertPurcaseInfo(testData.customerName, testData.customerCardNo);

Choose a reason for hiding this comment

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

The method assertPurcaseInfo seems to have a typo. It should likely be assertPurchaseInfo to correctly reflect the word 'purchase'. Ensure that the method name matches the implementation in the CartPageObject class.

class CartPageObject extends PageObject {
assertProductInCart(prodName) {
cy.get('tbody tr td:nth-child(2)')
.should('have.text', prodName + '\n');

Choose a reason for hiding this comment

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

The assertion for the product name in the cart might fail due to the newline character \n. Ensure that the actual text in the DOM includes this newline character, or adjust the assertion to match the exact format of the text.

.click();
}

assertPurcaseInfo(name, cardNo) {

Choose a reason for hiding this comment

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

There is a typo in the method name assertPurcaseInfo. It should be assertPurchaseInfo to correctly reflect the word 'purchase'. Make sure to update any references to this method accordingly.

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 checkout flow using the Page Object Model in Cypress! 🎉 Your code meets the task requirements and constraints, effectively automating the specified flow. However, there are a few critical issues that need attention:

  1. Card Year Range: The range for cardYear is incorrect, allowing years from 1900 to 2012. Adjust this to a more realistic range, such as from the current year onwards, to ensure valid credit card expiration dates.

  2. Alert Handling: Ensure the assertAllert method is correctly implemented to handle alert messages. If it's meant to check for a browser alert, verify that it interacts with the alert properly.

  3. Selector Accuracy: Make sure the selector .sweet-alert p.text-muted correctly targets the element containing the purchase information. If the alert structure changes, this selector might need updating.

  4. Flexible Assertions: Consider using should('include.text', prodName) instead of should('have.text', prodName) for more flexibility, avoiding issues with additional spaces or text around the product name.

These improvements will enhance the robustness and accuracy of your tests. Keep up the great work, and use this feedback to refine your code further. Your dedication to learning and improving is commendable! 🚀

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

customerCity: 'NY',
customerCardNo: faker.finance.creditCardNumber(),
cardMonth: faker.number.int({ min: 1, max: 12 }),
cardYear: faker.number.int({ min: 1900, max: 2012 })

Choose a reason for hiding this comment

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

The range for cardYear seems incorrect. Typically, credit card expiration years should be in the future or recent past, not starting from 1900. Consider adjusting the range to a more realistic set of years, such as from the current year onwards.

homePage.clickOnCategory(testData.categoryName);
homePage.clickOnProduct(testData.product);
productPage.addToCart();
productPage.assertAllert(testData.message);

Choose a reason for hiding this comment

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

Ensure that the assertAllert method is correctly implemented to handle alert messages. If this method is supposed to check for a browser alert, make sure it interacts with the alert correctly.

class CartPageObject extends PageObject {
assertProductInCart(prodName) {
cy.get('tbody tr td:nth-child(2)')
.should('have.text', prodName);

Choose a reason for hiding this comment

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

Consider using a more flexible assertion like should('include.text', prodName) instead of should('have.text', prodName). This change can help avoid issues if there are additional spaces or text around the product name.


assertPurchaseInfo(name) {
cy.get('.sweet-alert p.text-muted')
.should('contain.text', 'Name: ' + name);

Choose a reason for hiding this comment

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

Ensure that the selector .sweet-alert p.text-muted correctly targets the element containing the purchase information. If the structure of the alert changes, this selector might need to be updated.

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