Extract proxy server into testable modules#1632
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1632 +/- ##
===========================================
+ Coverage 47.81% 62.88% +15.07%
===========================================
Files 382 393 +11
Lines 8525 9218 +693
Branches 3159 3511 +352
===========================================
+ Hits 4076 5797 +1721
+ Misses 3070 2373 -697
+ Partials 1379 1048 -331
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| .post("/logger") | ||
| .set("level", "verbose") | ||
| .set("message", JSON.stringify("msg")); | ||
| expect(response.status).toBe(500); |
There was a problem hiding this comment.
Issue created to deal with oddness of this endpoint.
| const response = await request(app) | ||
| .post(`/${route}`) | ||
| .send({ query: "test query" }); | ||
| expect(response.status).toBe(500); |
There was a problem hiding this comment.
Issue created for this incorrect behavior.
kmcginnes
force-pushed
the
proxy-server-testability
branch
from
5d52f4f to
99ee896
Compare
| it("sets useHttps to false when HTTPS is enabled but cert files do not exist", () => { | ||
| vi.spyOn(fs, "existsSync").mockReturnValue(false); | ||
|
|
||
| const config = resolveServerConfig( | ||
| createEnv({ PROXY_SERVER_HTTPS_CONNECTION: true }), | ||
| ); | ||
|
|
||
| expect(config.useHttps).toBe(false); | ||
| }); |
There was a problem hiding this comment.
This is what we are fixing in the next step.
kmcginnes
force-pushed
the
proxy-server-testability
branch
from
99ee896 to
c794295
Compare
| app.post("/sparql", async (req, res, next) => { | ||
| const logger = getLogger(); | ||
| // Gather info from the headers | ||
| const headers = req.headers as DbQueryIncomingHttpHeaders; |
There was a problem hiding this comment.
the next 10 lines feel like something to be extracted into its own middleware but I understand that this is more structural refactoring, so I guess, this is not in scope
There was a problem hiding this comment.
Correct. I'll likely push all the header parsing in to a utility using Zod for validation when I work on:
2 participants
Description
Refactors the monolithic
node-server.tsin the proxy server package into three focused, independently testable modules:createApp— Express app setup with all routes and middleware, extracted fromnode-server.tsso route handlers can be tested with supertest without starting a real servercreateServer— HTTP/HTTPS server creation decision, separated so the TLS branching logic can be tested in isolationresolveServerConfig— Server configuration resolution (port selection, HTTPS detection, certificate paths, base URL construction), extracted so config logic can be unit tested without side effectsnode-server.tsbecomes a thin orchestrator that wires these modules together.What changed
createApp()with all route handlers, middleware, and error handling intoapp.tscreateServer()for HTTP vs HTTPS server creation intoserver.tsresolveServerConfig()andbuildBaseUrl()intoserver-config.tssetupTests.ts) with credential safety mocks to prevent real AWS credentials or HTTP requests from leaking in testsapp.locals.envassignment, unreachableelsebranch for static file virtual pathNo behavioral changes
All route logic, middleware ordering, IAM signing, query cancellation, and error handling are preserved exactly. The refactoring is purely structural.
Validation
pnpm checkspasses (lint, format, types)pnpm testpasses with 121 tests across 7 test filesRelated Issues
Check List
pnpm checkspasses with no errors.pnpm testpasses with no failures.