failing tests to demonstrate .pipe problems #67

Open
wants to merge 1 commit into
from

Projects

None yet

5 participants

@kirbysayshi

This is a followup PR for #49. The question is: how to fix these based on how people think supertest should behave? Or of course I could be doing something wrong as well :)

There are two issues, as outlined in the comments, reproduced below:

  1. .pipe returns this.end().req.on(...). this of .on is http(s) Request, not supertest, meaning calls to expect, assert, etc fail because they don't exist

  2. .pipe, defined in superagent Request#pipe, calls this.end()... without an fn arg. this.end is actually Test#end, which has no guards for an undefined callback. What should the behavior of this.end with no callback be in supertest when there are errors?

sephalon commented Jul 4, 2013

+1

in node .pipe should always return the stream that's piped to.

Member

Pipe is already implemented correctly in superagent in regards to returning the correct stream.

As for .end() without a callback, were going to leverage it to return a "thunk" in order to make superagent easier to use with generators. (in progress https://github.com/visionmedia/superagent/blob/thunkify/examples/co.js)

@gjohnson gjohnson added the bug label Apr 14, 2014
dmkelly commented Jan 6, 2015

I got around this issue (with the help of @kirbysayshi's comment) with an ugly override of the req.end function for cases where I was piping.

var superagent = require('superagent');
var Request = superagent.Request;

function pipePatch(fn) {
  var self = this;
  var server = this._server;
  var end = Request.prototype.end;

  end.call(this, function(err, res){
    if (err) {
      return fn && fn(err);
    }
    if (server) {
      return server.close(assert);
    }

    assert();

    function assert(){
      if (fn) {
        self.assert(res, fn);
      }
    }
  });

  return this;
}

req.end = pipePatch;
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment