Skip to content

DOCs + Tests[api]#3

Closed
FreezB11 wants to merge 11 commits into
Algorithec:mainfrom
FreezB11:main
Closed

DOCs + Tests[api]#3
FreezB11 wants to merge 11 commits into
Algorithec:mainfrom
FreezB11:main

Conversation

@FreezB11

Copy link
Copy Markdown
Contributor

We needed a better docs for later purpose, and also needed to test the api with mock data
which has passed on my end and works fine
also added a test.yml for the github actions to run the test.
so far everything looks good will go ahead with new APIs

for future reference please run this before committing
to this repo

# for local checks
NODE_ENV=test JWT_SECRET=test_secret pnpm test:run --reporter=verbose  
# before commits
pnpm prettier --write  .

closes #2

Comment thread server/routes/auth.test.ts Fixed
Comment thread server/routes/orders.test.ts Fixed
Comment thread server/routes/payments.test.ts Fixed
Comment thread server/routes/search.test.ts Fixed
Comment thread server/routes/users.test.ts Fixed
@RaghavDabra

Copy link
Copy Markdown
Member

Thanks for this, the docs are a genuine upgrade and the test scaffold is overdue. A few things to sort before it can merge though.

Blocking

  1. CI is red. The new Tests / Unit & Integration workflow fails on Node 22.x and 20.x got cancelled (matrix fail-fast defaults to true, so one failing leg kills the sibling). The existing CI / Node.js job passes, which means the two pipelines run different commands. You ran pnpm test:run locally, but CI runs pnpm test:coverage. Can you reproduce with pnpm test:coverage and get it green? Until that passes I do not think "works fine" holds.

  2. Coverage Gate was skipped because unit failed, so we have zero signal on whether the thresholds are actually met. That needs to run green too.

  3. CodeQL flagged 5 high-severity "Missing CSRF middleware" alerts. They land on the test makeApp() helpers, but that harness mirrors the real app (express + cookieParser, cookie-based deepenk_token JWT). Two questions: do the production cookie-auth routes actually have CSRF protection? If not, this is a real vuln, not a test artifact. If it genuinely is test-only, scope or suppress it so it stops polluting the security scan.

Non-blocking nits

  • Consider fail-fast: false on the matrix so a failure on one Node version does not cancel the other. Easier to debug.
  • The mock-based tests are great for speed, but one thin integration pass against real route wiring would stop mocks-passing from masking a broken handler.
  • Prettier-only changes (a7182ee) are bundled with substantive work here. Fine for now, but separate format PRs are easier to review.
  • ARCHITECTURE.md describes SSE as the realtime path while still marking the endpoint "(planned)" and socket.ts "[BEING REPLACED]". Just make sure the doc does not read as if SSE is already shipped.

Happy to re-review the moment CI is green. Nice work on the docs especially.

@RaghavDabra RaghavDabra left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Review: DOCs + Tests [api] (#3)

Good coverage of the route layer and the docs are genuinely useful. Main concerns are around the CSRF dependency....Requesting changes on the CSRF item before merge.

Blocking

  1. csrf-csrf is added as a dependency but I don't see it wired in. package.json gains "csrf-csrf": "^4.0.3", and the CodeQL "Missing CSRF middleware" alert is firing 10×, but nothing in this diff actually installs the middleware on the app. Two outcomes, both need addressing:
    • If the intent was to suppress the alert by adding test files to paths-ignore, say so explicitly — those 10 alerts are firing on the test harness cookieParser() calls, which are false positives. The real question is whether the production app (server/index.ts) has CSRF protection. That's not shown here.
    • If you do wire csrf-csrf in globally, it must exempt the Cashfree webhook (POST /api/payments/webhooks/cashfree) and any bearer-token API paths — external callers and mobile clients can't present a CSRF token. Otherwise payments break. Worth a follow-up ticket reference either way.

Coverage gate — needs a decision

  1. The 60% gate looks global but is scoped to a hand-picked file list. The coverage.include in vite.config.ts lists ~14 files (the ones this PR happens to test) and explicitly excludes the service/repository layer. So coverage-gate can pass at "60% lines / 50% branches" while the bulk of business logic (searchEngine, foodService, ridesService, repositories) sits at 0% and isn't counted. The exclusion is documented with a follow-up issue, which is the right instinct — but the gate as written gives a false sense of safety. Suggest either naming it honestly (it's a "tested-files gate," not a project gate) or counting the untested files so the number reflects reality and trends upward as coverage lands.

  2. Coverage is computed up to three times per run. unit runs test:coverage on Node 20 and 22, then coverage-gate re-runs it again from scratch with CLI threshold flags that duplicate the thresholds block already in vite.config.ts. Since the config thresholds already fail the unit job on a breach, coverage-gate is largely redundant compute. Either drop the separate job or have it download the uploaded artifact instead of re-running.

Needs confirmation

  1. smoke job runs pnpm dev:server — does that script exist? The package.json diff only adds test:run and test:coverage; I don't see dev:server. If it's not already in scripts, the smoke job fails immediately. Please confirm.

  2. Payment-intent endpoints accept unauthenticated requests by design? does not set x-user-id when unauthenticated asserts intents can be created without auth. If that's intentional (guest checkout) it's fine, but worth a comment in the route — otherwise it reads like a missing auth guard on a money path.

Non-blocking

  1. Inaccurate comment in vite.config.ts. The note says vitest's v8 provider "defaults to requiring 100% coverage project-wide." It doesn't — there's no default threshold; the thresholds block you added is what creates the gate. The comment will mislead the next person who touches this. Reword to "without this block there is no coverage gate."

  2. TESTING.md documents an SSE test for an endpoint that doesn't exist yet. GET /api/sse/price-alerts is described as "planned" in ARCHITECTURE.md but the testing guide presents a passing-looking test for it. Mark it clearly as aspirational so nobody assumes it's wired.

  3. JWT_SECRET is set at module load with a different value per test file. Works under Vitest's default file isolation, but will produce cross-test token failures if isolation is ever disabled (pool: 'threads' with isolate: false). A shared test setup file would be more robust. Minor.

  4. PR scope is large — docs, route tests, three CI workflows, a new runtime dependency, and a vite config rework in one PR. The csrf-csrf dependency in particular doesn't belong with a "DOCs + Tests" PR; splitting it out would make the security change reviewable on its own.

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

Successfully merging this pull request may close these issues.

New+better Docs under /docs and api endpoint testing

3 participants