diff --git a/app/src/components/FileBrowser.test.js b/app/src/components/FileBrowser.test.js index 0a937b91..3d0d7c35 100644 --- a/app/src/components/FileBrowser.test.js +++ b/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', () => ({ diff --git a/app/src/test-utils.js b/app/src/test-utils.js index 398fa0a0..534e05ed 100644 --- a/app/src/test-utils.js +++ b/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( {withProviders} @@ -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}; } /* diff --git a/app/src/test-utils.test.js b/app/src/test-utils.test.js new file mode 100644 index 00000000..e6cf5246 --- /dev/null +++ b/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
{`${theme}:${isDark}`}
; +}; + +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(, {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(, {theme: {theme}}); + + expect(document.documentElement).toHaveAttribute('data-theme', theme); + }); + }); + + it('renders light as a light scheme', () => { + render(, {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(, {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(, {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(
hi
, {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); + }); +});