Skip to content

fix: enable skipped proxy tests#1357

Merged
jescalada merged 9 commits intofinos:mainfrom
fabiovincenzi:failing-tests
Feb 3, 2026
Merged

fix: enable skipped proxy tests#1357
jescalada merged 9 commits intofinos:mainfrom
fabiovincenzi:failing-tests

Conversation

@fabiovincenzi
Copy link
Contributor

@fabiovincenzi fabiovincenzi commented Jan 19, 2026

Fixes: #1294

While investigating the issue, I noticed the stop() method in src/service/index.ts was not properly waiting for the server to close:

// BEFORE
async function stop() {
  console.log(`Stopping Service Listening on ${uiPort}`);
  _httpServer.close();  // Async call, but we don't wait for it!
}

This is a potential race condition, so I changed it to resolve only when the callback of _httpServer.close() is called

Also changed _httpServer to be created in start() instead of at module level, making the Service restartable.

However, this was not the cause of the failing tests.

proxy.test.ts

This test mocks the HTTP/HTTPS servers, so it doesn't use the real stop(). The issue was in the close mock wich didn't call the callback, the real stop() method wraps server.close() in a Promise that only resolves when the callback is called:

// BEFORE (buggy mock)
vi.spyOn(https, 'createServer').mockReturnValue({
  listen: vi.fn().mockReturnThis(),
  close: vi.fn(),  // Never calls callback!
} as any);

If the mock's close() never calls the callback, the Promise never resolves, and the test hangs until it times out.

Fix: Mock both http and https, using the pre-defined mockHttpServer/mockHttpsServer that properly call callbacks

testProxyRoute.test.ts

After enabling testProxyRoute.test.ts, users created in testPush.test.ts were "NOT FOUND" during passport deserialization.

Root cause:

  • testProxyRoute.test.ts has afterAll(() => { vi.resetModules(); })
  • This creates a new instance of the in-memory database
  • Passport callbacks captured a reference to the old db module at configuration time
  • New users were created in the new database, but passport looked in the old one

Fix: Use dynamic import in passport callbacks (src/service/passport/local.ts):

Note: This PR fixes the immediate issue, but we should consider #1294 (comment) from @coopernetes about refactoring the test architecture.

@netlify
Copy link

netlify bot commented Jan 19, 2026

Deploy Preview for endearing-brigadeiros-63f9d0 canceled.

Name Link
🔨 Latest commit 1d4b1ff
🔍 Latest deploy log https://app.netlify.com/projects/endearing-brigadeiros-63f9d0/deploys/6980a800d0e7a700088bd903

@github-actions github-actions bot added the fix label Jan 19, 2026
@codecov
Copy link

codecov bot commented Jan 19, 2026

Codecov Report

❌ Patch coverage is 83.63636% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.35%. Comparing base (4a21364) to head (1d4b1ff).
⚠️ Report is 10 commits behind head on main.

Files with missing lines Patch % Lines
src/service/index.ts 75.00% 5 Missing ⚠️
src/proxy/index.ts 86.66% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1357      +/-   ##
==========================================
+ Coverage   80.40%   81.35%   +0.95%     
==========================================
  Files          65       65              
  Lines        4608     4639      +31     
  Branches      775      792      +17     
==========================================
+ Hits         3705     3774      +69     
+ Misses        888      850      -38     
  Partials       15       15              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Contributor

@jescalada jescalada left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When executing npm run test for the first time upon pulling the PR, I got the following error which is similar to the ones we were getting before:

Test script output
⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯ Unhandled Errors ⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯

Vitest caught 5 unhandled errors during the test run.
This might cause false positive tests. Resolve unhandled errors to make sure your tests are not affected.

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯ 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
 ❯ test/testProxyRoute.test.ts:77:5

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯
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.

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯ 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:188:13

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯
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

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯
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 stop proxying for a host when project is DELETED". 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
 ❯ test/testProxyRoute.test.ts:227:5
 ❯ node_modules/@vitest/runner/dist/chunk-hooks.js:752:20

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯
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 create the default repo if it does not exist". 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
 ❯ test/testProxyRoute.test.ts:235:5
 ❯ node_modules/@vitest/runner/dist/chunk-hooks.js:752:20

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯
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 create the default repo if it does not exist". 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.
⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯


 Test Files  48 passed (48)
      Tests  586 passed (586)
     Errors  5 errors
   Start at  22:02:13
   Duration  25.93s (transform 713ms, setup 0ms, collect 2.11s, tests 23.03s, environment 0ms, prepare 71ms)

npm notice
npm notice New major version of npm available! 10.9.2 -> 11.8.0
npm notice Changelog: https://github.com/npm/cli/releases/tag/v11.8.0
npm notice To update run: npm install -g npm@11.8.0
npm notice

