Can't test expected error responses because superagent throws

[motiz88]

Consider the following Mocha test (written in ES7 async/await style which is syntactic sugar for Promises):

describe('Querying a nonexistent table', function() {
  it('causes a 404', async function() {
    const res = await chai.request(app).get('/faketable');
    res.should.have.status(404);
  });
});

If the endpoint returns a 404, the response promise gets rejected with an Error originating in superagent's response handler, and the test fails before reaching the status assertion. Other 4xx/5xx status codes similarly cannot be tested for in this way. I think this is surprising behavior that makes testing error responses much harder than it should be.

[keithamus]

Hey @motiz88 thanks for the issue.

It's a great point - but I think SuperTest is doing the right thing here, I also think Chai-HTTP is doing the right thing here. I think the problem might lie with Chai; any Promise which rejects will throw in async, and not every rejecting Promise is catastrophic enough for a test to fail.

Do you fancy raising this issue upstream in chai? We can look at potential solutions there.

[motiz88]

I very respectfully disagree :)

Promise rejection is to async control flow as throwing exceptions is to sync control flow. The same way any exception will by default fail a test (unless guarded against, wrapped in a should.throw block, etc), any rejected promise (that's returned / awaited upon) should most definitely fail a test in any sane async-capable framework. This is a matter of JS semantics, not style.

So the issue at hand is that Chai-HTTP is misusing promise rejection to force its opinion as to correct handling of HTTP status codes, in a way that makes writing a certain kind of useful test case more difficult.

Is there any value to baking this opinion into Chai-HTTP over letting test authors plug in their own domain-specific, test-specific logic?

After all, that's what they'll be doing anyway. I mean, can a test author get away with not specifying the result of their request? I doubt you'll find a lot of tests like this:

it('works', function() {
    return chai.request(app).get('/');
});

But rather more like this:

it('works', function() {
    return chai.request(app).get('/')
    .should.eventually.have.status(200); // With chai-as-promised
});

---

A more correct Promise-based API, for me, would be in line with the W3C Fetch API, where fetch() always resolves to a Response object, which the app is then free to inspect and translate to a promise rejection in userland, like so:

fetch(url)
.then(res => {
  if (res.status >= 200 && res.status < 300) {
    return res;
  }
  throw new Error(`${response.status} ${response.statusText}`);
})
.then(res => ....);

[motiz88]

More concisely, Chai-HTTP is basically doing my asserts for me, it's doing them wrong (because it doesn't know what I'm testing for) and I can't opt out of this behavior. You've got to see why I consider this broken.

[keithamus]

Sorry @motiz88 I probably summarised too much with me above comment, here's a more elaborate answer:

Chai-HTTP simply exposes SuperTest's API but using Chai compatible assertions. SuperTest implements SuperAgent, with extra test utility. SuperAgent returns a callback with errors for status >= 200, SuperTest and Chai just pass a long what SuperAgent tells them.

What I'm saying is - you're not wrong; in that it is making assumptions about your API. But the problem is probably best solved elsewhere. Chai-HTTP couldn't fix the issue without silly hacks or very precise workarounds for the SuperAgent API. The way I see it you have 6 avenues:

1. Raise an issue with SuperAgent, and hope that they change their API (perhaps to closer mimic the fetch api). This will eventually trickle down to Chai-HTTP, and the problem will be fixed. This would be the purest way to fix this - but extremely unlikely to happen.
2. Chai-HTTP pulls out the SuperAgent API and replaces it with a Fetch-like implementation, which solves this issue. This would be incredibly disruptive to existing users who rely on this behaviour, and also rely on other parts of the SuperAgent API - such as its plugins. Very unlikely to happen - I would not support this change due to it being so disruptive.
3. Chai-HTTP adds new assertions which tack onto promises to handle rejected cases like these - for example await chai.request(app).get('/faketable').badRequest(); (something with a better name). This could be likely to happen, but isn't very pretty, and I can image a lot of issues around its use. I don't really see this as a useful solution, unless you can come up with something more elegant than I'm envisioning.
4. Raise the issue with Chai proper, about how to reasonably handle rejected Promises in a modern way - that might be through a similar mechanism to Chai-As-Promised's eventually.reject - basically flip a rejected Promise into a resolved one, and a resolved one into a Rejected one. If Chai can handle this in a decent - extensible - way, then Chai-HTTP could utilise this. There's been some small discussions about Chai handling promises, so this could be a likely scenario - but I don't see this getting fixed in the next six months.
5. Write your own version of Chai-HTTP, that uses a fetch-like API (maybe call it Chai-HTTP-Fetch 😄). This is as likely as you want it to be - you'd hear no argument from me or any upstream packages 😉.
6. Deal with the foibles of the existing ecosystem, and fix things in userland. As you rightly say, Chai-As-Promised has to.be.rejected, so use that. It means slightly more code per test for you, but out of all of your options, this one is the fastest fix.

Thoughts?

[motiz88]

My actual workaround is along the lines of:

// with serverAddress lifted from chai-http
const appFetch = (path, ...args) => fetch(serverAddress(appServer, path), ...args);
// ...
const res = await appFetch('/faketable');
res.should.have.property('status', 404);

Anything else (e.g. your suggestion in 6) quickly becomes non-semantic, more fragile and less readable. Imagine testing for a specific status code (my initial use case) - would you ever use to.be.rejectedWith(/Not Found/) over .to.have.status(404)? And what if I need to test the response body (and I do, in fact), which gets completely lost in the Promise rejection?

My other replies are:

1. SuperAgent's opinionated choice is justifiable for application code, so I don't have a problem there. But it breaks the test authoring use case with no real justification IMHO. (Perhaps SuperTest is at fault? I don't know the details.)
2. Too drastic of course.
3. Given the current situation I'm definitely thinking of at least an opt-out method (which would be semver-minor). Let me configure chai.request's behavior globally or per-call, such that if I wish, it replaces Super(Agent|Test)'s Promise wrapping logic with its own version, that doesn't reject based on response content. SuperAgent's callback API seems to support this usage. Would you consider accepting a PR to this effect?
4. Not applicable in my view, since this is ultimately about a Promise being rejected when it logically shouldn't be.
5. That's tempting, can't say it hasn't crossed my mind 😄 I would probably not provide any client logic of my own, and rather focus on adding Chai-HTTP-ish assertions to existing (native/polyfill/universal/mock) Fetch API Response objects. Perhaps with the Express support thrown in, too.
6. See above.

[keithamus]

Let me configure chai.request's behavior globally or per-call, such that if I wish, it replaces Super(Agent|Test)'s Promise wrapping logic with its own version, that doesn't reject based on response content. SuperAgent's callback API seems to support this usage. Would you consider accepting a PR to this effect?

Could you explain in more detail what you hope to achieve here? Bear in mind that the only Promise logic chai-http currently has is function (err, response) { if (err) { return reject(err); } resolve(response); }.

If you plan to take err and detect what kind of error it is, this could be done in userland, right? To wit:

function handleErrors(promise) {
  return promise.catch((err) => {
    if (err.code == 404) return err;
    throw err;
  });
}


describe('Querying a nonexistent table', function() {
  it('causes a 404', async function() {
    const res = await handleErrors(chai.request(app).get('/faketable'));
    res.should.have.status(404);
  });
});

[motiz88]

Actually, I'd do

if (err.response)
   return err.response;

in that handler. Which appears to be in line with SuperAgent's public API ("opinionated" response-based errors will have .response and .status set; anything else won't).

Yes, it could be done this way in userland. But the goal of writing clear test cases with minimal boilerplate takes a hit. My suggestion (and I'm willing to try and put together a PR to that effect) is to bake that logic into Chai-HTTP in a minimal, opt-in fashion.

[motiz88]

And, at least in this dev's experience, the current behavior violates the principle of least surprise.

[keithamus]

bake that logic into Chai-HTTP in a minimal, opt-in fashion.

how would you do this though? That's the bit I'm not following.

[motiz88]

Well, assuming this functionality is desired, there are some questions of scope/consistency, then style and bikeshedding. Here are the main ones in my view.

1. Do we only add this option to the promise code path?
   • Pro: Simple to implement (particularly if all promise-generating calls go through Test.prototype.then at some point).
   • Con: Inconsistency between .then/.catch and .end.
   • Justification: The user's .end callback already receives both the error object and the response, necessarily has separate branches for whatever its own definition of success/failure is, and so has the all the information available on an equal footing. Ergo there's less harm in leaving this path unchanged. (I would definitely warn newcomers about this in a documentation note, though. Come to think about it, I'd add a warning now 😄 )
2. How should the user opt in to this behavior?
   • Globally?
     • chai.use(require('chai-http').dontFilterPromises)
     • chai.use(require('chai-http').configure({dontFilterPromises: true})
     • etc
   • At the call site?
     • chai.request(app).unfiltered.get('/')
     • chai.request(app, {dontFilterPromises: true).get('/')
     • even chai.request(app).get('/').unfiltered.then() can be made to work
     • etc
3. What to call the behavior/flag? It needs something snappy that doesn't mislead users of .end etc. dontFilterPromises/dontRejectHttpErrors might be fine for a global flag, unfiltered or something shorter is preferable for per-call-site.

[motiz88]

Right now I like chai.use better than the per-call-site approach.

[keithamus]

It's not really Promises that are the problem here, its the way end provides error/response values. If we were to change a particular behaviour it'd be better coming from the root cause, which is .end(). Perhaps a flag like badStatusCausesError which defaults to true.

I think I'd prefer to see the behaviour at a call site level, as globally it could be a bit of a footgun.

Before we implement a change here though (and I'm not saying we definitely would right now), I'd like to at least raise the issue with SuperAgent to see if we can have an opt-in way of not erroring on bad status codes on their end.

[farmisen]

@keithamus, did you have any chance to discuss this with SuperAgent? Found this issue after struggling myself while trying to test response failures and I felt the default behavior was counter intuitive. I did fork and implemented your last suggestion to move forward w/ my tests and I am going to submit a PR for at least re-initiate this discussion. thanks!

[keithamus]

Hey @farmisen - sorry I have not had the time to raise this issue with SuperAgent. I've had a look at your PR and while it looks good at first glance, I think my comment still stands, we should raise this with SuperAgent before continuing. Would you like to raise it in the SuperAgent issue tracker?

[mvanlonden]

@keithamus this seems to be the responsibility of chai-http. It would be helpful if you accepted @farmisen 's PR :)

[teajaymars]

I'm trying to pick up the thread of this discussion, where using chai-http has also violated my own principle of least surprise and I'm confused as to how I should proceed.

I think the crux is that HTTP 4xx are client errors, whereas HTTP 5xx are server errors. When I'm testing a server with chai-http, I want to check that the server responds correctly to a misbehaving client.

SuperAgent's goal is to be a "progressive client-side HTTP request library", so I think their design is reasonable: 4xx errors and 5xx errors are all equally bad.

chai-http's goal does not entirely overlap with superagent. Client errors are "differently bad" in a test harness, and for this reason I think it's firmly chai-http's responsibility to differentiate between them in the API. So @keithamus, I also very respectfully disagree here, although I sympathise that it will make the superagent-wrapping implementation less elegant. :-)

[keithamus]

FYI it looks like ladjs/superagent#1069, when implemented, might be the solution we're after.

[richardpringle]

If SuperTest uses SuperAgent's api in such a way that the final test callback does not receive an error on 4xx and 5xx responses, chai-http should do the same.

I can intuitively use SuperTest without knowing much (if anything) about SuperAgent. The case should be the same for chai-http.

[keithamus]

ladjs/superagent#1069 has been fixed and released as v3.3.0, so this is now ready to be fixed.

---

The change:

We need to use .ok for each of the methods inside chai-http, to ensure that bad status codes still return a resolving promise. By calling .ok with a function returning true, all requests will resolve. This issue will be fixed.

Who can make this PR?

Ideally, as this is a reasonably straightforward fix, and we would like to encourage new contributors to the Chai community, I would like to suggest new contributors and yet-to-be contributors only. If you have made a few PRs, or if you think you could complete this easily, then we have lots of other issues across our organisation which could be looked at instead.

I want to make this PR!

Great! If you're an existing contributor who is still new, or have yet to contribute to chai, then this is the PR for you. You should take a look at our Code of Conduct and Contribution Guidelines (note: we need to work on our contribution guidelines, so these might not apply exactly to this sub-project).

If you want to work on this, you should comment below, something like:

Hey, I'd like to work on this issue, I'll be making a PR shortly.

If you don't comment you might run the risk of someone else doing the work before you!

If someone has already commented, then you should leave this PR for them to work on.

If someone has commented, but a few weeks have passed - it is worth asking the person who has commented if they're still interested in working on this, and if you can pick it up instead.

How to make a PR for this change:

Firstly, you'll need to update SuperAgent to ^3.3.0 in our package.json right here - to change the code by adding a call to .ok around L246 of lib/request.js. The code should look something like:

function Test() {
  Request.call(this, method, path);
  this.app = app;
  this.url = typeof app === 'string' ? app + path : serverAddress(app, path);
  this.ok(function () { return true });
}

You'll also need to add some tests (perhaps, around L76 of test/request.js that reflect the changes. Perhaps something like:

    it('succeeds when response has bad status', function (done) {
      request('https://httpbin.org/status/400')
        .end(function (err, res) {
          res.should.have.status(400);
          done(err);
        });
    });

    it('resolves as a promise when given bad status code', function () {
      return request('https://httpbin.org/status/400')
        .then(function (res) {
          res.should.have.status(400);
        });
    });

What happens then?

We'll need 2 contributors to accept the change, and it'll be merged. Once merged, we'll need to take some time to make a manual release, which will take a while - as we need to prepare release notes. It'll likely be released as chai-http@4.0.0 because this will be considered a breaking change.

[oluoluoxenfree]

Hi, would it be okay if did this one?

[richardpringle]

sorry @oluoluoxenfree, I didn't see your comment.

[keithamus]

@oluoluoxenfree sorry you missed out there. With our next issue which can be resolved with a PR, you will get first refusal to solve it. I'm also going to work on some guidelines to ratify all of this.

@richardpringle thanks very much for the awesome work - next time before working on the PR if you would so kindly announce in the issue comments, that'd be swell. 👍 😄

[wdullaer]

Any idea when this will be released to npm?

[inancgumus]

@wdullaer You could install it by this commit without waiting for a npm release:

npm i https://github.com/chaijs/chai-http#3ea4524 --save-dev

[shannonmoeller]

A solution that works right now without a PR is to catch the error with .then(successCallback, errorCallback). The error object contains the response object so you can do this:

it('should handle invalid ids', () => request(app)
    .get('/v1/foo/bogus/')
    .then(
        () => expect.fail(null, null, 'Should not succeed.'),
        ({ response }) => {
            expect(response).to.have.status(404);
            expect(response).to.be.json;
            expect(response.body.success).to.equal(false);
            expect(response.body.errors[0]).to.include('not found');
        }
    )
);

Edit: Updated example based on @Yitzi2's feedback.
Edit 2: The .catch(err => err.response) suggestion by @vieiralucas below is a cleaner solution.

[oreporan]

I'm using async/await
Would just like to point out that to overcome this issue my test looks like this now:

try {
                await chai.request(app).get('bad/path').send()
                expect(true).to.be.false // Fail the test
            } catch (err) {
                expect(err).to.have.property('status')
                expect(err.status).to.be.equal(400)

Now if the request returns 200 (which should make the test fail), it drops to the expect(true).to.be.false line, which also throws an exception, so now my test fails (as expected), but it fails just because there is no status on the error...Not really insightful....

[Yitzi2]

shannonmoeller, that's a good solution, though you probably want a then block too, so that if it does (incorrectly) return a "valid" status code, the test will fail rather than not running.

[shannonmoeller]

@Yitzi2 Good call. I'll edit the example to include that.

[shannonmoeller]

@oreporan You need to assert on the error.response property, not the error object itself:

try {
    await request(app).get('/v1/foo/bogus/');
    expect.fail(null, null, 'Should not succeed.');
}
catch ({ response }) {
    expect(response).to.have.status(404);
    expect(response).to.be.json;
    expect(response.body.success).to.equal(false);
    expect(response.body.errors[0]).to.include('not found');
}

[aPoCoMiLogin]

Guys, just follow @inancgumus comment #75 (comment) since when this will got published, you will have to change your code. EOT

[shannonmoeller]

@keithamus Given this is a breaking change, can we assume that chai-http will be getting a major-version bump when this is released?

[oreporan]

@shannonmoeller
But expect.fail(null, null, 'Should not succeed.'); throws an exception. So There is no status on the exception.

[cklanac]

This is the approach we use for testing 404's and other errors like 401, 403, 409 and 422. Checking the spy should ensure that if the path responds with 20x status, the test should fail.

'use strict';

const chai = require('chai');
const chaiHttp = require('chai-http');
const spies = require('chai-spies');

chai.should();
const { app } = require('../server');

chai.use(chaiHttp);
chai.use(spies);

describe('Wrong paths', function () {

  it('should respond with 404', function () {
    const spy = chai.spy();
    return chai.request(app)
      .get('/v1/foobar')
      .then(spy)
      .catch((err) => {
        const res = err.response;
        res.should.have.status(404);
      })
      .then(() => {
        spy.should.not.have.been.called();
      });
  });
  
});

[elias-garcia]

Any update on handling errors properly?

[vieiralucas]

@elias-garcia I believe that this will be fixed when #184 gets merged and we finally release a new version of chai-http

To workaround this you can catch the superagent error or install chai-http from master

catching superagent error

 chai
  .request(app)
  .get('/')
  .catch(err => err.response)
  .then(res => {
    expect(res).to.have.status(404)
  })

installing chai-http from master

npm i --save-dev https://github.com:chaijs/chai-http

 chai
  .request(app)
  .get('/')
  .then(res => {
    expect(res).to.have.status(404)
  })

[Stilldabomb]

February 2018, still not published on NPM? I'm still getting this same problem. 2 year issue..

[gigi]

Any plans for release? We are waiting for proper testing...

[richardpringle]

@gigi, I'm pretty sure this has been released in 4.0.0

@keithamus, I'm pretty sure this issue can be closed

[keithamus]

Thanks @richardpringle!

[mxmaxime]

Hi,
Yes, it has been released.
Thanks,
Happy testing!

[vidya3105]

i am new to node js and chai.
I am making a simple get request but getting 404 error.
chai.request(host).get(path).set('Content-Type', 'application/json').send().end((err, res)

and response i am getting is
{
"req": {
"method": "get",
"url": "http://localhost:8080/ping/",
"headers": {
"user-agent": "node-superagent/3.8.3"
}
},
"header": {
"content-type": "text/html",
"content-length": "233",
"server": "Werkzeug/0.11.15 Python/2.7.13",
"date": "Thu, 16 Aug 2018 03:4\n4:30 GMT"
},
"status": 404,
"text": "\n<title>404 Not Found</title>\n

Not Found

\n

The requested UR\nL was not found on the server. If you entered the URL manually please check you\nr spelling and try again.

\n"
}

can anyone help me out where i am doing wrong?

[joaomlneto]

all: sorry for the spam you got in your inboxes for being subscribed to this closed topic.

@vidya3105 this is not the right place nor time for that question. Does not fit the topic (ES7 async/await), and is doubly bad because the issue has been closed. There are a dozen people that got your message in their inboxes when they really shouldn't have.

Checking the response you got, you can see that the http query worked -- you got a reply from the server. The problem is either you set the wrong URL, or you have a problem with your web server.

Next time, open an issue if you find a bug or have a feature request. If looking for general help I'd suggest stackoverflow. In there you should also provide a minimal, verifiable and complete example. It's impossible to help you further since we can't reproduce it.

Sorry for the bluntness :P -- please don't take it the wrong way! Welcome! :-P but wrong place for that kind of questions! <3

[karosi12]

Hi guys, the .catch(err => err) help me to return the error and save it to the error variable inside the try block.

try {
  const data = {};
 const error = await request(app).post('url').send(data)
                         .set("Accept", "application/json")
                         .expect(400)
                         .catch(err => err);
                         expect(error.response.body)
   } catch (error) {
     throw new Error(error);
  }