amplify-js
amplify-js copied to clipboard
fix(@aws-amplify/api-graphql): tests failing in Node > 14
Description of changes
One test fix for GraphQL package when run in Node > 14. Looks like there is an error throw that is overwritten by another one in the code. Maybe needs some other handling ?
try { ... if (!token) { throw new Error(GraphQLAuthError.NO_FEDERATED_JWT); } ... } catch (e) { throw new Error(GraphQLAuthError.NO_CURRENT_USER); }
Regarding Node > 14 there are still some errors in ui-components but I am not able to fix them due to lack of skills.
Description of how you validated changes
Ran tests in api-graphql scope
Checklist
- [x] PR description included
- [x]
yarn test
passes (in api-graphql scope) - [x] Tests are changed
- [ ] Relevant documentation is changed or added (and PR referenced)
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Codecov Report
Merging #8927 (02a73f0) into main (f28918b) will not change coverage. The diff coverage is
n/a
.
@@ Coverage Diff @@
## main #8927 +/- ##
=======================================
Coverage 84.35% 84.35%
=======================================
Files 256 256
Lines 18582 18582
Branches 3986 3986
=======================================
Hits 15674 15674
Misses 2818 2818
Partials 90 90
:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more
Interesting find! Thank you for pointing this out. I think the true problem here is that expect.rejects
does not have await
. Starting with node 15 unhandled promise rejections throw an error instead of a warning.
You can verify this by placing any string in the toThrowError
call and see that the test always passes with node <= 14. You will see the warning message:
(node:15559) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block ...
Then add an await
in front of expect
and see the test fail.
This must have been a mistake when the test was first written and never should have passed. Do you mind including the await
with your PR so that the test works correctly for node <= 14 as well?
This seems abandoned so I am moving the changes over to another PR.
This seems abandoned so I am moving the changes over to another PR.
Abandoned by whom ?
Please see my previous comment. If you are willing to make these changes I can close my PR (https://github.com/aws-amplify/amplify-js/pull/10470) and approve this one.
Leave it like it is. I'm just surprised that it waited for a review almost one year and you expect me to react although it has been just a month and a half since your last comment. I need to take my time as well. Let's just respect each other.