The errors seem to come from the following test:

  it('should create the default repo if it does not exist', async function () {
    // Remove the default repo from the db and check it no longer exists
    await cleanupRepo(TEST_DEFAULT_REPO.url);

    const repo = await db.getRepoByUrl(TEST_DEFAULT_REPO.url);
    expect(repo).toBeNull();

    // Restart the proxy
    await proxy.stop();
    await proxy.start();

    // Check that the default repo was created in the db
    const repo2 = await db.getRepoByUrl(TEST_DEFAULT_REPO.url);
    expect(repo2).not.toBeNull();

    // Check that the default repo isn't duplicated on subsequent restarts
    await proxy.stop();
    await proxy.start();

    const allRepos = await db.getRepos();
    const matchingRepos = allRepos.filter((r) => r.url === TEST_DEFAULT_REPO.url);

    expect(matchingRepos).toHaveLength(1);
  });
});

What's very weird to me, is that these tests are passing in the CI despite it being the "first execution" there.

Any thoughts?

@fabiovincenzi
Copy link
Contributor Author

When executing npm run test for the first time upon pulling the PR, I got the following error which is similar to the ones we were getting before:

Test script output
The errors seem to come from the following test:

  it('should create the default repo if it does not exist', async function () {
    // Remove the default repo from the db and check it no longer exists
    await cleanupRepo(TEST_DEFAULT_REPO.url);

    const repo = await db.getRepoByUrl(TEST_DEFAULT_REPO.url);
    expect(repo).toBeNull();

    // Restart the proxy
    await proxy.stop();
    await proxy.start();

    // Check that the default repo was created in the db
    const repo2 = await db.getRepoByUrl(TEST_DEFAULT_REPO.url);
    expect(repo2).not.toBeNull();

    // Check that the default repo isn't duplicated on subsequent restarts
    await proxy.stop();
    await proxy.start();

    const allRepos = await db.getRepos();
    const matchingRepos = allRepos.filter((r) => r.url === TEST_DEFAULT_REPO.url);

    expect(matchingRepos).toHaveLength(1);
  });
});

What's very weird to me, is that these tests are passing in the CI despite it being the "first execution" there.

Any thoughts?

@jescalada I wasn't able to reproduce this locally - the tests pass consistently on my machine from the first run.

However, I believe this is due to a race condition in src/proxy/index.ts. Here we were doing the same as service/index.ts, the stop() method was calling resolve()
immediately without waiting for the server's close() callback to complete:

On systems with different timing, the port might not be released fast enough before start() of the next test is called, causing EADDRINUSE.

I've pushed a fix that properly waits for the close callbacks before resolving. It would be nice if you could test it out, since I can't reproduce this. Thank you!

Copy link
Contributor

@jescalada jescalada left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, however there are still a few issues related to test execution order:

Executing tests with the following script:

    "test-shuffle": "NODE_ENV=test vitest --run --dir ./test --sequence.shuffle",

Will result in a lot of failing tests, likely from improper database/temp file cleanup:

image

I made an issue to track this so we can move this PR forward #1362 🙂

@fabiovincenzi
Copy link
Contributor Author

LGTM, however there are still a few issues related to test execution order:

Executing tests with the following script:

    "test-shuffle": "NODE_ENV=test vitest --run --dir ./test --sequence.shuffle",

Will result in a lot of failing tests, likely from improper database/temp file cleanup:

image I made an issue to track this so we can move this PR forward #1362 🙂

Thanks for testing this! @jescalada
Interesting, I'll take a look at this

@jescalada
Copy link
Contributor

@fabiovincenzi The issue is reproducible from main as well, so I believe it's a pre-existing problem 👍🏼

@fabiovincenzi
Copy link
Contributor Author

@jescalada yes, I'm testing this and it seems that many tests depend on the previous ones

@kriswest
Copy link
Contributor

From memory, there were a great many individual tests that depended on state setup earlier in the same test group. How is the shuffling working, are you shuffling describe blocks or the tests within those blocks? Most of teh dependeencies I remember were within particular blocks.

@fabiovincenzi fabiovincenzi requested a review from a team January 28, 2026 09:17
@fabiovincenzi fabiovincenzi mentioned this pull request Jan 30, 2026
16 tasks
@fabiovincenzi
Copy link
Contributor Author

Created #1378 to make the tests runnable in random order.

@jescalada
Copy link
Contributor

Looks good to go! Any issues related to test execution order are being fixed in #1378 - which also shows all tests passing 👍🏼

@jescalada jescalada merged commit 3a27260 into finos:main Feb 3, 2026
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Failing proxy tests (test cleanup issues)

3 participants