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.

Sign up for GitHub

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

Jump to bottom

Remove Cypress promise exception handler #9457

Merged
merged 3 commits into from
Sep 28, 2021
Merged

Remove Cypress promise exception handler #9457

merged 3 commits into from
Sep 28, 2021

Conversation

amoore108
Copy link
Contributor

@amoore108 amoore108 commented Sep 27, 2021 •
edited
Loading

Resolves #9178

Overall change:
Removes condition that allowed unhandled promise exceptions to pass Cypress in tests.

Code changes:

  • Removes condition from cypress/support/index.js that allowed unhandled promise exceptions to not fail Cypress tests
  • Small update to the window:before:load listener as the stubbing function syntax used is now deprecated.
  • I have assigned myself to this PR and the corresponding issues
  • I have added the cross-team label to this PR if it requires visibility across World Service teams
  • I have assigned this PR to the Simorgh project
  • (BBC contributors only) This PR follows the repository use guidelines

Testing:

  • Automated (jest and/or cypress) tests added (for new features) or updated (for existing features)
  • If necessary, I have run the local E2E non-smoke tests relevant to my changes (CYPRESS_APP_ENV=local CYPRESS_SMOKE=false yarn test:e2e:interactive)
  • This PR requires manual testing

@amoore108 amoore108 self-assigned this Sep 27, 2021
@amoore108 amoore108 added cross-team For visibility for both World Service teams (Engage & Media) technical-work Technical debt, support work and building new technical tools and features cypress Cypress changes are required labels Sep 27, 2021
@amoore108
Copy link
Contributor Author

amoore108 commented Sep 28, 2021 •
edited
Loading

@jroebu14 I think this what you were after in the linked issue? I tested the removal of that block of code to handle promises in Cypress, and the tests appear to pass as normal. Not sure what has changed, but perhaps all the promises are having their rejections handled correctly and/or the tests for them are also being handled correctly. Can't see anything in the release notes from v7 to v8.2.0 that suggests any changes have been made to revert unhandled exceptions.

jroebu14
jroebu14 approved these changes Sep 28, 2021
View reviewed changes
Copy link
Contributor

@jroebu14 jroebu14 left a comment

Choose a reason for hiding this comment

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

ah brilliant. thanks for picking this one up!

DarioR01
DarioR01 approved these changes Sep 28, 2021
View reviewed changes
Copy link
Contributor

@DarioR01 DarioR01 left a comment

Choose a reason for hiding this comment

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

LGTM

@amoore108 amoore108 merged commit 70179a5 into latest Sep 28, 2021
@amoore108 amoore108 deleted the remove-cypress-promise-expection-handler branch September 28, 2021 11:19
@amoore108 amoore108 mentioned this pull request Sep 29, 2021
Closed
@amoore108 amoore108 mentioned this pull request Oct 7, 2021
Open
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DarioR01 DarioR01 DarioR01 approved these changes

@jroebu14 jroebu14 jroebu14 approved these changes

Assignees

@amoore108 amoore108

Labels
cross-team For visibility for both World Service teams (Engage & Media) cypress Cypress changes are required technical-work Technical debt, support work and building new technical tools and features
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Cypress test failure on FIX for AMP ads tests - unhandled promise rejection
3 participants
@amoore108 @jroebu14 @DarioR01