test: the theme drives the colour scheme, and mockModule takes the module

Both of Greptile's findings on #591 were real, and the first was a plain miss:
mockModule shipped with no caller, so its documented usage had never once been
executed and could not have worked.  It called jest.requireActual on a path
passed in by the spec, which resolves relative to test-utils.js rather than to
the caller -- for any spec in a subdirectory the documented
'../hooks/customHooks' pointed outside src altogether.  It now takes the module
the spec has already resolved, and throws on a string rather than quietly
resolving to the wrong one, which was the original failure.

It also has a caller now.  FileBrowser.test.js listed six hooks in a hand-written
factory and returned only those, so every other export of customHooks was
undefined for anything it rendered -- the exact landmine the helper exists to
remove.

The second: `inverted` drove Mantine's colour scheme on its own, so asking for
night or amber through the new theme option produced a tree that reported a dark
theme while Mantine rendered its light one.  Production cannot reach that
combination, and a harness that invents states the app never has is worse than
one that is merely incomplete -- what a test proves about such a tree is true of
nothing.  The scheme is now derived from the theme name via isDarkTheme, and
data-theme is stamped on <html>, without which a component test cannot exercise
any token table or the night-mode treatment at all.  isDark remains overridable
so a spec can still make the two disagree deliberately.

test-utils.test.js covers the harness itself, which had none.  Eight of its
twelve assertions were red against the code above.

986 jest, 46 e2e, clean cache-busted build, tsc clean.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

app/src/components/FileBrowser.test.js

@@ -28,14 +28,23 @@ const mockUseUploadFile = {
     inProgress: false,
 };
 
