Skip to content
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

Changed end() to return a Promise when called without arguments. #163

Closed

Conversation

jussi-kalliokoski
Copy link

This is a pretty handy thing to have, especially with Mocha, allowing you to go from this (example from supertest's own tests):

it('should support nested requests', function(done){
  var app = express();
  var test = request(app);

  app.get('/', function(req, res){
    res.send('supertest FTW!');
  });

  test
  .get('/')
  .end(function(){
    test
    .get('/')
    .end(function(err, res){
      (err === null).should.be.true;
      res.should.have.status(200);
      res.text.should.equal('supertest FTW!');
      done();
    });
  });
});

to this:

it('should support nested requests', function(){
  var app = express();
  var test = request(app);

  app.get('/', function(req, res){
    res.send('supertest FTW!');
  });

  return test
  .get('/')
  .end()
  .then(function (res) {
    return test
    .get('/')
    .end();
  }).then(function (res) {
    res.should.have.status(200);
    res.text.should.equal('supertest FTW!');
  });
});

Or, in a simpler case, the boilerplate reduction is even more obvious:

it('should do something', function (done) {
  request(app)
  .get('/')
  .expect(200)
  .end(function (err, res) {
    if (err) { return done(err); }
    done();
  });
});

vs

it('should do something', function(){
  return request(app)
  .get('/')
  .expect(200)
  .end();
});

Do note that this depends on ES6 Promises so it works only on node 0.11.x and later, but the es6-promise polyfill can be used for older versions.

@gjohnson
Copy link
Contributor

Let's keep this around until we can get superagent updated to keep them somewhat similar. It needs lots of work though, so might be a while :-/

@jakecraige
Copy link

@gjohnson What's the status of this? This would be a great addition, specifically for use with mocha

@gaastonsr
Copy link

👍

@jclem
Copy link

jclem commented Dec 11, 2014

I desperately needed this and hacked around it with this:

var Bluebird  = require('bluebird'); // Promise lib
var Test      = require('supertest/lib/test');
Test.prototype.end = Bluebird.promisify(Test.prototype.end);

@gaastonsr
Copy link

Exists a library for this for anyone interested.

@avimar
Copy link

avimar commented Mar 19, 2015

Hmm. I've also been using supertest-as-promised, which doesn't even require end() to return a promise, but I don't really know how hacky it is.
My main async calls - stripe and ORM (waterline) provide promises built-in, it would be great if my main API tester supertest also supported promises natively, since my test runner mocha is already able to handle them.

That said, supertest-as-promised seems to function great!

@arikon
Copy link

arikon commented Mar 20, 2015

It is not very hacky — you just need to add then() method to supertest's Test, that will accept callback and errback, that will call end(). And that's it.

@arikon
Copy link

arikon commented Mar 20, 2015

This is an example for Q library:

var q = require('q');

Test.prototype.then = function(onFulfilled, onRejected) {
    var self = this;
    return q.Promise(function(resolve, reject) {
        self.end(function(err, res) {
            if (err) {
                return reject(err);
            }
            resolve(res);
        });
    }).then(onFulfilled, onRejected);
};

With that fix you could use supertest like this (in mocha tests, for example)

var supertest = require('supertest');
it('should be ok', function() {
    return supertest(app).get('/').expect(200);
});

@gaastonsr
Copy link

I can confirm supertest-as-promised is a tiny wrap around supertest.

@JSteunou
Copy link

JSteunou commented Jan 7, 2016

👍 for this, would be great to see it in mainline project even if "promisified" solutions exist.

@r1b
Copy link

r1b commented Jan 20, 2016

+1

@grindfuk
Copy link

bump

@tilfin
Copy link

tilfin commented Mar 9, 2016

+1

@@ -122,6 +122,15 @@ Test.prototype.end = function(fn){
var server = this._server;
var end = Request.prototype.end;

if (!fn) {
return new Promise(function (resolve, reject) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should probably be using any-promise.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Be aware that the .pipe() method of Superagent expects .end() to return the instance itself and not a promise. IE. this breaks .pipe(). I managed to "promisify" .end() like shown below.

var originalEnd = Test.prototype.end;

Test.prototype.end = function (callback) {
  var deferred = Promise.defer();

  originalEnd.call(this, function (err, res) {
    if (callback) {
      callback(err, res);
    }

    if (err) {
      deferred.reject(err);
    } else {
      deferred.resolve(res);
    }
  });

  this.then = deferred.promise.then.bind(deferred.promise);
  this.catch = deferred.promise.catch.bind(deferred.promise);

  return this;
};

@gabzim
Copy link

gabzim commented Jun 6, 2016

+1

2 similar comments
@dzcpy
Copy link

dzcpy commented Jun 27, 2016

+1

@jeonghwan-kim
Copy link

+1

@robinvdvleuten
Copy link

I've opened a new PR for this functionality which uses native promises by default and the .then() method as suggested by @arikon

@rimiti
Copy link
Contributor

rimiti commented Apr 23, 2018

@jussi-kalliokoski can you resolve these conflicts ? Could you please take in consideration the @PlasmaPower and @robinvdvleuten comments.

@mikelax
Copy link
Contributor

mikelax commented May 17, 2018

@rimiti I think this PR can be closed at least in favor of #380
We should separately decide what we need beyond the Promise support already in superagent.

@rimiti rimiti closed this May 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.