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

WebTables #259

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

WebTables #259

wants to merge 2 commits into from

Conversation

mhyryak
Copy link

@mhyryak mhyryak commented Oct 17, 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.

It looks like there are a few critical issues that need to be addressed before we can approve this pull request. The assertions are not correctly implemented, which could lead to incorrect test results. Please separate the actions and assertions as suggested, and ensure that each value is checked correctly. Keep up the good work, and you'll have this ready in no time! 😊

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

cy.get('#salary').click().type('500');
cy.get('#department').click().type('Legal');
cy.get('#submit').click();
cy.get(':nth-child(4) > .rt-tr > :nth-child(1)').should('contain', username, lastname, email);

Choose a reason for hiding this comment

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

The assertion here is incorrect because cy.get(':nth-child(4) > .rt-tr > :nth-child(1)') is checking for a single element, and should('contain', username, lastname, email) is not a valid syntax for checking multiple values. You should check each value separately or use a different approach to validate the entire row content.

cy.get(':nth-child(4) > .rt-tr > :nth-child(1)').should('contain', username, lastname, email);
});
it('should provide an ability to delete a worker', () => {
cy.get('#delete-record-1').click().should('not.exist');

Choose a reason for hiding this comment

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

The should('not.exist') assertion is chained incorrectly. The click() command does not return a chainable subject for should('not.exist'). You should separate the actions and assertions.

Comment on lines 52 to 54
cy.get('#delete-record-1').click().should('not.exist');
cy.get('#delete-record-2').click().should('not.exist');
cy.get('#delete-record-3').click().should('not.exist');

Choose a reason for hiding this comment

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

Similar to line 49, the should('not.exist') assertion is chained incorrectly after click(). Separate the actions and assertions for each delete operation.

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 the pull request! 🎉 While there are a couple of improvements suggested, such as making the test case more dynamic and using assertions for validation, these are minor and don't impact the core functionality. Keep up the good work and consider these suggestions for future enhancements!

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

Comment on lines +50 to +53
cy.get(':nth-child(4) > .rt-tr > :nth-child(1)').then(($el) => {
console.log($el.html());
console.log($el.text());
});

Choose a reason for hiding this comment

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

It seems like you are logging the HTML and text of the element to the console. If the intention is to validate the data in the worker row, consider using assertions to check that the text matches the expected values instead of just logging it.

Comment on lines +61 to +71
it('should provide an ability to delete all workers', () => {
cy.get('#delete-record-1').click();
cy.get('#delete-record-1').should('not.exist');

cy.get('#delete-record-2').click();
cy.get('#delete-record-2').should('not.exist');

cy.get('#delete-record-3').click();
cy.get('#delete-record-3').should('not.exist');

});

Choose a reason for hiding this comment

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

The test case 'should provide an ability to delete all workers' assumes that there are exactly three workers to delete. If the number of workers can vary, consider using a loop or a more dynamic approach to ensure all workers are deleted.

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.

3 participants