-jest.mock('../hooks/customHooks', () => ({
-    useBrowseFiles: jest.fn(),
-    useMediaDirectory: jest.fn(() => '/media/wrolpi'),
-    useWROLMode: jest.fn(() => false),
-    useStatusFlag: jest.fn(() => false),
-    useUploadFile: () => mockUseUploadFile,
-    useElementWidth: jest.fn(() => [() => {}, 0]),
-}));
+/*
+ * Every other export of customHooks used to be dropped here -- the factory listed six hooks and
+ * returned only those, so the rest of the module was undefined for anything this spec rendered.
+ * mockModule spreads the real module first, so the next hook a component reaches for still
+ * exists.
+ */
+jest.mock('../hooks/customHooks', () => require('../test-utils').mockModule(
+    jest.requireActual('../hooks/customHooks'),
+    {
+        useBrowseFiles: jest.fn(),
+        useMediaDirectory: jest.fn(() => '/media/wrolpi'),
+        useWROLMode: jest.fn(() => false),
+        useStatusFlag: jest.fn(() => false),
+        useUploadFile: () => mockUseUploadFile,
+        useElementWidth: jest.fn(() => [() => {}, 0]),
+    },
+));
 
 // Mock react-dropzone
 jest.mock('react-dropzone', () => ({

app/src/test-utils.js

@@ -32,6 +32,7 @@ import {
     ThemeContext,
 } from './contexts/contexts';
 import {cssVariablesResolver, mantineTheme} from './themes/mantine';
+import {darkTheme, isDarkTheme, lightTheme} from './themes/names';
 import {
     domainFixture,
     domainsFixture,
@@ -127,12 +128,28 @@ export function renderWithProviders(
     // this by hand.
     if (route) window.history.pushState({}, '', route);
 
+    /*
+     * The theme decides the colour scheme, rather than the two being set independently.
+     *
+     * `inverted` used to drive Mantine's scheme on its own, so asking for night or amber
+     * through the theme option produced a tree that reported a dark theme while Mantine
+     * rendered its light one -- a combination production cannot reach.  A harness inventing
+     * states the app never has is worse than one that is merely incomplete: what a test proves
+     * about that tree is true of nothing.
+     *
+     * `isDark` follows from the theme name, but only as a default: a spec deliberately checking
+     * what a component does when the two disagree can still say so.
+     */
+    const requestedTheme = theme.theme ?? themeContext.theme ?? (inverted ? darkTheme : lightTheme);
     const themeValue = themeContextFixture({
-        theme: inverted ? 'dark' : 'light',
-        isDark: inverted,
+        theme: requestedTheme,
+        isDark: isDarkTheme(requestedTheme),
         ...themeContext,
         ...theme,
     });
+    // Every token table and the whole night-mode treatment key on this attribute, so a
+    // component test that leaves it unset cannot exercise any of them.
+    document.documentElement.dataset.theme = requestedTheme;
     const statusValue = statusContextFixture(status);
     const settingsValue = settingsContextFixture(settings);
     const queryValue = queryContextFixture(query);
@@ -170,7 +187,7 @@ export function renderWithProviders(
                 <MantineProvider
                     theme={mantineTheme}
                     cssVariablesResolver={cssVariablesResolver}
-                    forceColorScheme={inverted ? 'dark' : 'light'}
+                    forceColorScheme={isDarkTheme(requestedTheme) ? 'dark' : 'light'}
                 >
                     {withProviders}
                 </MantineProvider>
@@ -192,20 +209,33 @@ export function renderWithProviders(
 /**
  * Replace named exports of a module, keeping every other export intact.
  *
- * Use inside a `jest.mock` factory:
+ * Takes the module, which the spec resolves itself, not a path to it:
  *
  *   jest.mock('../hooks/customHooks', () =>
- *       require('../test-utils').mockModule('../hooks/customHooks', {
- *           useDomains: () => mockUseDomains(),
- *       }));
+ *       require('../test-utils').mockModule(
+ *           jest.requireActual('../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.
+ * 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 blanks the rest, and the next hook a component under it starts calling fails as "not a
+ * function" a long way from the cause.
+ *
+ * It first took a path and called `jest.requireActual` itself, which cannot work: the
+ * specifier would resolve relative to THIS file rather than to the spec, so the documented
+ * `'../hooks/customHooks'` pointed outside src altogether for any spec in a subdirectory.  The
+ * helper had no caller, so nothing ever ran it.  Throwing on a string is deliberate -- silently
+ * resolving to the wrong module is the failure that was there to begin with.
  */
-export function mockModule(modulePath, overrides) {
-    return {...jest.requireActual(modulePath), ...overrides};
+export function mockModule(actualModule, overrides) {
+    if (typeof actualModule === 'string') {
+        throw new Error(
+            'mockModule takes the module, not a path: pass jest.requireActual(path) so the '
+            + 'specifier resolves from your spec rather than from test-utils.js',
+        );
+    }
+    return {...actualModule, ...overrides};
 }
 
 /*

app/src/test-utils.test.js

@@ -0,0 +1,96 @@
+import React from 'react';
+import {mockModule, render, renderUI, screen} from './test-utils';
+import {ThemeContext} from './contexts/contexts';
+import {amberTheme, darkTheme, lightTheme, nightTheme} from './themes/names';
+
+/*
+ * The harness itself needs tests.
+ *
+ * Both of these cover mistakes it shipped with.  `mockModule` had no caller at all, so its
+ * documented usage was never once executed and could not have worked; and the theme option
+ * moved ThemeContext without moving Mantine's colour scheme, so asking for night produced a
+ * tree that reported night and rendered light -- a state production never reaches, which is
+ * the one thing a test harness must not invent.
+ */
+
+const ThemeProbe = () => {
+    const {theme, isDark} = React.useContext(ThemeContext);
+    return <div data-testid='probe'>{`${theme}:${isDark}`}</div>;
+};
+
+describe('render keeps the theme and the colour scheme together', () => {
+    // The dark-background themes, which is what Mantine has to be told about.  night and amber
+    // are the interesting ones: neither is ever chosen by prefers-color-scheme, so nothing else
+    // would put Mantine's own components on a dark scheme for them.
+    [darkTheme, nightTheme, amberTheme].forEach((theme) => {
+        it(`renders ${theme} as a dark scheme, and reports it as dark`, () => {
+            render(<ThemeProbe/>, {theme: {theme}});
+
+            expect(screen.getByTestId('probe')).toHaveTextContent(`${theme}:true`);
+            expect(document.documentElement)
+                .toHaveAttribute('data-mantine-color-scheme', 'dark');
+        });
+
+        it(`stamps data-theme for ${theme}, so CSS keyed on it applies`, () => {
+            // Every token table and the whole night-mode treatment key on this attribute; a
+            // component test that never sets it cannot exercise any of them.
+            render(<ThemeProbe/>, {theme: {theme}});
+
+            expect(document.documentElement).toHaveAttribute('data-theme', theme);
+        });
+    });
+
+    it('renders light as a light scheme', () => {
+        render(<ThemeProbe/>, {theme: {theme: lightTheme}});
+
+        expect(screen.getByTestId('probe')).toHaveTextContent(`${lightTheme}:false`);
+        expect(document.documentElement).toHaveAttribute('data-mantine-color-scheme', 'light');
+    });
+
+    it('still honours the older inverted flag', () => {
+        render(<ThemeProbe/>, {inverted: true});
+
+        expect(screen.getByTestId('probe')).toHaveTextContent(`${darkTheme}:true`);
+        expect(document.documentElement).toHaveAttribute('data-mantine-color-scheme', 'dark');
+    });
+
+    it('lets a test contradict the theme deliberately', () => {
+        // isDark is derived, not fixed: a spec checking what a component does when the two
+        // disagree must still be able to say so.
+        render(<ThemeProbe/>, {theme: {theme: nightTheme, isDark: false}});
+
+        expect(screen.getByTestId('probe')).toHaveTextContent(`${nightTheme}:false`);
+    });
+});
+
+describe('renderUI stamps the theme it is given', () => {
+    it('sets data-theme so token-driven CSS resolves', () => {
+        renderUI(<div data-testid='plain'>hi</div>, {theme: amberTheme});
+
+        expect(document.documentElement).toHaveAttribute('data-theme', amberTheme);
+    });
+});
+
+describe('mockModule', () => {
+    /*
+     * It takes the module, not a path to it.  Passing a path meant `jest.requireActual` ran
+     * from inside test-utils.js and resolved the caller's relative specifier against the wrong
+     * directory -- for a spec in components/ the documented `'../hooks/customHooks'` pointed
+     * outside src entirely.  Handing over the module the spec already resolved cannot go wrong.
+     */
+    it('keeps every export the caller did not replace', () => {
+        const actual = {useOne: () => 1, useTwo: () => 2, CONSTANT: 'x'};
+        const replaced = () => 99;
+
+        const result = mockModule(actual, {useTwo: replaced});
+
+        expect(result.useOne()).toBe(1);
+        expect(result.useTwo).toBe(replaced);
+        expect(result.CONSTANT).toBe('x');
+    });
+
+    it('rejects a path, which cannot resolve from here', () => {
+        // Failing loudly beats resolving to the wrong module or to nothing at all.
+        expect(() => mockModule('../hooks/customHooks', {})).toThrow(/module.*not a path/i);
+    });
+});