From b36f7beeb8eeaa78db596c65e315b6cac5d40a4b Mon Sep 17 00:00:00 2001 From: Roland Date: Sat, 1 Aug 2026 15:22:39 -0600 Subject: [PATCH] =?UTF-8?q?test:=20act=20on=20review=20=E2=80=94=20observe?= =?UTF-8?q?=20the=20wiring,=20and=20stop=20the=20soft=20assertions?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All five points were right. * The gallery's Toasts copy still read "Bottom right" after the container moved. Anyone using that page as a visual checklist would have looked in the wrong corner and concluded the change had not landed. * Nothing observed ThemeProvider's own wiring. Both suites mount their own ``, so moving the container back, or dropping it, would have failed neither -- while the comments claimed "container missing while call sites still fire" as the failure already shipped once. Three tests in ThemeProvider.test.js now raise a real toast through the provider and assert it appears, and where. Asserted through a toast rather than by querying for a container: Mantine renders an empty container for every position it supports, so the obvious selector returns `top-center` no matter how the provider is configured. That is what the first version of this test did, and it failed for that reason rather than the intended one. The five-at-once test is honest about what it covers: Mantine's own default limit is also 5, so it does not prove the prop is there. It catches the limit being raised. * The default duration was untested. Sleeping 150ms and finding the toast still up rules out "vanishes next tick" and nothing else -- it passes for a 200ms default, for `false`, and for Mantine's own 4000ms. Read the deadline back off Mantine's store instead: 5000 when unset, the caller's number when given, `false` for 0. * Space activates a followable toast in `toast.ts` and only Enter was covered, so dropping that branch stayed green. * Colour assertions used `toContain('red')`, which any declaration containing those three letters satisfies. They match the whole custom property now. Planted four regressions to check the new tests: position, limit, the 5000ms default, and the Space branch. Three went red; the limit one did not, for the reason recorded above. --- app/src/components/ThemeProvider.test.js | 69 +++++++++++++++++++++++- app/src/components/ThemeSamplePage.js | 2 +- app/src/components/ui/toast.test.js | 60 +++++++++++++++++---- 3 files changed, 119 insertions(+), 12 deletions(-) diff --git a/app/src/components/ThemeProvider.test.js b/app/src/components/ThemeProvider.test.js index 98995e66..be756acd 100644 --- a/app/src/components/ThemeProvider.test.js +++ b/app/src/components/ThemeProvider.test.js @@ -1,7 +1,7 @@ import fs from 'fs'; import path from 'path'; import React, {useContext} from 'react'; -import {act, render, screen} from '@testing-library/react'; +import {act, render, screen, waitFor} from '@testing-library/react'; import userEvent from '@testing-library/user-event'; import {ThemeContext} from '../contexts/contexts'; import { @@ -16,6 +16,7 @@ import { themeSessionKey, } from './Theme'; import {mediaFilterSessionKey, resolveMediaFilter, themeMediaFilter} from '../themes/names'; +import {clearToasts, toast} from './ui'; // Lets a test drive `prefers-color-scheme` and assert on what the provider applied. let prefersDark = false; @@ -302,3 +303,69 @@ describe('theme helpers', () => { expect(isDarkTheme(amberTheme)).toBe(true); }); }); + +describe('the toast container ThemeProvider mounts', () => { + /* + * Observing the wiring rather than re-stating it. + * + * `toast.test.js` and `ui-layout.cy.js` each mount their own `` with the + * position and limit they expect, so neither would notice if this file moved the container + * back, changed its limit, or dropped it altogether -- and a missing container while + * thirteen call sites keep firing is the failure that has already shipped once. Those + * suites test `toast()`; this tests that the app has somewhere to put what it produces. + */ + + beforeEach(() => { + prefersDark = false; + localStorage.clear(); + mockMatchMedia(); + }); + + afterEach(() => act(() => clearToasts())); + + const notifications = () => + document.querySelectorAll('[class*="mantine-Notification-root"]'); + + it('shows a toast raised by a call site', async () => { + // The regression that already shipped: no container, thirteen call sites still firing, + // nothing thrown and nothing shown. + renderProvider(); + + act(() => toast({title: 'Refresh started'})); + + expect(await screen.findByText('Refresh started')).toBeInTheDocument(); + }); + + it('puts it in the top right', async () => { + /* + * Asserted through a real toast rather than by finding a container. Mantine renders an + * empty container for every position it supports, so querying for one returns whichever + * comes first in the DOM -- `top-center` -- whatever the provider was configured with. + */ + renderProvider(); + + act(() => toast({title: 'Somewhere'})); + const notification = await screen.findByText('Somewhere'); + + expect(notification.closest('[class*="mantine-Notifications-root"]')) + .toHaveAttribute('data-position', 'top-right'); + }); + + it('shows five at once and queues the rest', async () => { + /* + * The user-visible behaviour, not the prop: Mantine's own default limit is also 5, so + * this stays green if `limit={5}` is deleted from the provider. It is here to catch a + * limit being *raised* -- a background task that reports twenty times should not bury + * the page -- rather than to prove the prop is present. + */ + renderProvider(); + + act(() => { + for (let i = 1; i <= 7; i += 1) { + toast({title: `Toast ${i}`}); + } + }); + + await waitFor(() => expect(notifications()).toHaveLength(5)); + }); +}); diff --git a/app/src/components/ThemeSamplePage.js b/app/src/components/ThemeSamplePage.js index 39fb8396..05644464 100644 --- a/app/src/components/ThemeSamplePage.js +++ b/app/src/components/ThemeSamplePage.js @@ -288,7 +288,7 @@ export function ThemeSamplePage() {

- Bottom right, five at a time, five seconds each unless the caller says + Top right, five at a time, five seconds each unless the caller says otherwise. Check each type in all four themes: night has no second hue to spend, so info, success, warning and error cannot be told apart by colour there. diff --git a/app/src/components/ui/toast.test.js b/app/src/components/ui/toast.test.js index 92c04bb1..338995c0 100644 --- a/app/src/components/ui/toast.test.js +++ b/app/src/components/ui/toast.test.js @@ -4,6 +4,7 @@ import userEvent from '@testing-library/user-event'; import {MantineProvider} from '@mantine/core'; import {Notifications} from '@mantine/notifications'; import {cssVariablesResolver, mantineTheme} from '../../themes/mantine'; +import {notificationsStore} from '@mantine/notifications'; import {clearToasts, toast} from './toast'; /* @@ -111,9 +112,10 @@ describe('toast', () => { showToast({type: 'error', title: 'Download failed'}); await waitFor(() => expect(visibleToasts()).toHaveLength(1)); - // Mantine writes the resolved colour onto the element as a custom property. - const notification = visibleToasts()[0]; - expect(notification.getAttribute('style')).toContain('red'); + // The whole custom property, not a substring: `toContain('red')` would also be + // satisfied by any other declaration that happens to carry those three letters. + expect(visibleToasts()[0].getAttribute('style')) + .toMatch(/--notification-color:\s*var\(--mantine-color-red-/); }); it('gives each type a colour of its own', async () => { @@ -142,7 +144,8 @@ describe('toast', () => { showToast({title: 'Refresh started'}); await waitFor(() => expect(visibleToasts()).toHaveLength(1)); - expect(visibleToasts()[0].getAttribute('style')).toContain('blue'); + expect(visibleToasts()[0].getAttribute('style')) + .toMatch(/--notification-color:\s*var\(--mantine-color-blue-/); }); it('survives being called with nothing at all', () => { @@ -203,6 +206,20 @@ describe('a toast that can be clicked', () => { expect(onClick).toHaveBeenCalledTimes(1); }); + it('can also be activated with Space', async () => { + // `toast.ts` handles Enter and Space; only Enter was covered, so dropping the Space + // branch -- or switching to `event.code` -- would have stayed green. + const onClick = jest.fn(); + withToasts(); + + showToast({title: 'Screenshot Generated', onClick}); + await waitFor(() => expect(visibleToasts()).toHaveLength(1)); + visibleToasts()[0].focus(); + await userEvent.keyboard(' '); + + expect(onClick).toHaveBeenCalledTimes(1); + }); + it('looks clickable only when it is', async () => { withToasts(); @@ -244,15 +261,38 @@ describe('how long a toast stays up', () => { await waitFor(() => expect(screen.queryByText('Saved')).not.toBeInTheDocument()); }); - it('is still up a moment after being shown, when no time is given', async () => { - // The default is five seconds, which is not worth waiting for; what is worth asserting - // is that a caller who names no time does not get a toast that vanishes instantly. + /* + * The dismissal deadline `toast()` hands Mantine, read back off its store. + * + * Waiting is no good for this. Sleeping 150ms and finding the toast still there catches + * "vanishes on the next tick" and nothing else -- it passes just as happily for a 200ms + * default, a 1000ms one, `false`, or Mantine's own container default of 4000ms. Waiting + * the full five seconds instead would put five seconds into every run. The store holds + * exactly what was passed, which is the thing worth asserting. + */ + const autoCloseOf = (options) => { + showToast(options); + const [notification] = notificationsStore.getState().notifications; + return notification.autoClose; + }; + + it('gives a toast five seconds when the caller names no time', () => { withToasts(); - showToast({title: 'Refresh started'}); - await new Promise(resolve => setTimeout(resolve, 150)); + expect(autoCloseOf({title: 'Refresh started'})).toBe(5000); + }); - expect(screen.getByText('Refresh started')).toBeInTheDocument(); + it('passes a time the caller chose straight through', () => { + withToasts(); + + expect(autoCloseOf({title: 'Brief', time: 250})).toBe(250); + }); + + it('turns time 0 into no deadline at all', () => { + // Not 0. Mantine reads a number as a delay, so a literal 0 dismisses on the next tick. + withToasts(); + + expect(autoCloseOf({title: 'Download failed', time: 0})).toBe(false); }); it('stays until dismissed when the caller asks for time 0', async () => {