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

test: add and configure cypress tests #410

Closed
wants to merge 5 commits into from
Closed

test: add and configure cypress tests #410

wants to merge 5 commits into from

Conversation

shyambh
Copy link

@shyambh shyambh commented Oct 10, 2022

I have added the Cypress package dependency in the package.json and also updated the tsconfig.json as per the official Cypress documentation:

https://docs.cypress.io/guides/tooling/typescript-support#Configure-tsconfig-json

The tests point to https://eats.seesparkbox.com/refresh for now and can be run in CLI as well as GUI-mode.

The following command will run the tests in CLI :
yarn cypress run

Below is the screenshot of the test run summary :

image

Comment on lines 6 to 11
const restaurantPlacardSelector = 'li.place-card__list-item'
const dropdownSelector = '.location-dropdown__button-text'
const dropdownOptionsSelector = 'li button.location-dropdown__list-button'
const footerSelector = '.footer'
const placeCardCitySelector = '.place-card__city'
const linkOfRestaurantSelector = '.place-card__name-link'
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you familiar with the Cypress practices for query priority?

We should avoid using class selectors like this and try to access the content in the order of 1. accessible queries, 2. semantic queries, and 3. test ids.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks. I will study about this and make the changes accordingly. I was previously unaware of this practice. Thank you for pointing it out. :)

@shyambh
Copy link
Author

shyambh commented Oct 11, 2022

Hi @yosevu , I have made the changes and have added another commit. However, as I am new to contributing, I am unsure if my latest commits will be part of the PR. Can you please confirm.
Otherwise, here are the links to the commits:
995a90d
b0484d5

Copy link
Contributor

@yosevu yosevu left a comment

Choose a reason for hiding this comment

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

@shyambh this is looking good!

There are a few things I would like to comment on, but I think it makes sense to make put the tests in a separate PR so we can get the Cypress dependency added and set up so that other tests can be worked on in parallel. Would you mind creating a separate PR to review these tests?

@yosevu
Copy link
Contributor

yosevu commented Oct 14, 2022

Will you add a test script to run the tests from @shyambh? That way, we can run them with the conventional command yarn test.

@shyambh
Copy link
Author

shyambh commented Oct 14, 2022

Sure, let me apply that as well.

@shyambh
Copy link
Author

shyambh commented Oct 14, 2022

Can you please take a look @yosevu ?

@yosevu
Copy link
Contributor

yosevu commented Oct 14, 2022

The test script looks good @shyambh. I also asked to remove basicTests.cy.ts from this PR and create a separate one because I have some more comments.

Also, will you rebase your branch? If you are unfamiliar with rebasing, please see this article.

@yosevu
Copy link
Contributor

yosevu commented Oct 21, 2022

I separated part of this PR and merged it in #427 @shyambh. Thanks for your help! You may update your branch and open another PR if you want to continue to work on this.

@yosevu yosevu closed this Oct 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants