fix: throw error so test failures are not swallowed #17926
Conversation
|
nice, thank you! I just wrote some today and was very confused why the asserts error out but still pass overall. You saved me A TON of digging :D |
cypress/e2e/signup.cy.ts
Outdated
| @@ -74,7 +74,8 @@ describe('Signup', () => { | |||
| cy.get('.Toastify [data-attr="error-toast"]').contains('Inactive social login session.') | |||
| }) | |||
|
|
|||
| it('Shows redirect notice if redirecting for maintenance', () => { | |||
| // skip this because it seems to be missing necessary setup feature flag, preflight cloud check... | |||
| it.skip('Shows redirect notice if redirecting for maintenance', () => { | |||
There was a problem hiding this comment.
@raquelmsmith can this Cypress test be deleted or should it be fixed?
There was a problem hiding this comment.
It shouldn't be deleted, we want to make sure this functionality works for when we do use the flag in production. It shouldn't be broken and was working before.. The bit we need for the flag is on line 83 below where we define what is in the decide response.
There was a problem hiding this comment.
Just to check... did the test pass for you running locally before?
The problem we've had is that the tests were passing in CI no matter what. I've had to change the code a little...
- I had to set preflight to cloud in the test itself
- the redirect is hard-coded to use prod US or prod EU URLs so have had to change the redirect code to not redirect from Cypress to production
There was a problem hiding this comment.
Yeah it did work locally! Thanks for the fix.
So obvious in retrospect
If you listen to the Cypress fail event without re-throwing then you swallow all test failures
"fun"
When reviewing #17919 I knew the Cypress tests would have to be failing which is what prompted me to check
Introduced in bbb7ed9 (July 10th!)