Skip to content

Add test foundation: Vitest unit suite, zero type errors, ESLint - #61

Open
ameyypawar wants to merge 4 commits into
masterfrom
feat/test-foundation
Open

Add test foundation: Vitest unit suite, zero type errors, ESLint#61
ameyypawar wants to merge 4 commits into
masterfrom
feat/test-foundation

Conversation

@ameyypawar

Copy link
Copy Markdown
Collaborator

Fixes #47
Fixes #52
Fixes #48
Fixes #40
Fixes #38
Fixes #20

Addresses #51 — see note below.

Why

The project had no tests and no CI, npx tsc --noEmit failed with 7 errors while next.config.ts suppressed them, and ESLint was neither installed nor configured despite a lint script existing. With outside contributors about to arrive, there was nothing to catch a regression in review.

This lands the foundation that runs with no Java, no emulator and no credentials, so a stranger can clone and get a signal in one command. Emulator-backed rules and integration tests are deliberately a follow-up.

Type errors: 7 → 0

  • src/lib/types.tsCommunity.icon is now required (all six COMMUNITIES entries supply one). Event gains createdAt?: string and author: UserProfile; both were already written by addEvent, so the type was wrong rather than the code. EventDataForFirestore's own Omit<Event, …'author'> was the tell.
  • src/components/settings/SettingsContent.tsx — "Member Since" read user.metadata?.creationTime, a Firebase Auth field that does not exist on this app's UserProfile. It silently rendered "N/A" for every user, always — a live bug, not a type nit. Now reads user.createdAt, which createUserProfile already stores.
  • src/lib/services/searchService.ts, src/lib/mockData.ts — supply author now that it is required.

Note on #51: the "Last Sign In" row is removed rather than fixed. There is no stored equivalent, and inventing a lastSignInAt field was out of scope. #51 stays open for that.

Three as unknown as Event casts in eventService.ts were attempted and reverted — removing them breaks tsc, because spreading DocumentData contributes any and the object literals then infer as missing every field. That is #48's deeper half and needs its own change.

Three bugs fixed by making them testable

Rather than test around them, the logic was extracted to pure functions and the bugs fixed in the move:

  • Copy the array before sorting question lists in place #40sortQuestions now sorts a copy. Previously Array.prototype.sort() mutated the questions React state array in place whenever no filter was active. MyForumsList.tsx had the identical shape and now shares the helper.
  • Normalize tag case before the duplicate check in QuestionForm #38addTagNormalized normalises once and dedupes against the normalised value, so "React" after "react" is correctly rejected. QuestionEditForm was already correct but is rewired to the same helper so the two forms cannot drift apart again.
  • Block javascript: URLs in event rsvpLink (stored XSS) #20isSafeHttpUrl allowlists http:/https:. Verified empirically against this project's Zod 3.24.2 that z.string().url() accepts javascript:, data:, vbscript: and ftp:. Also confirmed the WHATWG URL parser normalises case, leading whitespace and embedded newlines before exposing .protocol, so the allowlist defeats those obfuscations for free. Rewired the only two z.string().url() call sites in the tree, both in EventForm.tsx.

Still open at the render sites: EventCard.tsx (<Link href={event.rsvpLink}>) and events/[id]/page.tsx (<Image src={event.posterImageUrl}>) rely solely on write-time validation. Documents written before this change are unvalidated, so #20's sink-side guard is not yet in place.

Test suite

vitest.config.ts uses test.projects with only a unit project defined, shaped so rules and integration can be added without restructuring.

 ✓ tests/unit/userUtils.test.ts        (12 tests)
 ✓ tests/unit/urlUtils.test.ts         (13 tests)
 ✓ tests/unit/tagUtils.test.ts          (6 tests)
 ✓ tests/unit/questionListUtils.test.ts (6 tests)
 ✓ tests/unit/commentUtils.test.ts      (4 tests)

 Test Files  5 passed (5)
      Tests  41 passed (41)   —  318ms

The unit graph is verified Firebase-free: the five test files reach only the five util modules and @/lib/types, which imports nothing. So npm run test needs no JVM, no emulator and no credentials.

Notable coverage beyond the happy paths: an unrecognised role string falls through to the user branch rather than escalating; buildCommentTree promotes an orphan to top level (a comment whose parent was removed by the search filter) instead of dropping it; a parentId cycle terminates; and one test asserts side by side that bare z.string().url() accepts javascript:alert(1) where safeHttpUrl rejects it, so the refinement cannot be "simplified" away later.

Emulator support in firebase-admin.ts

getAdminApp() gains a branch, before credential lookup, that uses initializeApp({ projectId }) with no credential when FIRESTORE_EMULATOR_HOST or FIREBASE_AUTH_EMULATOR_HOST is set.

This is a prerequisite, not a nicety: cert() parses the PEM eagerly and rejects a synthetic key (DECODER routines::unsupported), so there is otherwise no way for a test to import any service module. It also means a contributor with no production credentials can run the app against emulators.

The production path — loadAdminCredential() and the real initializeApp({ credential }) — is byte-identical to master.

ESLint

Measured before gating. npx eslint . reported 37 problems (36 errors, 1 warning), with all 36 errors from a single rule, react/no-unescaped-entities, across ~15 pre-existing files. Only that rule is demoted to warn; --max-warnings is left unset. eslint . now exits 0 with 33 warnings. A narrow blocking gate beats a broad advisory one that everybody learns to ignore.

Also deleted src/app/auth/page copy.tsx and page copy 2.tsx — confirmed unrouted (the App Router only registers the exact filename page.tsx) and unimported anywhere.

