test: add coverage for TeamsService, MaintenancePoolService, and UsersService - #86
Merged
chonilius merged 7 commits intoAug 21, 2026
Conversation
Zero prior coverage on TeamsService (team-split.util.spec.ts only covers the pure percentage-math helpers, never the service). Covers create()'s validateSplitPercentages rejection and success path (one split saved per member, defaults createdById to null), and findOne()'s found/not-found cases.
MergeFi#41 Covers assignToBounty's not-found cases (missing team, missing bounty) and its happy path. Also adds two baseline tests documenting the method's current, unguarded behavior for MergeFi#41 (no bounty-status or ownership guard, allowing payout hijack via last-second team assignment): reassignment succeeds today regardless of the bounty's status or existing team. This is the 'known-good before state' the issue asks for so MergeFi#41's own fix has a regression baseline to update once it lands.
… create/findOne/list Zero prior coverage on MaintenancePoolService despite it directly locking real funds. Covers create()'s ACTIVE default and repositoryId/createdById null defaults, findOne()'s NotFoundException, and list().
…ne for MergeFi#48 Covers deposit's not-ACTIVE rejection, first-deposit escrow funding, and balance accumulation across deposits. Also adds a regression baseline for MergeFi#48 (repeat deposits create a brand-new orphaned Escrow row every time, permanently stranding those funds outside assignReward's reach): documents today's actual behavior — a second deposit funds a second escrow, but pool.escrowId stays pinned to the first, so the new escrow's funds become unreachable. This is the 'before' baseline MergeFi#48's fix needs to update.
…st-update regression for MergeFi#51 Covers assignReward's no-escrow and exceeds-balance rejections, and its happy path (releases the payment, decrements balance, forwards recipientId). Also adds a regression baseline for MergeFi#51 (MaintenancePool.balance is a hand-maintained running total with a lost-update race across concurrent deposit/assignReward calls): backs findOne()/save() with a single shared mutable record (mirroring a real Postgres row) and runs two concurrent assignReward calls via Promise.all. Confirmed this deterministically reproduces the race — both calls read the same starting balance before either writes back, so the final balance reflects only one of the two decrements (900 or 800) instead of the correct 700. This is the 'before' baseline MergeFi#51's fix (atomic update, lock, or transaction) needs to update.
…ame, upsertFromGithub, list Zero prior coverage on UsersService. Covers findById()'s NotFoundException and public-DTO mapping (private fields like email excluded), findByUsername(), upsertFromGithub()'s three paths (brand new user, existing user found by username, already-linked account token refresh) plus its null-refreshToken fallback, and list().
…for MergeFi#39 Covers addRole's add and already-has-role no-op cases, and setStellarAddress's NotFoundException and happy path. Also documents the scope boundary for MergeFi#39 (setStellarAddress is authenticated but not authorized): the IDOR lives in UsersController#setStellarAddress binding :id from the URL instead of the authenticated req.user.id -- UsersService.setStellarAddress itself has no caller/authorization concept in its signature to test against. A cross-user-rejection test belongs in a users.controller.spec.ts alongside MergeFi#39's actual fix; noted here rather than left implicit so the boundary isn't lost.
|
@davidishere1 is attempting to deploy a commit to the chonilius' projects Team on Vercel. A member of the Team first needs to authorize it. |
Contributor
|
checks passed |
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
TeamsService,MaintenancePoolService, andUsersServicehad zero test coverage despite directly controlling money movement (assignToBounty,deposit,assignReward) and payout-address changes (setStellarAddress) — even though every other exploitable bug this batch found lived in files that had no tests at all.Adds
teams.service.spec.ts,maintenance-pool.service.spec.ts, andusers.service.spec.ts, following this repo's existing house style (Test.createTestingModule+ mockedgetRepositoryToken(...)providers, no real DB — mirroringmilestones.service.spec.ts/sponsors.service.spec.ts).Regression baselines for the three companion bugs
Per the issue's requirement, at least one test per file documents a real bug found elsewhere in this batch, written against the current code so it's a meaningful "before" baseline for that bug's own fix PR:
assignToBountyhas no bounty-status or ownership guard): two tests confirm reassignment succeeds today regardless of the bounty's status or existing team assignment.depositcreates an orphanedEscrowrow on every deposit after the first): confirms a second deposit funds a second escrow viaescrowService.fund(), butpool.escrowIdstays pinned to the first — the second escrow's funds become unreachable throughassignReward.MaintenancePool.balancehas a lost-update race across concurrent calls): reproduces the race deterministically by backingfindOne()/save()with one shared mutable record (mirroring a real Postgres row) and running two concurrentassignRewardcalls viaPromise.all. Confirmed this reliably loses one of the two decrements (final balance is 900 or 800, never the correct 700).For #39 (
setStellarAddressIDOR),users.service.spec.tsdocuments why a cross-user-rejection test can't live at the service layer:UsersService.setStellarAddress(userId, address)has no caller/authorization concept in its own signature — the IDOR is entirely inUsersControllerbinding:idfrom the URL instead ofreq.user.id. Ausers.controller.spec.tswith that regression test belongs with #39's actual fix; noted explicitly here rather than faked at the wrong layer.Each regression test is commented with which companion issue it documents and what's expected to change once that issue's fix lands.
Acceptance criteria
teams.service.spec.ts,maintenance-pool.service.spec.ts, andusers.service.spec.tsexist with coverage of every public method on their respective servicesnpm run test:covshows 100% statement/line coverage on all three previously-untested files (was 0%)Test plan
npx jest— 173/173 passing across 22 suites (137 baseline + 36 new), run against local Postgres matching CI's service containernpm run lint— 0 errors (pre-existing unrelated warnings only)npm run build— passesnpm run test:cov— 100% stmts/funcs/lines onteams.service.ts,maintenance-pool.service.ts,users.service.tsCloses #61