ladjs / supertest

🕷 Super-agent driven library for testing node.js HTTP servers using a fluent API. Maintained for @forwardemail, @ladjs, @spamscanner, @breejs, @cabinjs, and @lassjs.
MIT License
13.81k stars 759 forks source link

Error response: WebSockets request was expected #787

Open nickgrealy opened 2 years ago

nickgrealy commented 2 years ago

I have an almost identical test case to https://github.com/visionmedia/supertest/issues/566 , except in my test I'm iterating over the await request().get().expect() 1000 times, to test the performance of the endpoint.

80% of the time, the tests pass, but occasionally I hit the following issue.

Error: expected '{"mydata":0}' response body, got 'WebSockets request was expected\r\n'

(400 Bad Request)

Has anyone seen this before?

I've tried adding Promise awaits for the handlers in the beforeEach/afterEach, to no avail.


Node: v18.3.0 Express: 4.17.1 Supertest: 6.2.4

TestCase.ts

import express, { Application } from 'express'
import { Server } from 'http'
import { describe, it } from 'mocha'
import request from 'supertest'
import MyRouter from '../../src/routes/MyRouter'

describe('wip PerformanceTesting - 1000x should be less than X milliseconds', () => {
    const app: Application = express().use(MyRouter)
    let server: Server

    beforeEach(async () => {
        server = await new Promise((resolve) => {
            const s = app.listen(0, () => resolve(s))
        })
    })

    afterEach(async () => {
        await new Promise((resolve) => server.close(() => resolve(null)))
    })

    it('my performance test', async () => {
        for (let i = 0; i < 1000; i++) await request(app).get('/api/sales/data').expect('{"mydata":0}').expect(200)
    }).timeout(1000)
})
nickgrealy commented 2 years ago

Makes me wonder whether this issue has always been possible, but just that there's a low likelihood of it occurring.

Related: https://github.com/davidje13/superwstest/issues/11

( @davidje13 - you might be interested? )

davidje13 commented 2 years ago

I suspect this is a different issue to the one I raised (#566), but you do also have that issue (since you are using request(app) instead of request(server), so it doesn't matter that you're managing your own server.

If you are seeing "WebSockets request was expected", that means a server has successfully started on that port already and is serving responses, but the server is expecting websocket requests. Looking around for that error message, it seems to be related to the built-in debugger (launched by passing --inspect-brk to the node invocation. I'd guess maybe your IDE is doing this automatically?)

My guess is that in the event that request(app) randomly picks the same port that the node debugger is already using, you get this error. It's a bit surprising though, because I think request(app) uses 0 for the port, which is supposed to pick a random available port.

Replacing request(app) with request(server) may fix both issues. I guess it would be interesting to see if it doesn't fix the specific issue you see here though (if it doesn't, that could actually be a bug in NodeJS itself)

davidje13 commented 2 years ago

Thinking about this some more, I think you are getting the port conflict due to differing hosts:

Since the hosts are different, I think the latter will consider 9229 to be an available port, and when it is chosen you will see this issue (because you end up with more than one server on port 9229; one listening just locally on your machine, and the other listening on the network too). You are seeing this particularly often in your test because as-written, you are actually starting 1,000 concurrent servers in the test, each needing its own port (but with the fix I mentioned above, you would only start a single server, as you intended).

So to fully fix it, you should do the fix I mentioned in my comment above and also update your app.listen(0, ...) line to app.listen(0, '127.0.0.1', ...) (or use '::1' if you prefer IPv6). This is a good thing to do anyway since it means your test server won't be visible on the local network, which is unlikely to be what you want.

joachimbulow commented 2 months ago

This is still an issue

davidje13 commented 2 months ago

@joachimbulow if you're getting the specific error "WebSockets request was expected", it means you're somehow accidentally connecting to the NodeJS process' built-in debugger port, likely due to an issue with port randomisation. See my comments above for full details on how this can happen and what you need to do to fix it (summary: make sure you're not starting hundreds / thousands of servers in your test, and bind them to 127.0.0.1 instead of the default 0.0.0.0).

joachimbulow commented 2 months ago

Using NestJs, i THINK, i do not KNOW, since i haven't tried brute forcing:

I was able to remedy by using const server = app.getHttpServer() and then apply: server.listen(0) in our Jest beforeAll method of each test suite.

Read some thread about "warming up" the server.