diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index ca386655..c30d2c06 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -41,6 +41,8 @@ Automated agents contributing to this repository should follow these rules: - **Commit Messages**: Use the convention `type(scope): description (agent)` (e.g., `fix(ai): formatting (agent)` or `docs(ai): update quickstart (agent)`). - **Reference Issues**: Link to relevant issues in the PR description. - **Review AGENTS.md**: Always consult `AGENTS.md` for specific architectural guidelines (like URL-first playback, token-first design system) and constraints before starting work. +- **Review KNOWN_ISSUES.md**: Before starting development or debugging, review `KNOWN_ISSUES.md` to avoid investigating pre-existing failures or limitations. +- **Dependency Upgrades**: Major or risky dependency upgrades must be documented by creating a Markdown file in the `issues/` directory (e.g., `issues/upgrade-.md`) with a plan and risk assessment, rather than opening an immediate PR. - **Efficiency**: Prefer running targeted or sharded tests (e.g., `npm run test:unit:shard1`, `shard2`, or `shard3`) to conserve resources during iteration. ## Submitting a Pull Request diff --git a/README.md b/README.md index aada60a8..4d6c0b64 100644 --- a/README.md +++ b/README.md @@ -181,6 +181,8 @@ To run **bitvid** locally: npm run lint ``` + _Tip: Use `npm run test:unit:shard1` (or `shard2`/`shard3`) for faster local feedback._ + #### Quick Reference - **Run unit tests**: `npm run test:unit` diff --git a/test-audit-report-2026-02-12.md b/test-audit-report-2026-02-12.md new file mode 100644 index 00000000..92c0d0d9 --- /dev/null +++ b/test-audit-report-2026-02-12.md @@ -0,0 +1,35 @@ +# Test Audit Report - 2026-02-12 + +## Summary +The unit test suite passed successfully (680 tests). A critical issue where `tests/auth-service.test.mjs` was skipped (resulting in 10% coverage for `authService.js`) was identified and fixed, raising coverage to 62%. Several critical production files remain with low coverage. + +## Test Run Details +* **Command:** `node --import ./tests/test-helpers/setup-localstorage.mjs --test --experimental-test-coverage tests/**/*.test.mjs` +* **Result:** 680 passed, 0 failed. +* **Duration:** ~42s + +## Critical Coverage Gaps +The following critical files have low coverage (< 50%): + +| File | Line Coverage | Function Coverage | Impact | +|------|--------------|-------------------|--------| +| `js/webtorrent.js` | 12.25% | 4.00% | **Critical** (Playback core) | +| `js/userBlocks.js` | 18.92% | 26.47% | **High** (Moderation) | +| `js/nostr/nip46Client.js` | 27.42% | 44.59% | **High** (Remote Signing) | +| `js/views/VideoListView.js` | 48.81% | 36.04% | **Medium** (Main Feed UI) | + +## Fixed Issues +1. **Auth Service Tests Skipped:** `tests/auth-service.test.mjs` was using raw async blocks without `node:test` imports, causing the runner to skip it. + * **Fix:** Refactored to use `describe`/`test` from `node:test`. + * **Result:** `js/services/authService.js` coverage improved from 10% to 62%. +2. **Console Noise:** Removed a leftover `console.log` in `tests/watchHistory/watch-history-telemetry.test.mjs`. + +## Static Analysis Findings +* **Time Dependency:** Extensive use of `setTimeout` found in `tests/nostr/client.test.mjs`, `tests/moderation/video-card.test.mjs`, and others. These are potential sources of flakiness. +* **Skipped Tests:** `tests/nostr/sessionActor.test.mjs` skips a test when WebCrypto is unavailable. +* **Network Calls:** No unmocked external network calls detected in unit tests (mocks appear to be used). + +## Recommendations +1. **Prioritize Webtorrent Tests:** Add unit tests for `js/webtorrent.js` to cover the torrent client wrapper logic. +2. **Moderation Coverage:** Expand `tests/user-blocks.test.mjs` (if it exists) or create it to cover `js/userBlocks.js`. +3. **Refactor Timeouts:** Replace `await new Promise(r => setTimeout(r, N))` with deterministic `waitFor` helpers in `tests/nostr/client.test.mjs` to reduce flakiness risk. diff --git a/tests/auth-service.test.mjs b/tests/auth-service.test.mjs index 3fb3a89b..e50cc15a 100644 --- a/tests/auth-service.test.mjs +++ b/tests/auth-service.test.mjs @@ -2,6 +2,7 @@ import "./test-helpers/setup-localstorage.mjs"; import assert from "node:assert/strict"; +import { test, describe, beforeEach } from "node:test"; import AuthService from "../js/services/authService.js"; import { @@ -70,162 +71,134 @@ function resetState() { } } -await (async () => { - resetState(); - setPubkey(SAMPLE_PUBKEY); +describe("AuthService", () => { + beforeEach(resetState); - const calls = []; - let resolveBlocks; + test("applyPostLoginState success flow", async () => { + setPubkey(SAMPLE_PUBKEY); - const service = new AuthService({ - userBlocks: { - loadBlocks: () => { - calls.push("blocks:start"); - return new Promise((resolve) => { - resolveBlocks = resolve; - }); + const calls = []; + let resolveBlocks; + + const service = new AuthService({ + userBlocks: { + loadBlocks: () => { + calls.push("blocks:start"); + return new Promise((resolve) => { + resolveBlocks = resolve; + }); + }, }, - }, - relayManager: { - loadRelayList: () => { - calls.push("relays:start"); - return Promise.resolve("relays"); + relayManager: { + loadRelayList: () => { + calls.push("relays:start"); + return Promise.resolve("relays"); + }, }, - }, + }); + + service.loadOwnProfile = () => { + calls.push("profile:start"); + return Promise.resolve({ name: "Test User" }); + }; + + const promise = service.applyPostLoginState(); + + await new Promise((resolve) => setTimeout(resolve, 10)); + + assert.deepEqual(calls, ["relays:start", "blocks:start", "profile:start"]); + + resolveBlocks(); + + const result = await promise; + const detail = result.detail; + await result.completionPromise; + + assert.equal(detail.pubkey, SAMPLE_PUBKEY); + assert.equal(detail.blocksLoaded, true); + assert.equal(detail.relaysLoaded, true); + assert.deepEqual(detail.profile, { name: "Test User" }); }); - service.loadOwnProfile = () => { - calls.push("profile:start"); - return Promise.resolve({ name: "Test User" }); - }; + test("applyPostLoginState with deferBlocks: true", async () => { + setPubkey(SAMPLE_PUBKEY); - const promise = service.applyPostLoginState(); + const calls = []; + // We leave resolveBlocks undefined/pending forever to simulate slow/hanging blocks + let resolveBlocks; - await new Promise((resolve) => setTimeout(resolve, 10)); - - assert.deepEqual(calls, ["relays:start", "blocks:start", "profile:start"]); - - resolveBlocks(); - - const result = await promise; - const detail = result.detail; - await result.completionPromise; - - assert.equal(detail.pubkey, SAMPLE_PUBKEY); - assert.equal(detail.blocksLoaded, true); - assert.equal(detail.relaysLoaded, true); - assert.deepEqual(detail.profile, { name: "Test User" }); -})(); - -await (async () => { - resetState(); - setPubkey(SAMPLE_PUBKEY); - - const calls = []; - // We leave resolveBlocks undefined/pending forever to simulate slow/hanging blocks - let resolveBlocks; - - const service = new AuthService({ - userBlocks: { - loadBlocks: () => { - calls.push("blocks:start"); - return new Promise((resolve) => { - resolveBlocks = resolve; - }); + const service = new AuthService({ + userBlocks: { + loadBlocks: () => { + calls.push("blocks:start"); + return new Promise((resolve) => { + resolveBlocks = resolve; + }); + }, }, - }, - relayManager: { - loadRelayList: () => { - calls.push("relays:start"); - return Promise.resolve("relays"); + relayManager: { + loadRelayList: () => { + calls.push("relays:start"); + return Promise.resolve("relays"); + }, }, - }, + }); + + service.loadOwnProfile = () => { + calls.push("profile:start"); + return Promise.resolve({ name: "Test User" }); + }; + + // Test deferBlocks: true + const promise = service.applyPostLoginState({ deferBlocks: true }); + + await new Promise((resolve) => setTimeout(resolve, 10)); + + // Blocks should NOT be started if deferred + // Wait, deferBlocks just means it's not awaited in completionPromise? + // No, checking code: + // if (!deferBlocks) { const blocksOperation = this.createBlocksLoadOperation... } + // So it shouldn't even be called/pushed to concurrentOps. + + assert.deepEqual(calls, ["relays:start", "profile:start"]); + + const result = await promise; + // This should resolve even if blocks are not loaded + await result.completionPromise; + + assert.equal(result.detail.blocksLoaded, null); + assert.equal(result.detail.relaysLoaded, true); }); - service.loadOwnProfile = () => { - calls.push("profile:start"); - return Promise.resolve({ name: "Test User" }); - }; + test("applyPostLoginState partial failure", async () => { + setPubkey(SAMPLE_PUBKEY); - // Test deferBlocks: true - const promise = service.applyPostLoginState({ deferBlocks: true }); - - await new Promise((resolve) => setTimeout(resolve, 10)); - - // Blocks should NOT be started if deferred - // Wait, deferBlocks just means it's not awaited in completionPromise? - // No, checking code: - // if (!deferBlocks) { const blocksOperation = this.createBlocksLoadOperation... } - // So it shouldn't even be called/pushed to concurrentOps. - - assert.deepEqual(calls, ["relays:start", "profile:start"]); - - const result = await promise; - // This should resolve even if blocks are not loaded - await result.completionPromise; - - assert.equal(result.detail.blocksLoaded, null); - assert.equal(result.detail.relaysLoaded, true); -})(); - -await (async () => { - resetState(); - setPubkey(SAMPLE_PUBKEY); - - const logs = []; - const service = new AuthService({ - userBlocks: { - loadBlocks: () => { - throw new Error("blocks failed synchronously"); + const logs = []; + const service = new AuthService({ + userBlocks: { + loadBlocks: () => { + throw new Error("blocks failed synchronously"); + }, }, - }, - relayManager: { - loadRelayList: () => Promise.reject(new Error("relays failed")), - }, - }); + relayManager: { + loadRelayList: () => Promise.reject(new Error("relays failed")), + }, + }); - service.log = (message, error) => { - logs.push({ message, error }); - }; + service.log = (message, error) => { + logs.push({ message, error }); + }; - service.loadOwnProfile = () => Promise.reject(new Error("profile failed")); + service.loadOwnProfile = () => Promise.reject(new Error("profile failed")); - const result = service.applyPostLoginState(); - const detail = result.detail; - await result.completionPromise; + const result = service.applyPostLoginState(); + const detail = result.detail; + await result.completionPromise; - assert.equal(detail.pubkey, SAMPLE_PUBKEY); - assert.equal(detail.blocksLoaded, false); - assert.equal(detail.relaysLoaded, false); - assert.deepEqual(detail.profile, { - name: "Unknown", - picture: "assets/svg/default-profile.svg", - about: "", - website: "", - banner: "", - lud16: "", - lud06: "", - }); - - assert.equal(logs.length, 3); - assert(logs.some(({ message }) => message.includes("block list"))); - assert(logs.some(({ message }) => message.includes("relay list"))); - assert(logs.some(({ message }) => message.includes("own profile"))); -})(); - -await (async () => { - resetState(); - - const service = new AuthService(); - const result = service.applyPostLoginState(); - const detail = result.detail; - await result.completionPromise; - - assert.deepEqual(detail, { - pubkey: null, - blocksLoaded: false, - relaysLoaded: false, - profile: { + assert.equal(detail.pubkey, SAMPLE_PUBKEY); + assert.equal(detail.blocksLoaded, false); + assert.equal(detail.relaysLoaded, false); + assert.deepEqual(detail.profile, { name: "Unknown", picture: "assets/svg/default-profile.svg", about: "", @@ -233,170 +206,176 @@ await (async () => { banner: "", lud16: "", lud06: "", - }, - }); -})(); + }); -await (async () => { - resetState(); - - const service = new AuthService(); - const postLogin = { - pubkey: SAMPLE_PUBKEY, - blocksLoaded: true, - relaysLoaded: false, - profile: { name: "Stub" }, - }; - - service.applyPostLoginState = () => ({ - detail: postLogin, - completionPromise: Promise.resolve(postLogin), + assert.equal(logs.length, 3); + assert(logs.some(({ message }) => message.includes("block list"))); + assert(logs.some(({ message }) => message.includes("relay list"))); + assert(logs.some(({ message }) => message.includes("own profile"))); }); - const detail = await service.login(SAMPLE_PUBKEY); + test("applyPostLoginState without pubkey", async () => { + const service = new AuthService(); + const result = service.applyPostLoginState(); + const detail = result.detail; + await result.completionPromise; - await detail.postLoginPromise; - - assert.equal(detail.postLogin, postLogin); - assert.deepEqual(detail.postLogin, postLogin); -})(); - -await (async () => { - resetState(); - - const adminCheckCalls = []; - const service = new AuthService({ - accessControl: { - isLockdownActive: () => true, - isAdminEditor: (npub) => { - adminCheckCalls.push(npub); - return false; + assert.deepEqual(detail, { + pubkey: null, + blocksLoaded: false, + relaysLoaded: false, + profile: { + name: "Unknown", + picture: "assets/svg/default-profile.svg", + about: "", + website: "", + banner: "", + lud16: "", + lud06: "", }, - }, + }); }); - let error; - try { - await service.login(SAMPLE_PUBKEY); - } catch (err) { - error = err; - } - - assert(error instanceof Error); - assert.equal(error.code, "site-lockdown"); - assert.match(error.message, /locked down/i); - assert.equal(adminCheckCalls.length, 1); - assert.equal(typeof adminCheckCalls[0], "string"); - assert(adminCheckCalls[0].length > 0); -})(); - -await (async () => { - resetState(); - - const service = new AuthService({ - accessControl: { - isLockdownActive: () => true, - isAdminEditor: () => true, - }, - }); - - const detail = await service.login(SAMPLE_PUBKEY); - - assert.ok(detail); - assert.equal(detail.pubkey, SAMPLE_PUBKEY); -})(); - -// === Tests for loadOwnProfile logic === - -await (async () => { - resetState(); - const PUBKEY = SAMPLE_PUBKEY; - - // Test Fast Relay Success - { - const mockNostrClient = { - relays: ["wss://fast1.com", "wss://fast2.com", "wss://slow.com"], - pool: { - list: async (relays) => { - const url = relays[0]; - if (url === "wss://fast1.com") { - return [{ - pubkey: PUBKEY, - created_at: 100, - content: JSON.stringify({ name: "Fast User" }) - }]; - } - return new Promise(() => {}); // Hang others - } - }, - handleEvent: () => {} + test("login mocks postLogin flow", async () => { + const service = new AuthService(); + const postLogin = { + pubkey: SAMPLE_PUBKEY, + blocksLoaded: true, + relaysLoaded: false, + profile: { name: "Stub" }, }; - const service = new AuthService({ nostrClient: mockNostrClient }); - // Override setProfileCacheEntry to avoid side effects or errors from real implementation if not needed, - // but here we just let it run. However, the service needs nostrClient.pool + service.applyPostLoginState = () => ({ + detail: postLogin, + completionPromise: Promise.resolve(postLogin), + }); - // We need to make sure setProfileCacheEntry doesn't fail. - // It imports from cache.js which uses profileCache. - // profileCache is imported from state/profileCache.js. + const detail = await service.login(SAMPLE_PUBKEY); - // The previous tests didn't seem to mock setProfileCacheEntry. + await detail.postLoginPromise; - const profile = await service.loadOwnProfile(PUBKEY); - assert.equal(profile.name, "Fast User"); - } + assert.equal(detail.postLogin, postLogin); + assert.deepEqual(detail.postLogin, postLogin); + }); - // Test All Relays Fail - { - const mockNostrClient = { - relays: ["wss://fail1.com", "wss://fail2.com"], - pool: { - list: async () => { - throw new Error("Failed"); - } + test("login fails on lockdown", async () => { + const adminCheckCalls = []; + const service = new AuthService({ + accessControl: { + isLockdownActive: () => true, + isAdminEditor: (npub) => { + adminCheckCalls.push(npub); + return false; + }, }, - handleEvent: () => {} - }; + }); - const service = new AuthService({ nostrClient: mockNostrClient }); - const profile = await service.loadOwnProfile(PUBKEY); - assert.equal(profile.name, "Unknown"); - } + let error; + try { + await service.login(SAMPLE_PUBKEY); + } catch (err) { + error = err; + } - // Test Timeout Fallback - { - let backgroundResolve; - const backgroundPromise = new Promise(r => backgroundResolve = r); + assert(error instanceof Error); + assert.equal(error.code, "site-lockdown"); + assert.match(error.message, /locked down/i); + assert.equal(adminCheckCalls.length, 1); + assert.equal(typeof adminCheckCalls[0], "string"); + assert(adminCheckCalls[0].length > 0); + }); - const mockNostrClient = { - relays: ["wss://fast-fail.com", "wss://fast-fail2.com", "wss://background-ok.com", "wss://background-ok-2.com"], + test("login succeeds for admin on lockdown", async () => { + const service = new AuthService({ + accessControl: { + isLockdownActive: () => true, + isAdminEditor: () => true, + }, + }); + + const detail = await service.login(SAMPLE_PUBKEY); + + assert.ok(detail); + assert.equal(detail.pubkey, SAMPLE_PUBKEY); + }); + + describe("loadOwnProfile", () => { + const PUBKEY = SAMPLE_PUBKEY; + + test("Fast Relay Success", async () => { + const mockNostrClient = { + relays: ["wss://fast1.com", "wss://fast2.com", "wss://slow.com"], pool: { - list: async (relays) => { - const url = relays[0]; - if (url.includes("fast")) { - // Simulate timeout or error - throw new Error("Fast fail"); - } - if (url.includes("background")) { - await backgroundPromise; - return [{ - pubkey: PUBKEY, - created_at: 200, - content: JSON.stringify({ name: "Background User" }) - }]; - } - return []; + list: async (relays) => { + const url = relays[0]; + if (url === "wss://fast1.com") { + return [{ + pubkey: PUBKEY, + created_at: 100, + content: JSON.stringify({ name: "Fast User" }) + }]; } + return new Promise(() => {}); // Hang others + } }, handleEvent: () => {} - }; + }; - const service = new AuthService({ nostrClient: mockNostrClient }); + const service = new AuthService({ nostrClient: mockNostrClient }); + const profile = await service.loadOwnProfile(PUBKEY); + assert.equal(profile.name, "Fast User"); + }); - const profile = await service.loadOwnProfile(PUBKEY); - assert.equal(profile.name, "Unknown"); + test("All Relays Fail", async () => { + const mockNostrClient = { + relays: ["wss://fail1.com", "wss://fail2.com"], + pool: { + list: async () => { + throw new Error("Failed"); + } + }, + handleEvent: () => {} + }; - // We resolve background promise to let background logic finish eventually (though test finishes here) - backgroundResolve(); - } -})(); + const service = new AuthService({ nostrClient: mockNostrClient }); + const profile = await service.loadOwnProfile(PUBKEY); + assert.equal(profile.name, "Unknown"); + }); + + test("Timeout Fallback", async () => { + let backgroundResolve; + const backgroundPromise = new Promise(r => backgroundResolve = r); + + const mockNostrClient = { + relays: ["wss://fast-fail.com", "wss://fast-fail2.com", "wss://background-ok.com", "wss://background-ok-2.com"], + pool: { + list: async (relays) => { + const url = relays[0]; + if (url.includes("fast")) { + // Simulate timeout or error + throw new Error("Fast fail"); + } + if (url.includes("background")) { + await backgroundPromise; + return [{ + pubkey: PUBKEY, + created_at: 200, + content: JSON.stringify({ name: "Background User" }) + }]; + } + return []; + } + }, + handleEvent: () => {} + }; + + const service = new AuthService({ nostrClient: mockNostrClient }); + + const profile = await service.loadOwnProfile(PUBKEY); + assert.equal(profile.name, "Unknown"); + + // We resolve background promise to let background logic finish eventually (though test finishes here) + backgroundResolve(); + }); + }); +}); diff --git a/tests/watchHistory/watch-history-telemetry.test.mjs b/tests/watchHistory/watch-history-telemetry.test.mjs index 9beaec5c..fedef8b7 100644 --- a/tests/watchHistory/watch-history-telemetry.test.mjs +++ b/tests/watchHistory/watch-history-telemetry.test.mjs @@ -221,4 +221,3 @@ await testPreparePlaybackLoggingPublishesOnce(); await testPreparePlaybackLoggingIgnoresMetadata(); await testCancelPlaybackLoggingFlushes(); -console.log("watch-history-telemetry tests passed");