amplify-js icon indicating copy to clipboard operation
amplify-js copied to clipboard

fix(@aws-amplify/api-graphql): tests failing in Node > 14

Open mzarnawski opened this issue 3 years ago • 2 comments

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.

mzarnawski avatar Sep 21 '21 12:09 mzarnawski

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

codecov-commenter avatar Sep 21 '21 12:09 codecov-commenter

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?

dpilch avatar Aug 30 '22 17:08 dpilch

This seems abandoned so I am moving the changes over to another PR.

dpilch avatar Oct 13 '22 15:10 dpilch

This seems abandoned so I am moving the changes over to another PR.

Abandoned by whom ?

mzarnawski avatar Oct 13 '22 16:10 mzarnawski

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.

dpilch avatar Oct 13 '22 17:10 dpilch

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.

mzarnawski avatar Oct 13 '22 18:10 mzarnawski