Failing proxy tests (test cleanup issues)

[jescalada]

After the Vitest migration in #1202, we're skipping a few tests that call proxy.start/proxy.stop due to some kind of race condition that makes them fail depending on execution order.

This is the list of problematic test files/tests:

• All tests in proxy.test.ts (3)
• "proxy express application" tests in testProxyRoute.test.ts (8)

Running the proxy.test.ts file results in the following error:

Serialized Error: { code: 'EADDRINUSE', errno: -98, syscall: 'listen', address: '::', port: 8000 }
This error originated in "test/testProxyRoute.test.ts" test file. It doesn't mean the error was thrown inside the file itself, but while it was running.
The latest test that might've caused the error is "should restart and proxy for a new host when project is ADDED". It might mean one of the following:
- The error was thrown, while Vitest was running this test.
- If the error occurred after the test had been completed, this was the last documented test before it was thrown.

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯ Uncaught Exception ⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯
Error: listen EADDRINUSE: address already in use :::8000
 ❯ Server.setupListenHandle [as _listen2] node:net:1937:16
 ❯ listenInCluster node:net:1994:12
 ❯ Server.listen node:net:2099:7
 ❯ Proxy.start src/proxy/index.ts:76:8
     74|     this.httpServer = http
     75|       .createServer(getServerOptions() as any, this.expressApp)
     76|       .listen(proxyHttpPort, () => {
       |        ^
     77|         console.log(`HTTP Proxy Listening on ${proxyHttpPort}`);
     78|       });
 ❯ processTicksAndRejections node:internal/process/task_queues:105:5
 ❯ src/service/routes/repo.ts:135:9

Meanwhile, running all of the files results in varying errors depending on test execution order. When re-running all tests locally, they pass without fail. The first test run always fails in the CI.

Describe the solution you'd like
We should look into these tests and figure out why they might be failing in Vitest since they were working without issues previously. Some things that come to mind:

• Check whether the default proxy.start/proxy.stop behaviour isn't being overridden, and internal dependencies too
  • I have a feeling that some of the doMock for the internal httpServer/httpsServer may be propagating to other tests
• Make sure all tests have proper cleanup functions
  • I've tried this for the misbehaving test files, might be good to do a more systematic check to make sure all test files are doing proper cleanup
  • Maybe we could write a testCleanup() util to unify all test cleanup procedures?

Additional context
Vitest migration PR #1202

[kriswest]

Possible clue:

https://github.com/finos/git-proxy/pull/1202/files/c5754c82815b2fe722f8a118834d8981477b18a1#r2571807306

[coopernetes]

Can we take this opportunity to simply refactor the Express app startup and route creation? Using a library such as https://github.com/eagleera/vitest-mock-express OR just testing the actual Route handler functions directly as unit tests would make this much simpler to test.

This smells of a set of integration tests that are colliding with shared state within the Express app.

It's likely overly complex trying to mock out the actual Express app itself. We can do mocking if we separate out the routes into a standalone set of functions or Router instance that can be tested directly. Testing express() directly isn't valuable.

I can see the value in this style of testing but unless we have a clean way to mock out Express in a sort of "indirect" way (similar to how chai-http used to handle this. That library & Chai in general doesn't have great TypeScript support and we're already moving over to Vitest with ESM support anyways so we need to figure out a better way to do integration testing in general with Express & the proxy middleware.