Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

superagent `pipe` is broken when using supertest #49

Open
kirbysayshi opened this Issue Feb 19, 2013 · 8 comments

Comments

Projects
None yet
7 participants

I have a fix, but need an opinion before submitting.

Doing something like:

supertest.get('/path/to/file.mp3')
  .buffer(false)
  .pipe(new _Speaker()) 
  .end(function(err, res){
    console.log('all done');
  })

Fails with:

/Users/drewp/dev/link-pipes/node_modules/supertest/lib/test.js:197
  fn(null, res);
  ^
TypeError: undefined is not a function
    at Test.assert (/Users/drewp/dev/link-pipes/node_modules/supertest/lib/test.js:197:3)
    at Test.end (/Users/drewp/dev/link-pipes/node_modules/supertest/lib/test.js:126:10)
    at Test.Request.callback (/Users/drewp/dev/link-pipes/node_modules/supertest/node_modules/superagent/lib/node/index.js:575:3)
    at Test.<anonymous> (/Users/drewp/dev/link-pipes/node_modules/supertest/node_modules/superagent/lib/node/index.js:133:10)
    at Test.EventEmitter.emit (events.js:96:17)
    at ClientRequest.Request.end (/Users/drewp/dev/link-pipes/node_modules/supertest/node_modules/superagent/lib/node/index.js:693:12)
    at ClientRequest.EventEmitter.emit (events.js:126:20)
    at HTTPParser.parserOnIncomingClient [as onIncoming] (http.js:1569:7)
    at HTTPParser.parserOnHeadersComplete [as onHeadersComplete] (http.js:111:23)
    at Socket.socketOnData [as ondata] (http.js:1472:20)

If I replace this.end() in line 384 of superagent/lib/node/index.js with this.end(noop), everything works as expected.

I've tracked this down but am not sure the best way to fix it. The issue is due to inheritance.

Request.pipe calls this.end()... (no args). Request.end checks for a passed fn or defaults to noop. But end is redefined in supertest to blindly call this.assert(res, fn), which always assumes fn is defined. So this.end in Request.pipe is actually calling Test.end.

I could fix this by editing either supertest or superagent, but I'm not sure of the intended behavior. If I were to change supertest so that assert's fn param was optional, that would fix all of this, but then assert wouldn't be relaying the results anywhere. Changing this is superagent requires all calls to this.end to always pass noop, which feels weird considering this only happens because supertest subclasses Request.

So perhaps the best thing to do is allow fn to be optional in Test.assert?

timoxley commented May 6, 2013

+1 why don't you send a PR, or at least a PR with a failing test.

+1

kirbysayshi added a commit to kirbysayshi/supertest that referenced this issue Jun 29, 2013

luka5 commented Jul 2, 2013

+1

@timoxley, and others with +1s, please see my PR #67 and give me some feedback on how to approach fixing this.

@gjohnson gjohnson added the question label Apr 14, 2014

Any updates on this?

Nope. I haven't looked at supertest since I started this... is it still an issue?

npkumar commented Oct 20, 2016

+1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment