Improve code coverage #162
Conversation
The point of keeping `currentStream` is to allow us to destroy it when another terminating condition arises. These lines were here to prevent a terminating event _arising from within the stream_ from triggering an unnecessary call to `.destroy`. These are no longer necessary because: 1. The way busboy works, it's impossible for a second stream to be created before the first has ended. 2. The busboy stream itself is no longer passed to the user, eliminating the possibility of user code calling `.destroy(err)` on the stream.
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.
|
graphql-upload/src/processRequest.mjs Line 137 in e98b7c1 This line is used in node |
|
graphql-upload/src/processRequest.mjs Line 311 in e98b7c1 This line could be changed to simply use we get the error created by dicer: This is conceptually a "secondary" error, but is the one that propagates. For this reason, we will use We could potentially remove the |
|
Hmmm 🤔 In situations when certain code runs for particular Node.js versions, it's probably best to add an Istanbul ignore comment before it: // istanbul ignore nextBabel has been configured to preserve such comments: graphql-upload/babel.config.js Line 19 in e98b7c1 |
I added a test that uses `processRequest` directly with node's HTTP library. In addition to being a valuable test, it adds code coverage of the option defaults.
|
I didn't know about I added another test in a341dc1 which uses |
This fixes code coverage inconsistencies accross supported Node.js versions.
This removes two lines that were impossible to reach given the current state of this library. In the process, I also fixed #130. See the commit messages for details.