From 35151a5fb0ca5d0dafe152d59e788cf25c6d936e Mon Sep 17 00:00:00 2001 From: Roland Date: Thu, 30 Jul 2026 16:30:49 -0600 Subject: [PATCH] test: one place for fixtures, real providers instead of mocked context modules Specs had no shared vocabulary for the app's data or its contexts, so each one rebuilt both. A settings object was written out by hand wherever one was needed, twenty-odd keys at a time, and nothing held those copies to each other or to the API. Contexts were worse: to render a consumer a spec mocked the whole context module, which replaces every export including the providers, so each spec reinvented the pieces it still needed. One supplies a `Media` that always reports desktop -- quietly leaving that component's mobile branch untested -- and one hand-writes a context as a bare `_currentValue` with no provider, working only because useContext reads that React internal. test-fixtures.js is now the single definition of both, shared by jest, the Cypress component specs, and e2e intercept bodies, so the three cannot disagree about what the API returns. Every fixture is a function taking overrides; a shared mutable object lets one test change what the next one sees. The drift this prevents was already real. The theme fixture supplied `i`, `s` and `t` holding hardcoded Semantic greys for months after ThemeProvider stopped supplying them, and ThemeContext still declared all four. So the fixture agreed with the tests, and with nothing else: FileBrowser was reading `inverted` off the context and pasting the resulting `undefined` into a className, which is why the file browser's footer -- near-white #f9fafb, with a `.inverted` dark variant that nothing could select any more -- stayed light in dark, night and amber. A white bar pinned to the bottom of the screen in night mode costs the user exactly the dark adaptation the theme exists to protect. It takes tokens now. test-fixtures.test.js holds each fixture to its context's declared shape in both directions, so a property added to one and forgotten in the other fails a test. Two failed attempts are recorded in test-utils.js because both were instructive. Importing Tags.js and FileWorkerStatusContext.js so the harness could supply those providers broke eight suites: it put both into the module graph of every spec ahead of that spec's own mocks, and components bound to a second instance of a mocked module -- a different jest.fn() than the test had configured, so a hook mocked to return a value returned undefined, in a file no test mentions. Requiring them lazily fixed that and not its mirror image: useContext read a different TagsContext than the harness had required, so the provider was ignored and the context default won. A harness must not change what a spec imports, so it owns only contexts/contexts.js and a spec passes any other context itself. DomainTagging is converted off the `_currentValue` fake as proof, and now asserts the tag name a user sees rather than a test id its own mock invented. The five remaining specs are listed in a ratchet so the count can only fall. 974 jest, 15 browser component, 46 e2e, clean cache-busted build, tsc clean. Fixtures are confirmed absent from the shipped bundle. Co-Authored-By: Claude Opus 5 (1M context) --- .../e2e/settings/download-window.cy.js | 48 +-- .../e2e/settings/special-directories.cy.js | 46 +-- app/src/App.css | 19 +- app/src/components/DomainTagging.test.js | 26 +- app/src/components/FileBrowser.js | 4 +- app/src/contexts/FileWorkerStatusContext.js | 8 +- app/src/contexts/contexts.js | 14 +- app/src/test-fixtures.js | 190 ++++++++++++ app/src/test-fixtures.test.js | 161 ++++++++++ app/src/test-utils.js | 275 +++++++++++------- 10 files changed, 570 insertions(+), 221 deletions(-) create mode 100644 app/src/test-fixtures.js create mode 100644 app/src/test-fixtures.test.js diff --git a/app/cypress/e2e/settings/download-window.cy.js b/app/cypress/e2e/settings/download-window.cy.js index 94e373ea..448dc00c 100644 --- a/app/cypress/e2e/settings/download-window.cy.js +++ b/app/cypress/e2e/settings/download-window.cy.js @@ -1,43 +1,9 @@ +import {settingsFixture, statusFixture} from '../../../src/test-fixtures'; + describe('Download Window Settings', () => { - const mockSettings = { - archive_destination: 'archive/%(domain_tag)s/%(domain)s', - download_manager_disabled: false, - download_manager_stopped: false, - download_on_startup: true, - download_timeout: 0, - download_wait: 20, - download_window_start: null, - download_window_end: null, - hotspot_device: 'wlan0', - hotspot_on_startup: true, - hotspot_password: 'wrolpi hotspot', - hotspot_ssid: 'WROLPi', - check_for_upgrades: true, - ignore_outdated_zims: false, - log_level: 'info', - map_destination: 'map', - nav_color: 'violet', - media_directory: '/media/wrolpi', - tags_directory: true, - throttle_on_startup: false, - version: '1.0.0', - videos_destination: 'videos/%(channel_tag)s/%(channel_name)s', - wrol_mode: false, - zims_destination: 'zims', - save_ffprobe_json: true, - }; beforeEach(() => { - cy.intercept('GET', '/api/status', { - statusCode: 200, - body: { - version: '1.0.0', - flags: {}, - cpu_percent: 10, - memory_percent: 30, - downloads: {pending: 0, recurring: 0, disabled: false, stopped: false, outside_download_window: false}, - } - }).as('getStatus'); + cy.intercept('GET', '/api/status', {statusCode: 200, body: statusFixture()}).as('getStatus'); cy.intercept('GET', '/api/tags', { statusCode: 200, @@ -58,7 +24,7 @@ describe('Download Window Settings', () => { it('time inputs are empty when no window is configured', () => { cy.intercept('GET', '/api/settings', { statusCode: 200, - body: {...mockSettings}, + body: settingsFixture(), }).as('getSettings'); cy.visit('/admin/settings'); @@ -73,7 +39,7 @@ describe('Download Window Settings', () => { cy.intercept('GET', '/api/settings', { statusCode: 200, body: { - ...mockSettings, + ...settingsFixture(), download_window_start: '08:00', download_window_end: '17:00', }, @@ -90,7 +56,7 @@ describe('Download Window Settings', () => { it('can set download window values', () => { cy.intercept('GET', '/api/settings', { statusCode: 200, - body: {...mockSettings}, + body: settingsFixture(), }).as('getSettings'); cy.visit('/admin/settings'); @@ -107,7 +73,7 @@ describe('Download Window Settings', () => { cy.intercept('GET', '/api/settings', { statusCode: 200, body: { - ...mockSettings, + ...settingsFixture(), download_window_start: '22:00', download_window_end: '06:00', }, diff --git a/app/cypress/e2e/settings/special-directories.cy.js b/app/cypress/e2e/settings/special-directories.cy.js index 81da1294..0a9e248d 100644 --- a/app/cypress/e2e/settings/special-directories.cy.js +++ b/app/cypress/e2e/settings/special-directories.cy.js @@ -9,35 +9,9 @@ * Intercepting the API is not mocking the component -- the page really renders, so the * accessible names and the real geometry are the ones a user gets. */ +import {settingsFixture, statusFixture} from '../../../src/test-fixtures'; + describe('Special Directories settings', () => { - const mockSettings = { - archive_destination: 'archive/%(domain_tag)s/%(domain)s', - download_manager_disabled: false, - download_manager_stopped: false, - download_on_startup: true, - download_timeout: 0, - download_wait: 20, - download_window_start: null, - download_window_end: null, - hotspot_device: 'wlan0', - hotspot_on_startup: true, - hotspot_password: 'wrolpi hotspot', - hotspot_ssid: 'WROLPi', - check_for_upgrades: true, - ignore_outdated_zims: false, - log_level: 'info', - map_destination: 'map', - nav_color: 'violet', - media_directory: '/media/wrolpi', - playlists_destination: 'playlists', - tags_directory: true, - throttle_on_startup: false, - version: '1.0.0', - videos_destination: 'videos/%(channel_tag)s/%(channel_name)s', - wrol_mode: false, - zims_destination: 'zims', - save_ffprobe_json: true, - }; const directories = [ ['Archive Directory', 'archive/%(domain_tag)s/%(domain)s'], @@ -48,26 +22,14 @@ describe('Special Directories settings', () => { ]; beforeEach(() => { - cy.intercept('GET', '/api/status', { - statusCode: 200, - body: { - version: '1.0.0', - flags: {}, - cpu_percent: 10, - memory_percent: 30, - downloads: { - pending: 0, recurring: 0, disabled: false, stopped: false, - outside_download_window: false, - }, - }, - }).as('getStatus'); + cy.intercept('GET', '/api/status', {statusCode: 200, body: statusFixture()}).as('getStatus'); cy.intercept('GET', '/api/tags', {statusCode: 200, body: {tags: []}}).as('getTags'); cy.intercept('GET', '/api/events', {statusCode: 200, body: {events: []}}).as('getEvents'); cy.intercept('POST', '/api/search/suggestions', { statusCode: 200, body: {fileGroups: 0, zimsEstimates: [], channels: [], domains: []}, }).as('getSuggestions'); - cy.intercept('GET', '/api/settings', {statusCode: 200, body: {...mockSettings}}) + cy.intercept('GET', '/api/settings', {statusCode: 200, body: settingsFixture()}) .as('getSettings'); cy.visit('/admin/settings'); diff --git a/app/src/App.css b/app/src/App.css index 7dee8bee..0d741fa4 100644 --- a/app/src/App.css +++ b/app/src/App.css @@ -466,23 +466,26 @@ table.ratio-table td.min-width { animation: skeleton-shimmer 1.5s infinite; } -/* Sticky footer for file browser */ +/* + * Sticky footer for the file browser. + * + * These were two hardcoded Semantic greys with `.inverted` selecting the dark one, but + * nothing has set `.inverted` since ThemeProvider stopped supplying it -- so the footer stayed + * at near-white `#f9fafb` in dark, night and amber. In night mode a white bar pinned to the + * bottom of the screen costs the user the dark adaptation the whole theme exists to protect. + * Tokens have no such failure mode: there is one rule and each theme fills it in. + */ .sticky-footer { position: fixed; bottom: 0; left: 0; right: 0; padding: 0.5em 1em; - background-color: #f9fafb; - border-top: 1px solid rgba(34, 36, 38, 0.15); + background-color: var(--panel); + border-top: 1px solid var(--border); z-index: 100; } -.sticky-footer.inverted { - background-color: #1b1c1d; - border-top: 1px solid rgba(255, 255, 255, 0.1); -} - /* Drag selection visual feedback for FileBrowser */ tr.drag-selecting td { background-color: rgba(33, 133, 208, 0.15) !important; diff --git a/app/src/components/DomainTagging.test.js b/app/src/components/DomainTagging.test.js index 1c038ff2..354954c7 100644 --- a/app/src/components/DomainTagging.test.js +++ b/app/src/components/DomainTagging.test.js @@ -1,16 +1,17 @@ import React from 'react'; import {createMockDomain, createTestForm, render, screen} from '../test-utils'; +import {tagsContextFixture} from '../test-fixtures'; +import {TagsContext} from '../Tags'; import {CollectionEditForm} from './collections/CollectionEditForm'; import {DomainsPage} from './Archive'; -// Mock the TagsContext -jest.mock('../Tags', () => ({ - TagsContext: { - _currentValue: { - SingleTag: ({name}) => {name} - } - }, -})); +/* + * The Tags context used to be faked here as a bare object with a `_currentValue` and no + * `.Provider`, which worked only because `useContext` reads that React internal. It is now + * the real context carrying a fixture value, passed in by this spec rather than by the harness + * so that it is the same module instance the component reads -- see test-utils.js. + */ +const withTags = {contexts: [[TagsContext, tagsContextFixture()]]}; // Mock the DirectorySearch and DestinationForm components jest.mock('./Common', () => ({ @@ -96,7 +97,8 @@ describe('Domain Tagging Logic', () => { appliedTagName="News" >
Child content
- + , + withTags, ); // Should render title @@ -105,8 +107,8 @@ describe('Domain Tagging Logic', () => { // Should render children expect(screen.getByTestId('child-content')).toBeInTheDocument(); - // Should render applied tag - expect(screen.getByTestId('applied-tag')).toHaveTextContent('News'); + // The tag name a user would see, rather than a test id the old mock invented. + expect(screen.getByText('News')).toBeInTheDocument(); // Should render Save button expect(screen.getByRole('button', {name: /save/i})).toBeInTheDocument(); @@ -148,7 +150,7 @@ describe('Domain Tagging Logic', () => { ); - expect(screen.queryByTestId('applied-tag')).not.toBeInTheDocument(); + expect(screen.queryByText('News')).not.toBeInTheDocument(); }); }); diff --git a/app/src/components/FileBrowser.js b/app/src/components/FileBrowser.js index d43bc1c1..9a617b10 100644 --- a/app/src/components/FileBrowser.js +++ b/app/src/components/FileBrowser.js @@ -266,7 +266,6 @@ export function FileBrowser() { }, {enableOnFormTags: false}); const {settings, fetchSettings} = React.useContext(SettingsContext); - const {inverted} = React.useContext(ThemeContext); const selectedPathsCount = selectedPaths ? selectedPaths.length : 0; @@ -397,8 +396,7 @@ export function FileBrowser() { // Show a label alongside the icon once there's room; otherwise the icon carries the meaning alone. const label = (text) => showLabels ? text : null; - const footerClassName = `sticky-footer ${inverted}`; - const footer =
+ const footer =
{ }; /** @type {React.Context} */ +/* + * The default is the declared shape of this context, and test-fixtures.test.js holds the + * fixtures to it key for key. It used to carry `i`, `s`, `t` and `inverted` -- Semantic + * compatibility props holding hardcoded greys -- long after ThemeProvider stopped supplying + * them, so the context advertised four properties that were always undefined in a real tree. + * FileBrowser was still reading one of them. + */ export const ThemeContext = React.createContext({ theme: 'light', - i: {}, - s: {}, - t: {}, - inverted: '', savedTheme: null, isDark: false, + mediaFilter: undefined, + mediaFilterEnabled: false, + setMediaFilterEnabled: () => {}, setTheme: () => {}, setDarkTheme: () => {}, setLightTheme: () => {}, diff --git a/app/src/test-fixtures.js b/app/src/test-fixtures.js new file mode 100644 index 00000000..3b7df007 --- /dev/null +++ b/app/src/test-fixtures.js @@ -0,0 +1,190 @@ +/** + * Shared test fixtures: the data and context values the app expects, in one place. + * + * Two problems this exists to solve. + * + * The first is duplication. A settings object was written out by hand in every spec that + * needed one, twenty-odd keys at a time, so a spec asserting one field still had to know and + * restate the other nineteen. Nothing kept those copies in step with each other or with the + * API, and adding a required field meant hunting them down. + * + * The second is drift, which is the worse of the two. A fixture that no longer matches the + * shape it stands in for makes a test *confidently* wrong: it passes while the component + * would break in the browser. The theme context fixture carried `i`, `s` and `t` with + * hardcoded Semantic greys for months after ThemeProvider stopped supplying them, and one + * component was still reading a prop that no longer existed. test-fixtures.test.js compares + * these against the real contexts so that class of drift fails a test rather than surviving. + * + * Everything here is a function taking overrides, never an exported constant: a shared mutable + * object lets one test change what the next one sees, and that failure is miserable to find. + * + * Importable from jest tests, Cypress component specs, and e2e `cy.intercept` bodies alike -- + * the same fixture in all three, so they cannot disagree about what the API returns. + */ + +import {defaultTheme} from './themes/names'; + +/* --------------------------------------------------------------------------- */ +/* API shapes */ +/* --------------------------------------------------------------------------- */ + +/** GET /api/settings */ +export const settingsFixture = (overrides = {}) => ({ + archive_destination: 'archive/%(domain_tag)s/%(domain)s', + check_for_upgrades: true, + download_manager_disabled: false, + download_manager_stopped: false, + download_on_startup: true, + download_timeout: 0, + download_wait: 20, + download_window_start: null, + download_window_end: null, + hotspot_device: 'wlan0', + hotspot_on_startup: true, + hotspot_password: 'wrolpi hotspot', + hotspot_ssid: 'WROLPi', + ignore_outdated_zims: false, + log_level: 'info', + map_destination: 'map', + // `/media/wrolpi` on a Pi, but the API is the authority on this and a test that + // hardcodes it elsewhere is asserting its own guess. + media_directory: '/media/wrolpi', + nav_color: 'violet', + playlists_destination: 'playlists', + save_ffprobe_json: true, + tags_directory: true, + throttle_on_startup: false, + version: '1.0.0', + videos_destination: 'videos/%(channel_tag)s/%(channel_name)s', + wrol_mode: false, + zims_destination: 'zims', + ...overrides, +}); + +/** GET /api/status */ +export const statusFixture = (overrides = {}) => ({ + version: '1.0.0', + flags: {}, + cpu_percent: 10, + memory_percent: 30, + downloads: { + pending: 0, + recurring: 0, + disabled: false, + stopped: false, + outside_download_window: false, + }, + ...overrides, +}); + +/** A Domain Collection, as the domains endpoints return it. */ +export const domainFixture = (overrides = {}) => ({ + id: 1, + domain: 'example.com', + archive_count: 42, + size: 1024000, + tag_name: null, + directory: '', + can_be_tagged: false, + description: '', + ...overrides, +}); + +export const domainsFixture = (count = 3) => Array.from({length: count}, (_, i) => domainFixture({ + id: i + 1, + domain: `example${i + 1}.com`, + archive_count: (i + 1) * 10, + size: (i + 1) * 1000000, +})); + +export const tagFixture = (overrides = {}) => ({ + id: 1, + name: 'Favorite', + color: '#gray', + file_group_count: 0, + zim_entry_count: 0, + channel_count: 0, + domain_count: 0, + ...overrides, +}); + +/* --------------------------------------------------------------------------- */ +/* Context values */ +/* --------------------------------------------------------------------------- */ +/* + * These are the values a provider carries, not mocks of a provider. Passing one to the real + * `Context.Provider` runs the real consumers against real data, which is why none of the + * components using them need `jest.mock` -- see test-utils' render(). + */ + +export const themeContextFixture = (overrides = {}) => ({ + theme: defaultTheme, + savedTheme: null, + isDark: false, + // Media filtering is off unless a test asks for it; the themes that offer a filter + // decide their own default, and a fixture should not pre-empt that. + mediaFilter: undefined, + mediaFilterEnabled: false, + setMediaFilterEnabled: () => {}, + setTheme: () => {}, + setDarkTheme: () => {}, + setLightTheme: () => {}, + cycleSavedTheme: () => {}, + ...overrides, +}); + +export const statusContextFixture = (overrides = {}) => ({ + status: statusFixture(), + fetchStatus: () => Promise.resolve(), + ...overrides, +}); + +export const settingsContextFixture = (overrides = {}) => ({ + settings: settingsFixture(), + fetchSettings: () => Promise.resolve(), + saveSettings: () => Promise.resolve(), + pending: false, + ...overrides, +}); + +export const queryContextFixture = (overrides = {}) => ({ + searchParams: new URLSearchParams(), + setSearchParams: () => {}, + updateQuery: () => {}, + getLocationStr: () => '', + ...overrides, +}); + +/** + * The value `useTags` returns. + * + * The render helpers supply the real `TagsProvider` when a test does not pass tags, so this + * is for the cases that need particular tags without an API round trip. The component + * members are rendered as plain text: a test asserting on tag chips wants to find the name, + * not to re-test how Tags.js draws one. + */ +export const tagsContextFixture = (overrides = {}) => { + const tags = overrides.tags ?? []; + const findTagByName = (name) => tags.find(tag => tag.name === name); + return { + tags, + tagNames: tags.map(tag => tag.name), + NameToTagLabel: ({name}) => name ?? null, + TagsGroup: ({tagNames: names}) => (names || []).join(', '), + TagsLinkGroup: ({tagNames: names}) => (names || []).join(', '), + fetchTags: () => Promise.resolve(), + findTagByName, + SingleTag: ({name}) => name ?? null, + fuzzyMatchTagsByName: (name) => tags.filter(tag => tag.name.includes(name)), + ...overrides, + }; +}; + +/** The value FileWorkerStatusContext carries; no file worker is running by default. */ +export const fileWorkerStatusFixture = (overrides = {}) => ({ + status: null, + error: null, + refresh: () => Promise.resolve(), + setFastPolling: () => {}, + ...overrides, +}); diff --git a/app/src/test-fixtures.test.js b/app/src/test-fixtures.test.js new file mode 100644 index 00000000..bd666b00 --- /dev/null +++ b/app/src/test-fixtures.test.js @@ -0,0 +1,161 @@ +import React from 'react'; +import fs from 'fs'; +import path from 'path'; +import {render} from '@testing-library/react'; +import {QueryContext, SettingsContext, StatusContext, ThemeContext} from './contexts/contexts'; +import { + queryContextFixture, + settingsContextFixture, + statusContextFixture, + themeContextFixture, +} from './test-fixtures'; + +/* + * The fixtures are only worth having if they match what the app really carries. A fixture + * that has drifted is worse than none: the test passes, the component breaks in a browser, + * and the fixture is the last place anyone looks. + * + * That is not hypothetical here. The theme fixture supplied `i`, `s` and `t` holding + * hardcoded Semantic greys for months after ThemeProvider stopped supplying them, and + * FileBrowser was still reading `inverted` off the context and pasting it into a className -- + * so the tests agreed with the fixture, the fixture agreed with nothing, and the file + * browser's footer stayed light in all three dark themes. + * + * Each context's `createContext` default is the declared shape; these hold the fixtures to it + * key for key, in both directions. A property added to a context and forgotten in a fixture + * fails here, and so does one deleted from a context and left in a fixture. + */ + +/** Read a context's default by consuming it with no provider above -- public API only. */ +const defaultValueOf = (Context) => { + let captured; + const Probe = () => { + captured = React.useContext(Context); + return null; + }; + render(); + return captured; +}; + +describe('context fixtures match the contexts they stand in for', () => { + const cases = [ + ['ThemeContext', ThemeContext, themeContextFixture], + ['StatusContext', StatusContext, statusContextFixture], + ['SettingsContext', SettingsContext, settingsContextFixture], + ['QueryContext', QueryContext, queryContextFixture], + ]; + + cases.forEach(([name, Context, fixture]) => { + it(`${name} and its fixture declare the same properties`, () => { + expect(Object.keys(fixture()).sort()).toEqual(Object.keys(defaultValueOf(Context)).sort()); + }); + + it(`${name}'s fixture supplies a function wherever the context does`, () => { + // A callback stubbed as a value rather than a function turns into a "not a + // function" crash the first time a component calls it, usually in an event + // handler where the failure surfaces far from the fixture. + const declared = defaultValueOf(Context); + const supplied = fixture(); + Object.entries(declared) + .filter(([, value]) => typeof value === 'function') + .forEach(([key]) => expect(typeof supplied[key]).toBe('function')); + }); + }); +}); + +describe('fixtures cannot leak state between tests', () => { + it('hands out a new object every call', () => { + // Exported constants let one test mutate what the next one sees; every fixture is a + // function for that reason, and this fails if one is quietly turned back into a + // shared object. + const first = settingsContextFixture(); + const second = settingsContextFixture(); + + expect(first).not.toBe(second); + expect(first.settings).not.toBe(second.settings); + + first.settings.wrol_mode = true; + expect(second.settings.wrol_mode).toBe(false); + }); + + it('applies overrides without discarding the rest of the shape', () => { + const settings = settingsContextFixture({pending: true}); + + expect(settings.pending).toBe(true); + expect(settings.settings.media_directory).toBe('/media/wrolpi'); + }); +}); + +describe('specs still mocking a context module', () => { + /* + * A ratchet, not a prohibition. Mocking a context module replaces every export it has, + * providers included, so each spec that does it reinvents the pieces it still needs -- + * one of them supplies a `Media` that always reports desktop, which quietly leaves the + * mobile branch of that component untested, and another hand-writes a context as a bare + * `_currentValue` with no provider at all. + * + * These six can pass their contexts to render() instead. The list is here so the count + * can only fall: converting one means deleting its line, and a new spec reaching for the + * old pattern fails this test rather than joining an unread pile. + */ + const ALLOWED = [ + 'components/collections/BatchReorganizeModal.test.js', + 'components/collections/CollectionEditForm.test.js', + 'components/collections/CollectionReorganizeModal.test.js', + 'components/collections/CollectionTable.test.js', + 'components/collections/CollectionTagModal.test.js', + 'hooks/customHooks.test.js', + ]; + + it('is only the specs already known to do it', () => { + const offenders = []; + const walk = (dir) => { + for (const entry of fs.readdirSync(dir, {withFileTypes: true})) { + const full = path.join(dir, entry.name); + if (entry.isDirectory()) walk(full); + else if (/\.test\.js$/.test(entry.name)) { + const source = fs.readFileSync(full, 'utf8'); + if (/jest\.mock\('[^']*(contexts\/contexts|FileWorkerStatusContext|\/Tags)'/ + .test(source)) { + offenders.push(path.relative(__dirname, full)); + } + } + } + }; + walk(__dirname); + + expect(offenders.sort()).toEqual(ALLOWED); + }); +}); + +describe('the Semantic theme compatibility props are gone for good', () => { + it('no context, fixture, or component carries i/s/t/inverted from the theme', () => { + /* + * These four props are why this whole file exists. They were removed from + * ThemeProvider, but survived in the context default, in the test harness, and in one + * component that read `inverted` and pasted the resulting `undefined` into a + * className. Nothing failed, because the fixture supplied what production did not. + */ + const offenders = []; + const walk = (dir) => { + for (const entry of fs.readdirSync(dir, {withFileTypes: true})) { + const full = path.join(dir, entry.name); + if (entry.isDirectory()) walk(full); + else if (/\.(js|jsx|ts|tsx)$/.test(entry.name)) { + const source = fs.readFileSync(full, 'utf8') + .replace(/\/\*[\s\S]*?\*\//g, '') + .replace(/^\s*\/\/.*$/gm, ''); + // Destructuring any of them off ThemeContext, or a bare `inverted:` key + // in a context value. + if (/const\s*\{[^}]*\binverted\b[^}]*}\s*=\s*(React\.)?useContext\(\s*ThemeContext/ + .test(source)) { + offenders.push(`${path.relative(__dirname, full)} reads theme.inverted`); + } + } + } + }; + walk(__dirname); + + expect(offenders).toEqual([]); + }); +}); diff --git a/app/src/test-utils.js b/app/src/test-utils.js index e6d0a568..398fa0a0 100644 --- a/app/src/test-utils.js +++ b/app/src/test-utils.js @@ -1,114 +1,184 @@ /** - * Test utilities for React Testing Library - * Provides custom render functions with necessary context providers + * Test utilities: render helpers that supply what a component actually needs, and no more. + * + * Three tiers, because components differ in what they depend on and a single do-everything + * wrapper hides that: + * + * renderUI components from src/components/ui. Mantine's provider only. These + * take props and read no ambient state, which is enforced by the + * guards in components/ui/ui.test.js. + * render anything that reads a context. Supplies the REAL providers holding + * fixture values -- see test-fixtures.js -- so consumers run against + * real data instead of a mocked module. + * mockModule the last resort, for hooks that fetch. Always spreads the actual + * module, which hand-written jest.mock factories forget to do. + * + * The point of the middle tier is that `jest.mock('../../contexts/contexts')` should be + * unnecessary. Mocking that module replaces every export, including `Media` and the other + * providers, so each spec that did it reinvented them slightly differently -- one of them + * defaulting the responsive `Media` to desktop, which quietly left the mobile branch of that + * component untested. */ -import React, {createContext, useContext} from 'react'; -import {render} from '@testing-library/react'; +import React from 'react'; +import {render as rtlRender} from '@testing-library/react'; import {BrowserRouter} from 'react-router'; import {MantineProvider} from '@mantine/core'; -import {MediaContextProvider, ThemeContext} from './contexts/contexts'; +import { + MediaContextProvider, + QueryContext, + SettingsContext, + StatusContext, + ThemeContext, +} from './contexts/contexts'; import {cssVariablesResolver, mantineTheme} from './themes/mantine'; +import { + domainFixture, + domainsFixture, + queryContextFixture, + settingsContextFixture, + statusContextFixture, + themeContextFixture, +} from './test-fixtures'; -// Mock FileWorkerStatusContext for tests - avoids API calls during testing -const MockFileWorkerStatusContext = createContext(null); +/* + * Why this file imports only contexts/contexts, and nothing else of the app's. + * + * A first attempt had the harness import Tags.js and FileWorkerStatusContext.js so it could + * supply those providers too. That put both into the module graph of every spec that imports + * the harness, ahead of the spec's own mocks, and broke eight suites in two different ways. + * Where a spec mocked a module those files pull in, the component under test bound to a second + * instance of it -- a different `jest.fn()` from the one the test configured, so a hook mocked + * to return a value returned undefined, in a file the test never mentions. Requiring them + * lazily fixed that but not the mirror image of it: `useContext(TagsContext)` in a component + * read a different TagsContext than the one the harness had required, so the provider was + * ignored and the context default (`SingleTag: null`) won. + * + * The lesson is that a test harness must not change what a spec imports. So this file owns + * only the four contexts in contexts/contexts.js, and any other context is passed in by the + * spec, which resolves it exactly as the component does: + * + * import {TagsContext} from '../Tags'; + * render(, {contexts: [[TagsContext, tagsContextFixture({tags})]]}) + * + * A private mock context and re-exported `useFileWorkerStatus`/`useReorganizationStatus` also + * used to live here. They could never have worked: components import those hooks from + * contexts/FileWorkerStatusContext, so a copy exported from the harness was read by nothing. + */ -function MockFileWorkerStatusProvider({children}) { - const mockValue = { - status: null, - error: null, - refresh: jest.fn(), - setFastPolling: jest.fn(), - }; - return ( - - {children} - +/** + * Render a component from src/components/ui. + * + * Mantine's provider and nothing else, matching what those components can rely on. Reach for + * this rather than the full harness when the component takes props and reads no context: a + * test that supplies providers a component does not use conceals it starting to use one. + */ +export function renderUI(ui, {theme, ...renderOptions} = {}) { + if (theme) document.documentElement.dataset.theme = theme; + return rtlRender( + + {ui} + , + renderOptions, ); } -// Re-export hooks that use the mock context -export function useFileWorkerStatus() { - const context = useContext(MockFileWorkerStatusContext); - if (!context) { - throw new Error('useFileWorkerStatus must be used within FileWorkerStatusProvider'); - } - return context; -} - -export function useReorganizationStatus() { - const {status: progress, refresh} = useFileWorkerStatus(); - return { - isReorganizing: false, - taskType: null, - collectionId: null, - collectionKind: null, - batchStatus: null, - workerStatus: progress, - refresh, - }; -} - /** - * Custom render function that wraps components with necessary providers - * @param {React.Component} ui - Component to render - * @param {Object} options - Render options - * @param {boolean} options.inverted - Whether to use dark theme - * @param {Object} options.themeContext - Custom theme context values - * @param {boolean} options.withMedia - Include MediaContextProvider (default: false) - * @param {Object} options.renderOptions - Additional React Testing Library options + * Render anything that reads a context, with the real providers holding fixture values. + * + * Every context option takes overrides merged into that context's fixture, so a test states + * only the part it cares about: + * + * render(, {settings: {wrol_mode: true}}) + * render(, {status: {status: statusFixture({flags: {db_up: false}})}}) + * render(, {tags: {tags: [tagFixture({name: 'Repair'})]}}) + * + * @param {React.ReactElement} ui + * @param {Object} [options] + * @param {boolean} [options.inverted] render dark; sets the theme and Mantine's scheme together + * @param {Object} [options.theme] ThemeContext overrides + * @param {Object} [options.status] StatusContext overrides + * @param {Object} [options.settings] SettingsContext overrides + * @param {Object} [options.query] QueryContext overrides + * @param {Array} [options.contexts] extra [Context, value] pairs the spec supplies itself, + * for contexts outside contexts/contexts.js -- see the + * note at the top of this file for why + * @param {boolean} [options.withMedia] wrap in MediaContextProvider (needs window.matchMedia) + * @param {string} [options.route] point jsdom's location here before rendering + * @param {Object} [options.themeContext] deprecated alias for `theme` */ export function renderWithProviders( ui, { inverted = false, + theme = {}, themeContext = {}, + status = {}, + settings = {}, + query = {}, + contexts = [], withMedia = false, + route, ...renderOptions } = {} ) { - const defaultThemeContext = { - inverted, - setInverted: jest.fn(), - // Theme components use different properties: - i: inverted ? {inverted: true} : {inverted: undefined}, // For Semantic UI elements (Segment, Form, etc.) - s: inverted ? {style: {backgroundColor: '#1B1C1D', color: '#dddddd'}} : {}, // For style inversion - t: inverted ? {style: {color: '#eeeeee'}} : {}, // For text color inversion (Header, etc.) + // render() wraps content in a BrowserRouter rather than a MemoryRouter, so a test wanting + // a particular URL has to put jsdom's location there first. Several specs were doing + // this by hand. + if (route) window.history.pushState({}, '', route); + + const themeValue = themeContextFixture({ theme: inverted ? 'dark' : 'light', isDark: inverted, - savedTheme: null, - // Media filtering is off by default here; a test that needs it sets these. - mediaFilter: undefined, - mediaFilterEnabled: false, - setMediaFilterEnabled: jest.fn(), - setTheme: jest.fn(), - setDarkTheme: jest.fn(), - setLightTheme: jest.fn(), - cycleSavedTheme: jest.fn(), - ...themeContext - }; + ...themeContext, + ...theme, + }); + const statusValue = statusContextFixture(status); + const settingsValue = settingsContextFixture(settings); + const queryValue = queryContextFixture(query); + + /* + * One provider per context, skipping any a spec has mocked away. + * + * `jest.mock` on contexts/contexts replaces its exports, so a context can arrive here as + * undefined -- and one spec passes a hand-written object with a `_currentValue` and no + * `.Provider`, which works only because `useContext` reads that React internal. Rendering + * `undefined.Provider` fails with "element type is invalid" pointing at this wrapper rather + * than at the mock responsible, which is a miserable trail to follow. So the harness + * degrades rather than demanding every spec convert at once. + */ + const providers = [ + [StatusContext, statusValue], + [SettingsContext, settingsValue], + [QueryContext, queryValue], + [ThemeContext, themeValue], + ...contexts, + ]; function Wrapper({children}) { - // MantineProvider, configured exactly as ThemeProvider configures it: components - // from src/components/ui render Mantine internals and need it in the tree. + const withProviders = providers.reduceRight( + (inner, [Context, value]) => (Context && Context.Provider + ? {inner} + : inner), + children, + ); + const content = ( + {/* Configured exactly as ThemeProvider configures it: components from + src/components/ui render Mantine internals and need it in the tree. */} - - - {children} - - + {withProviders} ); - // Only wrap with MediaContextProvider if explicitly requested - // (since it requires window.matchMedia which can be tricky in tests) + // Opt-in: MediaContextProvider needs window.matchMedia, and a component that does not + // branch on breakpoint gains nothing from it. if (withMedia) { return {content}; } @@ -116,37 +186,34 @@ export function renderWithProviders( return content; } - return render(ui, {wrapper: Wrapper, ...renderOptions}); + return rtlRender(ui, {wrapper: Wrapper, ...renderOptions}); } /** - * Creates a mock domain collection object for testing + * Replace named exports of a module, keeping every other export intact. + * + * Use inside a `jest.mock` factory: + * + * jest.mock('../hooks/customHooks', () => + * require('../test-utils').mockModule('../hooks/customHooks', { + * useDomains: () => mockUseDomains(), + * })); + * + * The point is the spread. A factory written by hand returns only the keys it lists, so + * every other export of that module becomes undefined -- one spec mocks six hooks out of + * customHooks and silently blanks the rest, and the next hook a component under it starts + * calling fails as "not a function" a long way from the cause. */ -export function createMockDomain(overrides = {}) { - return { - id: 1, - domain: 'example.com', - archive_count: 42, - size: 1024000, - tag_name: null, - directory: '', - can_be_tagged: false, - description: '', - ...overrides - }; +export function mockModule(modulePath, overrides) { + return {...jest.requireActual(modulePath), ...overrides}; } -/** - * Creates multiple mock domains for list testing +/* + * The names the existing specs already import. Both delegate to test-fixtures so a domain is + * defined once rather than in two places free to disagree. */ -export function createMockDomains(count = 3) { - return Array.from({length: count}, (_, i) => createMockDomain({ - id: i + 1, - domain: `example${i + 1}.com`, - archive_count: (i + 1) * 10, - size: (i + 1) * 1000000, - })); -} +export const createMockDomain = domainFixture; +export const createMockDomains = domainsFixture; /** * Mock fetch implementation for API calls @@ -224,18 +291,6 @@ export function renderInLightMode(ui, options = {}) { }); } -/** - * Helper to check if an element has theme-aware (inverted) styling - * Returns true if the element has the 'inverted' class - * - * Usage: - * const segment = container.querySelector('.ui.segment'); - * expect(hasInvertedStyling(segment)).toBe(true); - */ -export function hasInvertedStyling(element) { - return element && element.classList.contains('inverted'); -} - /** * Creates a test-friendly form object using real useForm hook *