-
Notifications
You must be signed in to change notification settings - Fork 2
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
[tests][m]- Setup Integration Tests with Cypress #26
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/datopian1/portal/nioyvp6by |
e19c95a
to
b13b846
Compare
8c9be03
to
07bde93
Compare
07bde93
to
2b9f8fd
Compare
1adc150
to
c66e159
Compare
672da21
to
b4ada35
Compare
b4ada35
to
4f7f619
Compare
package.json
Outdated
@@ -36,6 +41,8 @@ | |||
"@types/react": "^16.9.35", | |||
"babel-jest": "^26.0.1", | |||
"babel-plugin-graphql-tag": "^2.5.0", | |||
"cypress": "^4.8.0", | |||
"dotenv": "^8.2.0", |
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.
@lauragift21 do you need dotenv
or is it leftover?
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.
I don't think so I'll take it out.
cy.contains('Find, Share and Publish Quality Data with Datahub'); | ||
}); | ||
it('displays a search form', () => { | ||
cy.get('form').type('dataset').contains('Search').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.
Can we test if it actually works? E.g., it should redirect you to search page with results for your query.
cy.get('form').type('dataset').contains('Search').click(); | ||
}); | ||
it('show the recent datasets', () => { | ||
cy.contains('Recent Datasets'); |
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.
Can we test if it shows expected number of recent datasets?
describe('Test Search Page', () => { | ||
it('has a search form', () => { | ||
cy.visit('/search'); | ||
cy.location('pathname').should('equal', '/search'); |
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.
Not sure if this is valuable here.
cy.visit('/search'); | ||
cy.location('pathname').should('equal', '/search'); | ||
cy.get('form').find('input').type('world population'); | ||
cy.get('form').submit(); |
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.
I think we can have separate case for testing if the search form actually works.. eg, you type in and submit and get some results.
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.
Again, I'd test for expected result when you type "world population":
- query param
- it returns X number of results
@lauragift21 I've added number of comments above. How the CI works? I've seen github actions and Travis.. Why are we using both? |
@anuveyatsu I added GitHub Actions first before seeing you requested doing the setup for Travis CI. We can take GitHub Actions out if it's not necessary. |
@anuveyatsu i think github actions by default atm (and no travis) unless big reason not to, wdyt? |
@rufuspollock @lauragift21 I only mentioned Travis because Cypress had official docs about setting up Travis. If we can use GitHub actions to run Cypress then no problem at all. @lauragift21 is it running with GitHub actions? |
Alright. Yeah it is running with GitHub Actions. I'll suggest we can have both configured it's good to have double checks wdyt? @rufuspollock @anuveyatsu |
@lauragift21 no, we don't want to maintain both of them. Let's remove Travis then. |
}); | ||
it('submits the search form', () => { | ||
cy.get('form').find('[type="text"]').type('my-dataset'); | ||
cy.get('form').submit(); |
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.
To understand if search form works, we need to test what is the result of the form submission. E.g., you typed "my-dataset" and you'd expect to receive X number of datasets listed on the search page. So you'd test:
- it redirects you to search page with query param, eg,
/search?q=my-dataset
- it returns X number of results
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.
Also, I think the form selector is not explicit. It should be specifically searching form elements.
it('shows the recent datasets', () => { | ||
cy.contains('Recent Datasets'); | ||
}); | ||
it('returns the expected number of datasets', () => { |
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.
Did you mean "the expected number of recent datasets"?
cy.get('.recent') | ||
.find('div') | ||
.should(($div) => { | ||
expect($div).to.have.length(3); |
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.
Against which DMS backend are we testing? If against mocks then I'd expect length of 2.
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.
Testing against CKAN API and not the mocks.
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.
@lauragift21 ok, which CKAN API? Where can we set it up?
cy.get('form').find('input').type('world population'); | ||
cy.get('form').submit(); | ||
}); | ||
it('should return a search result', () => {}); |
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.
I see you've booted this test case which is the most important re search page.
@lauragift21 I've added new comments. |
@lauragift21 any updates here? |
@anuveyatsu I'll resolve these comments today. I was out on Friday. |
23a7493
to
02a610b
Compare
[fix][s]- fix typescript bug 'All files must be modules when the '--isolatedModules' flag is provided' by adding an 'export {}' to all test files
02a610b
to
f5ffde7
Compare
cy.url().should('include', '/search?q=my-dataset&sort='); | ||
cy.get('.text-3xl') | ||
.should('be.visible') | ||
.should('have.text', '366 results found'); |
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.
Since you're testing against live CKAN API, this number will change almost every day.
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.
@anuveyatsu This is true and I will check how to make it flexible.
cy.get('form').find('[type="text"]').type('world population'); | ||
cy.get('form').submit(); | ||
cy.url().should('include', 'search?q=world%20population&sort='); | ||
cy.get('.text-3xl').should('have.text', '1 results found'); |
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.
Again, if I go and publish another dataset for that query, this test would fail.
@anuveyatsu Can you review this so that we can merge it tomorrow? |
LGTM. |
fixes:#21