-
Notifications
You must be signed in to change notification settings - Fork 357
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
1 #305
base: main
Are you sure you want to change the base?
1 #305
Conversation
There was a problem hiding this 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 project! While your code covers the required functionalities and follows best practices like using the Page Object Model, there are a few critical issues that need to be addressed before we can approve the pull request:
-
Incorrect Calculation: The calculation of
randomNumber
is incorrect. Please update it to useMath.random() * 1000
to ensure it generates a random number correctly. -
Nesting of Test Cases: The nested
it
blocks within anotherit
block need to be restructured. Each test case should be a separateit
block at the top level within thedescribe
block to ensure they run independently. -
Assertions and Verifications: There are several areas where additional assertions are needed to verify actions like user registration, login, logout, article creation, editing, and deletion. These assertions are crucial for ensuring the robustness and reliability of your tests.
-
Faker Library Usage: Ensure that the
faker
library methods you are using are valid for the version you have implemented.
Please address these issues to enhance the functionality and reliability of your tests. Keep up the great work, and don't hesitate to reach out if you need any help! 😊
✨ 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.
cypress.config.js
Outdated
@@ -14,9 +14,10 @@ module.exports = defineConfig({ | |||
generateUser() { | |||
const randomNumber = Math.ceil(Math.random(1000) * 1000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The calculation of randomNumber
is incorrect. The Math.random(1000)
function does not take any arguments. You should use Math.random() * 1000
instead to generate a random number between 0 and 1000.
email: 'test' + `${randomNumber}` + '@mail.com', | ||
password: '12345Qwert!' | ||
password: '12345Qwert!', | ||
bio: faker.person.bio() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure that faker.person.bio()
is a valid method in the version of faker
you are using. If not, you might need to use a different method to generate a bio.
describe('Article', () => { | ||
before(() => { | ||
cy.visit('/'); | ||
cy.register(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure that the cy.register()
command is correctly defined in your Cypress commands. This command should handle user registration, and it's important to verify that it works as expected.
articlePage.typeDescription('My Description test'); | ||
articlePage.typeBody('My Body test'); | ||
articlePage.submitArticle(); | ||
articlePage.assertArticleCreated('My Title test'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding more assertions to verify other aspects of the article creation, such as checking the article's content or tags, to ensure the article is created correctly.
articlePage.typeDescription('My Description new'); | ||
articlePage.typeBody('My Body new'); | ||
articlePage.submitArticle(); | ||
articlePage.assertArticleCreated('My Title new'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the creation test, consider adding more assertions to verify that the article's content has been updated correctly after editing.
} | ||
|
||
typePassword(password) { | ||
this.passwordField.type(password); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider clearing the password field before typing a new password to ensure that no previous input is retained.
} | ||
|
||
typeBio(bio) { | ||
this.bioField.type(bio); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider clearing the bio field before typing a new bio to ensure that no previous input is retained.
this.usernameField | ||
.type(username); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider clearing the username field before typing a new username to ensure that no previous input is retained.
this.emailField | ||
.type(email); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider clearing the email field before typing a new email to ensure that no previous input is retained.
this.passwordField | ||
.type(password); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider clearing the password field before typing a new password to ensure that no previous input is retained.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Summary
Great job on addressing the previous issues and improving the code structure! 🎉 Your implementation now meets the task requirements and constraints. However, there are a few critical issues that need attention to ensure the robustness and functionality of your code:
-
Assertions and Verifications: Ensure that assertions are added after key actions like registration, login, and article deletion to verify their success. This includes checking for error messages or success indicators to confirm the expected outcomes.
-
Method Verifications:
- The
assertFailedSignUp
andassertFailedLogin
methods should accurately verify failed attempts by checking for appropriate error messages. - The
assertProfilePage
method should confirm updates to user details like username, email, bio, and password. - The
assertArticleDeleted
method should ensure the article is no longer present by verifying the absence of unique identifiers or confirmation messages.
- The
-
Random Number Calculation: The calculation of
randomNumber
has been corrected toMath.random() * 1000
, which is now accurate.
Your code is on the right track, and the decision to approve is made to help you move forward with your learning. Please review the comments to further improve your code and address any critical issues in future tasks. Keep up the good work and continue learning from these experiences! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
@@ -12,11 +12,12 @@ module.exports = defineConfig({ | |||
setupNodeEvents(on, config) { | |||
on('task', { | |||
generateUser() { | |||
const randomNumber = Math.ceil(Math.random(1000) * 1000); | |||
const randomNumber = Math.ceil(Math.random() * 1000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The calculation of randomNumber
is incorrect. Using Math.ceil(Math.random() * 1000)
can potentially generate a number outside the desired range. Consider using Math.floor(Math.random() * 1000)
to ensure it generates a random number between 0 and 999.
describe('Article', () => { | ||
before(() => { | ||
cy.visit('/'); | ||
cy.register(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding assertions after the cy.register()
call to verify that the registration was successful. This will help ensure that the test is robust and can catch any issues with the registration process.
signInPage.visit(); | ||
signInPage.typeEmail('[email protected]'); | ||
signInPage.typePassword('12345Qwert!'); | ||
signInPage.clickSignInBtn(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding assertions after the login steps to verify that the user is successfully logged in. This will help ensure the reliability of the login process in your tests.
}); | ||
|
||
it('should be deleted using Delete button', () => { | ||
|
||
articlePage.clickDeleteArticleBtn(); | ||
articlePage.assertArticleDeleted(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure that the assertArticleDeleted
method properly verifies that the article has been deleted. Consider checking for the absence of the article in the list or any confirmation message that indicates successful deletion.
|
||
cy.task('db:clear'); | ||
cy.visit('/'); | ||
cy.register(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding assertions after the cy.register()
call to verify that the registration was successful. This will help ensure that the test is robust and can catch any issues with the registration process.
signUpPage.typeEmail(user.email); | ||
signUpPage.typePassword('test'); | ||
signUpPage.clickSignUpBtn(); | ||
signUpPage.assertFailedSignUp(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure that the assertFailedSignUp
method correctly verifies that the registration attempt failed due to an invalid password. Consider checking for error messages or other indicators of a failed registration.
signUpPage.typePassword(user.password); | ||
signUpPage.clickSignUpBtn(); | ||
cy.visit('#/@riot'); | ||
cy.get('.btn.btn-sm.btn-outline-secondary.action-btn').click(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding an assertion after clicking the follow button to verify that the user is successfully following another user. This could involve checking for a change in the button's text or appearance, or verifying a success message.
assertArticleDeleted() { | ||
this.container | ||
.should('not.contain', 'Delete Article') | ||
.and('not.contain', 'Edit Article').and('contain', '/'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The assertArticleDeleted
method should ensure that the article is no longer present. The current assertion checks for the absence of 'Delete Article' and 'Edit Article' but also checks for the presence of '/'. Make sure this logic aligns with the expected outcome after an article is deleted. Consider verifying the absence of the article title or other unique identifiers.
} | ||
|
||
assertProfilePage(username = 'riot') { | ||
return cy.url().should('contain', `/profile/${username}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The assertProfilePage
method checks if the URL contains /profile/${username}
. Ensure that this logic aligns with the expected URL structure after navigating to the profile page. Consider verifying other elements on the profile page to confirm successful navigation.
|
||
assertFailedSignUp() { | ||
cy.get('.swal-modal') | ||
.should('contain', 'Registration failed!'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The assertFailedSignUp
method checks for a modal containing 'Registration failed!'. Ensure that this message is accurate and matches the actual error message displayed during a failed registration attempt. Consider verifying other elements or messages that indicate a registration failure.
No description provided.