Verification

  • npx tsc --noEmit0 errors (from 7)
  • npm run test → 41 passing, no Java/emulator/credentials
  • npm run build → compiles, all 16 routes
  • npx eslint . → exit 0, 33 warnings, 0 errors
  • npm run dev → boots, 200 on /
  • Confirmed all 3 event documents in the live database carry author, so making that field required does not diverge from real data

Deliberately out of scope

next.config.ts keeps ignoreBuildErrors and ignoreDuringBuilds for now. Removing them while the gates are brand new means one unrelated type error could block a production deploy. That becomes a no-op change once typecheck and lint have run green in CI for a few PRs.

- Community.icon is required: all six COMMUNITIES entries already
  supply one, so the optional type was allowing an unreachable
  undefined-component case in community/[communityId]/page.tsx.
- Event gains createdAt?: string and author: UserProfile: both are
  already written by eventService.addEvent (server timestamp + the
  full UserProfile), the type just never reflected it. Propagates a
  matching `author` fix into searchService.searchEvents and the unused
  mockEvents fixtures, both of which build Event literals by hand.
- SettingsContent read user.metadata, which doesn't exist on the app's
  UserProfile (that's a Firebase Auth User field, not what useAuth()
  returns) - "Member Since" silently showed N/A for every user. Fixed
  to read UserProfile.createdAt. There's no stored equivalent for
  "Last Sign In", so that row is removed rather than inventing a field.
cert() parses the PEM eagerly and throws DECODER routines::unsupported
on a synthetic test key, so there's no way to satisfy the production
credential path in a test. When FIRESTORE_EMULATOR_HOST or
FIREBASE_AUTH_EMULATOR_HOST is set, initialize with just a projectId
(from GCLOUD_PROJECT or FIREBASE_PROJECT_ID) and no credential instead.
loadAdminCredential() and the production path are untouched.

Side benefit: this also lets a contributor without production Firebase
credentials run the app locally against emulators.
Vitest (^3.2.7, needs >=3.2 for test.projects) with a single `unit`
project: tests/unit/**/*.test.ts, node environment, no setupFiles, no
env, @ -> ./src alias. resolve.alias lives at the config root rather
than inside the project so the `rules`/`integration` projects Phase 2
adds can inherit it without this file being reshaped (inline projects
need `extends: true` to actually inherit root options - verified
empirically, the docs undersell how load-bearing that flag is).
`npm test` runs `vitest run --project unit`; added `engines.node`.

Extracted four pure modules out of components/pages so they're
testable without a browser or a live Firestore connection, each
imports nothing but @/lib/types and (for urlUtils) zod:

- lib/utils/commentUtils.ts: buildCommentTree, moved out of
  app/qna/[id]/page.tsx verbatim.
- lib/utils/questionListUtils.ts: filterQuestions/sortQuestions, moved
  out of QuestionList.tsx. Fixes #40: sortQuestions now sorts a copy
  instead of calling Array.prototype.sort() on the array that was also
  the `questions` React state, which silently reordered it in place
  whenever no filter was active. MyForumsList.tsx had the identical
  shape (search filter + activity-desc sort) and now shares the same
  two functions instead of its own copy.
- lib/utils/tagUtils.ts: addTagNormalized. Fixes #38: QuestionForm
  compared the *raw* tag input against the (lowercased) stored list,
  so "React" typed after "react" passed the duplicate check and got
  added anyway. Normalizing once up front before the dedupe check
  fixes it. QuestionEditForm already normalized correctly but is
  rewired to the same helper so the two forms can't drift again.
- lib/utils/urlUtils.ts: isSafeHttpUrl + safeHttpUrl (Zod refinement).
  Fixes #20: empirically confirmed (against this project's installed
  Zod 3.24.2) that z.string().url() accepts javascript:, data:,
  vbscript:, and ftp: URLs - it only checks for *an* absolute URL, not
  a safe scheme. Allowlisting http/https via the WHATWG URL parser
  also closes the case/leading-whitespace/embedded-newline bypasses
  for free, since the parser normalizes all of those before exposing
  `.protocol`. Rewired EventForm's posterImageUrl and rsvpLink, the
  only two `z.string().url()` call sites in the codebase (grepped).

41 unit tests across 5 files cover all of the above, including a
side-by-side test asserting bare z.string().url() still accepts
javascript: so the refinement doesn't quietly get "simplified" away.
There was no ESLint config and no eslint dependency at all - "lint":
"next lint" had nothing to run, and next lint is deprecated in Next 15
and removed in 16. Adds eslint ^9.39.5, eslint-config-next@15.5.23
(pinned to match the installed Next version), and @eslint/eslintrc for
FlatCompat. eslint.config.mjs extends next/core-web-vitals only, with
ignores in the config itself (flat config doesn't read .eslintignore).
"lint" now runs "eslint .".

Measured before gating: a first `eslint .` run reported 36 errors, all
react/no-unescaped-entities (unescaped apostrophes/quotes in JSX text)
across ~15 files unrelated to this change, plus one pre-existing
react-hooks/exhaustive-deps warning. Demoted react/no-unescaped-entities
to 'warn' rather than fixing unrelated file content in a test-
infrastructure change; --max-warnings is left unset. `eslint .` now
exits 0 with 33 warnings (32 react/no-unescaped-entities that were
errors before, plus the pre-existing exhaustive-deps one).

Also deletes src/app/auth/page copy.tsx and page copy 2.tsx - the App
Router only registers the exact filename page.tsx, so these two were
unrouted dead code (confirmed unimported anywhere) that would
otherwise sit there getting type-checked and linted indefinitely.
Removing them dropped 4 of the 36 errors above.
@vercel

vercel Bot commented Aug 9, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
vforum Ready Ready Preview Aug 9, 2026 5:04pm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment