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

4xx response body is null #1

Closed
ngot opened this issue Jun 15, 2020 · 5 comments · Fixed by #5
Closed

4xx response body is null #1

ngot opened this issue Jun 15, 2020 · 5 comments · Fixed by #5
Assignees
Labels
bug Something isn't working

Comments

@ngot
Copy link

ngot commented Jun 15, 2020

Issue

Setup:

  • deno 1.0.5
  • v8 8.4.300
  • typescript 3.9.2

4xx response body is null, even though there is a response body.

Details

I want to test the custom response body, but there is nothing in res body.

By the way, when responding 4xx, superdeno also throws error which is different from supertest, which is annoying.

@asos-craigmorten
Copy link
Collaborator

asos-craigmorten commented Jun 15, 2020

Hmm, this is actually an issue that has impacted supertest on several occasions, see:

And is generally as a result of the underlying superagent library having an issue, e.g.:

It looks like we're using a later version of sugeragent than supertest is, when I next have time I'll see if matching the versions (i.e. a downgrade) solves it. Given this library almost exactly mirrors how supertest works I suspect it is likely the dependency, but it may be a nuance that was missed in porting it over.

Do you have a minimal reproducible code example that can be used (e.g. server and test you're trying to run)?

Alternatively, please feel free to investigate and try and tackle the issue! PRs are welcome 😄

@ngot
Copy link
Author

ngot commented Jun 15, 2020

It can be reproduced by:

import {
  Application,
} from "https://deno.land/x/ako/mod.ts";
import { superdeno } from "https://deno.land/x/[email protected]/mod.ts";
import { describe, it } from "https://deno.land/x/[email protected]/test/utils.ts";
import {
  assert,
  assertEquals,
} from "https://deno.land/[email protected]/testing/asserts.ts";

describe("ctx.onerror(err)", () => {
  it("should respond", (done) => {
    const app = new Application();

    app.use((ctx, next) => {
      ctx.body = "something else";

      ctx.throw(418, "boom");
    });

    const server = app.listen();

    superdeno(server)
      .head("/")
      .expect(418)
      .expect("Content-Length", "4")
      .expect("Content-Type", "text/plain; charset=utf-8")
      .end((err, res) => {
        assertEquals(err.message, "I'm a teapot");
        assertEquals(res.status, 418);
        // TODO: https://github.com/asos-craigmorten/superdeno/issues/1
        // assertEquals(res.text, "boom");
        done();
      });
  });
});

According to ladjs/superagent#1468, seems like it was fixed. I will try to fix it this week sometime.

@asos-craigmorten
Copy link
Collaborator

asos-craigmorten commented Jun 19, 2020

Taking a look at this. Added a simple unit test for .expect(400) and seeing:

error: Uncaught Error: Bad Request
          new_err = new Error(res.statusText || res.text || 'Unsuccessful HTTP response');
                    ^
    at Test.<anonymous> (https://dev.jspm.io/npm:[email protected]/lib/client.dew.js:475:21)
    at Test.Emitter.emit (https://dev.jspm.io/npm:[email protected]/index.dew.js:148:22)
    at XMLHttpRequestSham.xhr.onreadystatechange (https://dev.jspm.io/npm:[email protected]/lib/client.dew.js:784:12)
    at XMLHttpRequestSham.xhrReceive (~/git/asos-craigmorten/superdeno/src/xhrSham.js:105:29)
    at ~/git/asos-craigmorten/superdeno/src/xhrSham.js:52:21
    at Object.xhr.onreadystatechange (~/git/asos-craigmorten/superdeno/src/xhrSham.js:186:7)
    at XMLHttpRequestSham.xhrSend (~/git/asos-craigmorten/superdeno/src/xhrSham.js:287:9)
    at async XMLHttpRequestSham.send (~/git/asos-craigmorten/superdeno/src/xhrSham.js:51:7)

The referenced line is here --> https://github.com/visionmedia/superagent/blob/master/src/client.js#L468

Introduced in ladjs/superagent#1473


I can confirm that isn't an issue with latest supertest having tried to reproduce with:

const express = require("express");
const request = require("supertest");

const app = express();

app.get("/", (req, res, next) => {
  res.sendStatus(400);
});

request(app)
  .get("/")
  .expect(400)
  .end((err, res) => {
    if (err) throw err;
  });

So it is something different with SuperDeno. The version of superagent is different, so going to try downgrading to 5.1.0 to match supertest.

@asos-craigmorten asos-craigmorten linked a pull request Jun 19, 2020 that will close this issue
2 tasks
asos-craigmorten added a commit that referenced this issue Jun 19, 2020
@asos-craigmorten asos-craigmorten added bug Something isn't working and removed needs investigation labels Jun 19, 2020
@asos-craigmorten
Copy link
Collaborator

Thanks for raising this @ngot 😄 found the bug and should be fixed in latest version! 🎉

@asos-craigmorten
Copy link
Collaborator

Let me know if still encountering any issues!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants