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

Error response: WebSockets request was expected #787

Open
nickgrealy opened this issue Sep 7, 2022 · 3 comments
Open

Error response: WebSockets request was expected #787

nickgrealy opened this issue Sep 7, 2022 · 3 comments

Comments

@nickgrealy
Copy link

nickgrealy commented Sep 7, 2022

I have an almost identical test case to #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
Copy link
Author

nickgrealy commented Sep 8, 2022

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

Related: davidje13/superwstest#11

( @davidje13 - you might be interested? )

@nickgrealy nickgrealy changed the title WebSockets request was expected Error response: WebSockets request was expected Sep 8, 2022
@davidje13
Copy link

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
Copy link

davidje13 commented Sep 8, 2022

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

  • --inspect-brk defaults to listening on 127.0.0.1:9229 (ref)
  • thing.listen(0) defaults to listening on an available port on :: or 0.0.0.0 (and this is what request(app) does under-the-hood when called with an express app, but also what you're doing in your explicit code)

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.

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

No branches or pull requests

2 participants