-
-
Notifications
You must be signed in to change notification settings - Fork 134
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
Errors in tests appearing for Node.js >= v10.14.2 #130
Comments
These Node.js changes may be related?
|
Just looking at changes in node, this may be related. I think the first thing to do is figure out where these are being logged. I do remember having to wrestle tap at one point to keep it from logging expected errors, so this may be something like an unhandled The actual error that is logged is not surprising to see, as we are aborting the request in those tests. I know that between node 8, 9, and 10, errors would appear in the stream's pieces in different orders, so we already have logic that covers a broad range of timings. My initial suspicion is that node made a very subtle change, and we know about (and handle) an error before we would attach an error handler to a downstream piece that previously wasn't propagated the error, but now is. |
I narrowed down these tests to aborting in a Koa app. This happened because: 1. Koa listens for the error event on the HTTP stream itself, and re-emits it on the app's own event emitter. 2. If there is no listener for an app error, Koa will deviate from the node default of crashing, and will instead log the error. 3. This particular error originates from within the HTTP stream, causing Koa to log the error message. I fixed this by actually _expecting_ one error to be emit from the app, adding an assertion to test this.
Enforce 100% code coverage and improve processRequest internals and tests (including a new test using vanilla Node.js HTTP), fixing #130 .
Enforce 100% code coverage and improve processRequest internals and tests (including a new test using vanilla Node.js HTTP), fixing jaydenseric/graphql-upload#130 .
Something that changed between Node.js v10.14.1 and v10.14.2 has caused errors to log in a few places in tests (which still pass):
https://travis-ci.org/jaydenseric/graphql-upload/jobs/477605581#L570
We need to work out if its a bug in Node.js, or a
graphql-upload
compatibility issue with the new Node.js versions.The text was updated successfully, but these errors were encountered: