-
Notifications
You must be signed in to change notification settings - Fork 903
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
Adds some Cypress integration tests #4157
Conversation
Kongkille
commented
May 19, 2022
- Remove test for countryPanel. It can potentially fail for other reasons than what we are testing for.
- Add history and state API tests to make sure the app requests the correct data succesfully
fail for other reasons than what we are testing for.
the app requests the correct data succesfully
it('Asserts visiting zone requests history data succesfully', () => { | ||
cy.intercept('https://app-backend.electricitymap.org/v4/history?countryCode=FR').as('fetchHistory'); | ||
cy.visit('/zone/FR?remote=true'); | ||
cy.wait('@fetchHistory').its('response.statusCode').should('eq', 200); |
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.
Should you also do a check for what is returned? Or are you just looking for a 200 response?
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.
Just looking for the 200 response, as we can't be certain that there is data and I would rather have separate tests for the app-backend
@@ -0,0 +1,7 @@ | |||
describe('History', () => { |
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.
In the longer term, should this testing go into the app backend testing directly? Since here we are testing the output of something that we should trust (the app should assume that the API works and is returning things correctly).
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 idea is to have both types of testing. This should not be considered as our main API tests, it just verified that the app is able to query the backend correctly :)
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.
Is the way the app queries the data different from a regular API call? I'm trying to understand the difference and why this test is needed on top of the API tests.
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.
Yes, well the app queries the app-backend which again queries our standard API, so there's a middlestep which is the app-backend.
Ideally we want a bunch of unit tests for the app and the app-backend but now I am just testing the integration step, i.e. whether the app and the app-backend is able to communicate properly
So to put some words on these tests, it essentially tests that we're able to spin up the app and create authorized requests to the production-app-backend based on user actions, which may seem simple, but is a super nice sanity check for us before merging things
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.
Good to know, makes sense!
@@ -1,9 +1,4 @@ | |||
describe('Country Panel', () => { | |||
it('asserts countryPanel contains carbon intensity when available', () => { |
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.
Maybe worth adding a TODO here so we don't forget to reintroduce a test later
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.
Hmm I don't think that's necessary I just added the test initially yesterday, and I am not sure we'd want to handle the test like this
it('Asserts visiting map requests state data correctly', () => { | ||
cy.visit('/map?remote=true'); | ||
cy.intercept('https://app-backend.electricitymap.org/v4/state').as('getState'); | ||
cy.wait('@getState').its('response.statusCode').should('eq', 200); |
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.
Same comment as above—only for a 200 test or should we also test output?
…electricitymap-contrib into mk/add-integration-tests