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

Fixes #258, handles undefined Response object rather than throwing an… #260

Merged
merged 3 commits into from Sep 17, 2015

Conversation

mmerkes
Copy link
Contributor

@mmerkes mmerkes commented Jul 20, 2015

… uncaught error

I kept getting this error on random occasions:

/Users/Matt/open_source/express-route-validator/node_modules/supertest/lib/test.js:202
    if (res.status !== status) {
           ^
TypeError: Cannot read property 'status' of undefined
    at Test.assert (/Users/Matt/open_source/express-route-validator/node_modules/supertest/lib/test.js:202:12)
    at Server.assert (/Users/Matt/open_source/express-route-validator/node_modules/supertest/lib/test.js:131:12)
    at Server.g (events.js:180:16)
    at Server.emit (events.js:92:17)
    at net.js:1276:10
    at process._tickCallback (node.js:419:13)

When I dug down further, I could set that when res was undefined, it was because the server was unexpectedly timing out:

Error: connect ETIMEDOUT
    at errnoException (net.js:904:11)
    at Object.afterConnect [as oncomplete] (net.js:895:19)

So, if we check for the error first and fire the callback immediately there's not res, the problem is resolved.

This PR is an alternative to #240 and fixes #237

@mikelax mikelax self-assigned this Jul 22, 2015
@mikelax mikelax added this to the 1.1.0 milestone Jul 22, 2015
@Starefossen
Copy link

@mikelax any eta on when this may get merged?

@mikelax
Copy link
Contributor

mikelax commented Aug 15, 2015

I am planning to try and get the 1.1.0 release out next week.

@Starefossen
Copy link

Nice, looking forward to that 😄

@mikelax
Copy link
Contributor

mikelax commented Aug 21, 2015

@mmerkes I just merged in #239 which was a big refactor to test.js
Do you want to resolve the conflicts for this PR?

@mmerkes
Copy link
Contributor Author

mmerkes commented Aug 21, 2015

@mikelax Yeah, I'm happy to resolve the merge conflicts.

@mmerkes
Copy link
Contributor Author

mmerkes commented Sep 11, 2015

@mikelax Meant to knock this out earlier, but been crazy around here. It turns out that the refactor resolved the issues around my changes, so my changes in lib/test.js are obsolete, but I kept the tests that were previously failing (without my fix) to confirm the issue was resolved.

mikelax added a commit that referenced this pull request Sep 17, 2015
Fixes #258, handles undefined Response object rather than throwing an…
@mikelax mikelax merged commit 5ec6685 into ladjs:master Sep 17, 2015
@Starefossen
Copy link

Thanks for fixing this @mikelax. Could you also tag the v1.1.0 version in Git/GitHub?

@mikelax
Copy link
Contributor

mikelax commented Sep 21, 2015

np @Starefossen I just created the v1.1.0 release.

@kadishmal
Copy link

I don't see any changes in this PR other than the test case. Should it fix something? It definitely doesn't fix #258.

@mmerkes
Copy link
Contributor Author

mmerkes commented Oct 19, 2015

@kadishmal As described in the thread, I had fixed this issue in the 1.0 code, but there was a major refactor of the file in 1.1 that held the root of the issue which resolved the problem. The tests that were added were cases that I was able to repro the issue in 1.0, but they pass with no fix in 1.1; thus, the PR became tests confirming the issue was resolved rather than the resolution. If you're still seeing this in 1.1, you should include the repro.

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.

supertest gets exception if the HTTP server is down
4 participants