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.

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

fix: [botbuilder-core] TestAdapter throws unhandled promise rejection on error #3990

Merged

Conversation

alexrecuenco
Copy link
Contributor

@alexrecuenco alexrecuenco commented Nov 30, 2021

Description

TestAdapter throws unhalded promise rejections on errors

  • The testadapter should handle errors
  • For that, TestFlow must implement the thenable implementation correctly

Specific Changes

  • Implemented further the tenable on TestFlow to allow handling catch clauses correctly

Testing

A test is added to verify the behavior change (If you run that test on main before this PR is merged, it fails)

#minor

@alexrecuenco alexrecuenco requested a review from a team as a code owner November 30, 2021 08:31
@alexrecuenco alexrecuenco force-pushed the test-adapter-unhandled-rejection branch 4 times, most recently from c0fb83b to 447a103 Compare November 30, 2021 08:45
@alexrecuenco
Copy link
Contributor Author

Working around this bug on the testing framework is a bit painful currently, since we can't test that the application recovers correctly after unexpected errors.

@alexrecuenco alexrecuenco changed the title [Botbuilder-core] TestAdapter throws unhandled promise rejection on error [botbuilder-core] TestAdapter throws unhandled promise rejection on error Nov 30, 2021
@alexrecuenco alexrecuenco force-pushed the test-adapter-unhandled-rejection branch from 447a103 to fa5cc81 Compare November 30, 2021 08:50
@alexrecuenco alexrecuenco force-pushed the test-adapter-unhandled-rejection branch 3 times, most recently from b0289af to 73eb9f7 Compare February 26, 2022 07:09
@alexrecuenco
Copy link
Contributor Author

@joshgummersall , I rebased on main and fixed the issues raised. Thank you for your time

@alexrecuenco alexrecuenco force-pushed the test-adapter-unhandled-rejection branch from 73eb9f7 to f1b773d Compare February 26, 2022 07:19
Copy link
Contributor

@mrivera-ms mrivera-ms left a comment

Choose a reason for hiding this comment

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

:shipit:

@mrivera-ms mrivera-ms changed the title [botbuilder-core] TestAdapter throws unhandled promise rejection on error fix: [botbuilder-core] TestAdapter throws unhandled promise rejection on error Mar 4, 2022
@coveralls
Copy link

coveralls commented Mar 4, 2022

Pull Request Test Coverage Report for Build 1931446834

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.0005%) to 84.546%

Totals Coverage Status
Change from base Build 1912781064: 0.0005%
Covered Lines: 19669
Relevant Lines: 22037

💛 - Coveralls

Description

TestAdapter throws unhalded promise rejections on errors

- The testadapter should handle errors
- For that, TestFlow must implement the thenable implementation correctly

Specific Changes

  - Implemented further the tenable on TestFlow to allow handling catch clauses correctly

Testing

A test is added to verify the behavior change (If you run that test on main before this PR is merged, it fails)
@alexrecuenco alexrecuenco force-pushed the test-adapter-unhandled-rejection branch from 3b58c97 to ad397d6 Compare March 4, 2022 01:37
@alexrecuenco
Copy link
Contributor Author

Rebased again and pushed the api change as obtained running yarn test:compat --local

- Ran yarn test:compat --local
@alexrecuenco alexrecuenco force-pushed the test-adapter-unhandled-rejection branch from ad397d6 to 0d3aac2 Compare March 4, 2022 01:55
@mrivera-ms mrivera-ms merged commit 6f3c511 into microsoft:main Mar 9, 2022
@alexrecuenco alexrecuenco deleted the test-adapter-unhandled-rejection branch March 10, 2022 04:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants