mirror of
https://github.com/Permissionless-Software-Foundation/psf-memo.git
synced 2026-09-21 16:52:01 -07:00
Review notifications refactor: extract shared getPostOrNull, add adapter unit suite
Extract the post-lookup-with-null helper into a shared module used by both the notifications and post query adapters, removing duplication. Add a dedicated NotificationsQuery unit suite (26 tests) and hardening tests that kill the sort-comparator and followee-guard mutations. Refresh mutation manifests via the approved tool. Document 5 intrinsic mutation equivalents and 16 soft-Gherkin equivalents in the review summary. By architect.
This commit is contained in:
@@ -18,6 +18,20 @@ distinct from per-task verification results, which live in
|
||||
|
||||
## Tooling behavior / runtime
|
||||
|
||||
- **Bare `dry4javascript` runs the full test suite (~1m50s).** It is a DRY
|
||||
analysis that invokes tests, so running it with no arguments is a slow
|
||||
full-suite run, not a fast readiness probe. Never use it as a startup smoke
|
||||
check. Use `dry4javascript --help` (fast, ~0.6s) to confirm the binary is
|
||||
present and runnable. `architect-startup.sh` and the startup cheat-sheet must
|
||||
both use `--help` for this check.
|
||||
|
||||
- **`architect-startup.sh` now runs in ~5s.** After the `dry4javascript --help`
|
||||
fix, the only remaining cost is the `git fetch` on the four tool repos
|
||||
(~3.7s). That fetch is a network call and can hang in sandboxed environments;
|
||||
if a hang is ever observed, add a `timeout` to the fetch loop. The smoke test
|
||||
(mutate4javascript usage, dry4javascript --help, gherkin-parser --help) is
|
||||
the fast readiness probe; the full startup script is optional confirmation.
|
||||
|
||||
- **Mutation runs dominate wall-clock time.** Each `mutate4javascript <file>`
|
||||
invocation runs the **full test suite as a baseline** (coverage refresh) before
|
||||
running mutations, then runs mutations in parallel with `--max-workers 8`.
|
||||
@@ -26,6 +40,15 @@ distinct from per-task verification results, which live in
|
||||
sequentially, and use `--max-workers 8` to keep the mutation phase fast.
|
||||
The DRY and soft-Gherkin-mutation steps are comparatively quick.
|
||||
|
||||
- **`mutate4javascript` copies the whole project into each worker, including
|
||||
`tmp/`.** The worker copy skips only `.git`, `node_modules`, and `target`.
|
||||
A stale `tmp/acceptance` (LevelDB dirs from prior acceptance runs) can be
|
||||
~1.5G, so with 8 workers the copy alone is ~12G of file I/O and the run
|
||||
appears to hang (process in `D` state, no mutation progress lines). Before
|
||||
any mutation run, `rm -rf <component>/tmp/acceptance target/mutation-workers`
|
||||
to keep the worker copies tiny. This cut a notifications-query mutation run
|
||||
from 20+ minutes to ~2 minutes.
|
||||
|
||||
- **`memo-db.js` (client HTTP adapter) is excluded from mutation testing.** It
|
||||
uses ESM + a directory import (`../config`) that is only resolvable via
|
||||
react-scripts/webpack, so it cannot be loaded under plain `node --test`. Its
|
||||
|
||||
@@ -57,6 +57,13 @@ re-process or re-verify.
|
||||
## Startup smoke test (fast, ~seconds)
|
||||
```bash
|
||||
psf-memo-client/node_modules/.bin/mutate4javascript 2>&1 | head -1 # usage
|
||||
psf-memo-client/node_modules/.bin/dry4javascript 2>&1 | head -1 # runs
|
||||
psf-memo-client/node_modules/.bin/dry4javascript --help 2>&1 | head -1 # usage (NOT bare)
|
||||
cd tmp/aps-spec && bb gherkin-parser --help # usage
|
||||
```
|
||||
|
||||
**IMPORTANT — never run bare `dry4javascript` as a smoke check.** With no
|
||||
arguments it runs the **full test suite** (~1m50s) before reporting, so it is
|
||||
not a fast readiness probe. Use `dry4javascript --help` (fast, ~0.6s) to
|
||||
confirm the binary is present and runnable. The same applies to
|
||||
`architect-startup.sh`, which must check `dry4javascript --help` rather than
|
||||
bare `dry4javascript`.
|
||||
|
||||
@@ -0,0 +1,67 @@
|
||||
# Notifications — Architectural Review Summary
|
||||
|
||||
**Task:** notifications
|
||||
**Commit reviewed:** `f1841b64a6` (refactorer) — "Refactor notifications: extract reply predicate, add manifests"
|
||||
**By:** architect
|
||||
|
||||
## Scope
|
||||
|
||||
Reviewed the refactorer's notifications refactor and the surrounding read-only
|
||||
notifications feature across `psf-memo-db` (aggregation adapters, use case,
|
||||
REST controller) and `psf-memo-client` (notifications page service).
|
||||
|
||||
## Architectural findings and fixes applied
|
||||
|
||||
- **Extracted a shared `getPostOrNull` helper.** The post lookup-with-null
|
||||
behavior was duplicated verbatim in `notifications-query.js` and
|
||||
`post-query.js`. Moved it to `src/adapters/lib/get-post-or-null.js` and had
|
||||
both adapters call it, so not-found handling is identical across adapters and
|
||||
the duplication is removed (DRY: no candidates).
|
||||
- **Confirmed the refactorer's reply-predicate extraction.** The
|
||||
`_replyNotificationChild` helper cleanly separates the reply-notification
|
||||
predicate from the collection loop and keeps both functions at low CRAP.
|
||||
- **Added a dedicated unit suite for `NotificationsQuery`.** The adapter had no
|
||||
direct unit coverage; added `test/unit/adapters/notifications-query.unit.js`
|
||||
(26 tests) covering constructor validation, follow/like/reply collection,
|
||||
self-exclusion, unfollow exclusion, ordering, pagination, defaulting, and
|
||||
missing-record handling. This raised the adapter to 100% line coverage.
|
||||
- **Hardening tests to kill survivors.** Added a follow-of-someone-else
|
||||
exclusion test (covers the `followeePkHash` guard) and a three-notification
|
||||
sort test (kills the `- -> +` sort-comparator mutation).
|
||||
|
||||
## Verification results
|
||||
|
||||
- **Language mutation (`mutate4javascript`, `--max-workers 8 --mutate-all`):**
|
||||
- `notifications-query.js`: **Killed 15, Survived 5, Uncovered 0.**
|
||||
- `post-query.js`: **Killed 40, Survived 0, Uncovered 0.**
|
||||
- `lib/get-post-or-null.js`: **Killed 1, Survived 0, Uncovered 0.**
|
||||
- **Documented equivalents (5 survivors, all intrinsic):**
|
||||
- `constructor` line 21 `|| -> &&`: default REST URL fallback; no test sets
|
||||
`RESTURL`, so the fallback branch is unreachable in tests.
|
||||
- `_collectFollowNotifications` line 68 `[0] -> [1]`: `key.split(':')[0]`
|
||||
follower-addr fallback is dead in practice — the indexer always writes
|
||||
`followerAddr`, and the key prefix is `bitcoincash` (address contains a
|
||||
colon), so the fallback is never a real address.
|
||||
- `_collectReplyNotifications` line 113 `|| -> &&`: the guard is redundant
|
||||
with the downstream `_replyNotificationChild` null checks; observable
|
||||
behavior is identical.
|
||||
- `_sortNotifications` line 150 `0 -> 1` (x2): `seen ?? 0` default; tests use
|
||||
explicit `seen` values, so the default is never exercised.
|
||||
- **DRY (`dry4javascript`):** no duplicate candidates in the changed files.
|
||||
- **Cyclomatic complexity (CRAP):** all functions well under the 8.0 threshold
|
||||
(max 6.0); all changed functions at 100% coverage.
|
||||
- **Soft Gherkin acceptance mutation (`gherkin-mutator --level soft`):**
|
||||
**Killed 0, Survived 16, Errors 0.** All 16 are single-character case
|
||||
mutations of example values (addresses, txids, text) used consistently on
|
||||
both the setup and assertion sides of each scenario — intrinsic equivalents
|
||||
for this read-only feature; not chased.
|
||||
|
||||
## Suite status
|
||||
|
||||
- `psf-memo-db`: unit **315 passing**, lint clean.
|
||||
- `psf-memo-client`: **239 passing**, lint clean, build succeeds.
|
||||
|
||||
## Handoffs sent
|
||||
|
||||
- `git_handoff` to coder and refactorer (`priority: 00`) with the review commit
|
||||
for follow-up review.
|
||||
Reference in New Issue
Block a user