-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[AppServices] Fix unhandled promise rejections in search tests #112849
[AppServices] Fix unhandled promise rejections in search tests #112849
Conversation
Pinging @elastic/kibana-app-services (Team:AppServices) |
@elasticmachine merge upstream |
@elasticmachine merge upstream |
await searchSource | ||
.fetch$(options) | ||
.pipe(catchError((e) => e)) |
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.
I played around with this code for a bit hoping to find a solution that was a bit more direct. Why do you need this line when catch
is called below? I approve of the PR but wish I understood this a bit better.
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.
I think the problem in this test is actually a few lines up above:
searchSourceDependencies.search = jest.fn().mockReturnValue(of(Promise.reject('aaaaa')));
The mock search
function will throw an uncaught error as soon as fetch$
is called (rather than when it's subscribed to, as the real search
function would). In fact, if you keep this line as-is and get rid of the rest of this test, then run the test with the flag, you'll still get the unhandled rejection.
Instead, I think we want to change this line to the following:
searchSourceDependencies.search = jest.fn().mockReturnValue(throwError('aaaaa'));
If we make this change, the rest of the test can remain unchanged and we won't see the given error.
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.
Thanks for taking a closer look @mattkime , I was also a bit confused! And thanks for the explanation @lukasolson ! I'll update the PR.
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.
nice work @lukasolson !
@elasticmachine merge upstream |
💚 Build SucceededMetrics [docs]
History
To update your PR or re-run it, just comment with: |
…ic#112849) * updated jest tests to avoid generating unhandled promise rejections * rather use throwError * remove unused imports Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> # Conflicts: # src/plugins/data/common/search/search_source/search_source.test.ts
…112849) (#113264) * [AppServices] Fix unhandled promise rejections in search tests (#112849) * updated jest tests to avoid generating unhandled promise rejections * rather use throwError * remove unused imports Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> # Conflicts: # src/plugins/data/common/search/search_source/search_source.test.ts * Fix lint issues
Summary
Partially addresses #112699
How to check
Run the respective files with:
Notes
Added
catchError
to search source test, not sure this is the correct way to ensure that this rejection gets handled?