-
Notifications
You must be signed in to change notification settings - Fork 224
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
update faq and faq_de regarding in-app statistics #3151
Conversation
This fixes: #1497 |
This PR breaks the Cypress test https://github.com/corona-warn-app/cwa-website/blob/master/cypress/integration/faq.js |
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.
Do not merge until Cypress issues have been fixed.
Unfortunately the faq.js test relies on exact text in this FAQ (#further_details
). This is another example of a questionable test strategy, but in the short term the solution is to change the text in https://github.com/corona-warn-app/cwa-website/tree/master/cypress/fixtures/copy
Also, the guidance in https://github.com/corona-warn-app/cwa-website/blob/master/README.md#testing says:
"To minimize the occurrence of errors we would ask you to perform all tests when contributing to our repository."
This step was omitted apparently.
|
Thank you for highlighting the problem and providing a solution. I will review your PR shortly. |
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.
Great, one update of the article covers several updates of the app, thanks a million!
Approved.
Since PR #3153 has been merged, this should no longer break the tests, as tested by @MikeMcC399 and me. |
@MikeMcC399 Thanks for pointing out the cypress issue and reviewing. |
Fixes #1497
The red lines roughly highlight the area of changes
DE:
EN:
Internal Tracking ID: EXPOSUREAPP-8383