From acd27fe28e9171361712272e64e1eb3c8ebe4c55 Mon Sep 17 00:00:00 2001 From: Roland Date: Sat, 1 Aug 2026 18:17:18 -0600 Subject: [PATCH] =?UTF-8?q?test:=20act=20on=20review=20=E2=80=94=20the=20b?= =?UTF-8?q?rowser=20test=20named=20LoadStatistic=20but=20never=20mounted?= =?UTF-8?q?=20it?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The suite was titled "a load reading ranks itself", commented as covering the thresholds, and cross-referenced from Common.test.js as the place the painted result is checked. It mounted a hand-built ``. So it proved that role tokens resolve distinctly -- which the roles suite two blocks up already covers -- and would have stayed green with LoadStatistic reverted to `orange`/`red`. The same shape as the column-border test in #609: a claim that outran its measurement. It mounts the real helper at the real thresholds now, and the reversion fails it. The source guards are gone. They scraped function bodies out of files with indexOf slices, covered two helpers, and never executed a branch. `loadRole` and `healthRole` are pure exported functions instead, tested as mappings: either side of both load thresholds, every SMART assessment, lower case (the value comes from smartctl), and the case that matters most -- an unreadable drive must report as unknown, never as healthy. That is also the better answer to the jsdom problem that produced the guards. jsdom rejects `color: var(--danger)` and drops it, so the mapping cannot be read back off a rendered component; extracting it is the honest fix rather than asserting on source text. And the gallery was itself half-migrated: 15 Min. Load became `danger` while 5 Min. Load stayed `orange` -- in the one sample whose caption tells the reader to go and check night and amber, where orange is byte-identical to --text. Co-Authored-By: Claude Opus 5 (1M context) --- app/src/components/Common.js | 25 +++++-- app/src/components/Common.test.js | 87 +++++++++++++--------- app/src/components/ThemeSamplePage.js | 2 +- app/src/components/admin/ControllerPage.js | 36 ++++++--- app/src/components/ui/ui-layout.cy.js | 25 ++++--- 5 files changed, 110 insertions(+), 65 deletions(-) diff --git a/app/src/components/Common.js b/app/src/components/Common.js index 9c493765..6cbdec6e 100644 --- a/app/src/components/Common.js +++ b/app/src/components/Common.js @@ -1224,15 +1224,24 @@ export function FileIcon({file, disabled = true, size = 48, ...props}) { /> } -export function LoadStatistic({label, value, cores, ...props}) { - // Load is a warning above half the cores, and a problem above three quarters. +/** + * The severity of a load average: a warning above half the cores, a problem above three + * quarters, and nothing to say below that or when the core count is unknown. + * + * Exported and pure so the branches can be tested as branches. Reading the colour back off + * a rendered Statistic cannot do it -- jsdom rejects `color: var(--danger)` as an invalid + * declaration and drops it, so every value comes back as the empty string. + */ +export const loadRole = (value, cores) => { + if (!cores) return undefined; const quarter = cores / 4; - let color; - if (cores && value >= (quarter * 3)) { - color = 'danger'; - } else if (cores && value >= (quarter * 2)) { - color = 'warning'; - } + if (value >= quarter * 3) return 'danger'; + if (value >= quarter * 2) return 'warning'; + return undefined; +}; + +export function LoadStatistic({label, value, cores, ...props}) { + const color = loadRole(value, cores); return jest.fn(fn => { @@ -563,45 +563,62 @@ describe('contrastingColor with shorthand hex', () => { }); }); -describe('LoadStatistic ranks load by role, not by hue', () => { +describe('loadRole', () => { /* - * A source guard, not a render test, and deliberately so: jsdom's CSS parser REJECTS - * `color: var(--danger)` as an invalid declaration and drops it, so every inline colour - * reads back as the empty string here. A render test would pass whatever the component - * did -- the first version of this asserted `''` for the below-threshold cases and would - * have gone on passing with the hues restored. + * The branches, tested as branches. This used to be a source guard scraping the + * function body out of Common.js, because the obvious render test cannot work: jsdom + * REJECTS `color: var(--danger)` as an invalid declaration and drops it, so every inline + * colour reads back as the empty string and the assertions passed for that reason alone. + * Pulling the mapping out as a pure function is the honest fix. * - * What is painted is asserted in ui-layout.cy.js, in a browser, at the thresholds. + * Thresholds are halves and three quarters of the core count, so each is asserted either + * side of its boundary rather than at one comfortable value. */ - it('names roles rather than hues at both thresholds', () => { - const source = fs.readFileSync(path.join(__dirname, 'Common.js'), 'utf8'); - const helper = source.slice( - source.indexOf('export function LoadStatistic'), - source.indexOf('export function', source.indexOf('export function LoadStatistic') + 10)); + it('says nothing about a load below half the cores', () => { + expect(loadRole(1.9, 4)).toBeUndefined(); + expect(loadRole(0, 4)).toBeUndefined(); + }); - expect(helper).toContain("color = 'danger'"); - expect(helper).toContain("color = 'warning'"); - expect(helper.match(/'(red|green|amber|yellow|orange)'/)).toBeNull(); + it('warns from half the cores', () => { + expect(loadRole(2.0, 4)).toBe('warning'); + expect(loadRole(2.9, 4)).toBe('warning'); + }); + + it('calls it danger from three quarters', () => { + expect(loadRole(3.0, 4)).toBe('danger'); + expect(loadRole(99, 4)).toBe('danger'); + }); + + it('says nothing when the core count is unknown', () => { + // Both thresholds are guarded on `cores`. Without it every load would read as fine, + // which is the safe direction, but worth pinning so it stays deliberate. + expect(loadRole(12, undefined)).toBeUndefined(); + expect(loadRole(12, 0)).toBeUndefined(); + }); + + it('never returns a hue', () => { + // The regression this exists for: `orange` is byte-identical to `--text` in night, + // so a warning load rendered as an ordinary uncoloured number. + [loadRole(1, 4), loadRole(2.5, 4), loadRole(4, 4)].filter(Boolean).forEach(role => + expect(role).not.toMatch(/^(red|green|amber|yellow|orange|blue)$/)); }); }); -describe('a drive\'s SMART assessment is a severity, not a colour', () => { - /* - * A source guard: `getHealthColor` is module-private, and exporting it only so a test - * can reach it would be shaping the code around the test. What matters is that the - * helper stays on the ramp -- all four branches or none, since a helper where PASS and - * FAIL rank but WARN is an unranked hue is worse than either choice. - */ - it('maps every branch to a role', () => { - const source = fs.readFileSync( - path.join(__dirname, 'admin', 'ControllerPage.js'), 'utf8'); - const helper = source.slice( - source.indexOf('const getHealthColor'), - source.indexOf('};', source.indexOf('const getHealthColor'))); +describe('healthRole', () => { + it('maps every SMART assessment to a severity', () => { + expect(healthRole('PASS')).toBe('success'); + expect(healthRole('WARN')).toBe('warning'); + expect(healthRole('FAIL')).toBe('danger'); + }); - expect(helper).toContain("'var(--success)'"); - expect(helper).toContain("'var(--warning)'"); - expect(helper).toContain("'var(--danger)'"); - expect(helper.match(/var\(--(red|green|amber|yellow|orange)\)/)).toBeNull(); + it('is not case sensitive, because the value comes from smartctl', () => { + expect(healthRole('pass')).toBe('success'); + expect(healthRole('Fail')).toBe('danger'); + }); + + it('treats a missing or unrecognised assessment as unknown, not as healthy', () => { + // Reporting an unreadable drive as PASS would be the dangerous direction. + [undefined, null, '', 'SOMETHING_ELSE'].forEach(value => + expect(healthRole(value)).toBe('neutral')); }); }); diff --git a/app/src/components/ThemeSamplePage.js b/app/src/components/ThemeSamplePage.js index d15c3963..2e501795 100644 --- a/app/src/components/ThemeSamplePage.js +++ b/app/src/components/ThemeSamplePage.js @@ -196,7 +196,7 @@ export function ThemeSamplePage() {
Coloured readings, as Status draws them
- + diff --git a/app/src/components/admin/ControllerPage.js b/app/src/components/admin/ControllerPage.js index 20e9dec4..14395160 100644 --- a/app/src/components/admin/ControllerPage.js +++ b/app/src/components/admin/ControllerPage.js @@ -605,19 +605,33 @@ function ServicesSection() { // Helper to get health status color from SMART assessment -const getHealthColor = (assessment) => { - // Roles, not hues: a drive's assessment is a severity and has to rank as one. All - // four branches together -- a helper where PASS and FAIL rank but WARN is an - // unranked hue is worse than either choice, and in night `--amber` is byte-identical - // to `--yellow` so WARN would stop sitting between them. - if (!assessment) return 'var(--neutral)'; - const upper = assessment.toUpperCase(); - if (upper === 'PASS') return 'var(--success)'; - if (upper === 'WARN') return 'var(--warning)'; - if (upper === 'FAIL') return 'var(--danger)'; - return 'var(--neutral)'; +/** + * The severity of a SMART assessment. + * + * Roles, not hues: a drive's health is a severity and has to rank as one. All four + * branches together -- a helper where PASS and FAIL rank but WARN is an unranked hue is + * worse than either choice, and in night `--amber` is byte-identical to `--yellow`, so WARN + * would stop sitting between them. + * + * Exported and pure so the mapping can be tested as a mapping rather than scraped out of + * the source, which is what this needed before it was separated from the CSS wrapper. + */ +export const healthRole = (assessment) => { + if (!assessment) return 'neutral'; + switch (assessment.toUpperCase()) { + case 'PASS': + return 'success'; + case 'WARN': + return 'warning'; + case 'FAIL': + return 'danger'; + default: + return 'neutral'; + } }; +const getHealthColor = (assessment) => `var(--${healthRole(assessment)})`; + // Helper to format power-on hours const formatPowerOnHours = (hours) => { if (hours === null || hours === undefined) return '-'; diff --git a/app/src/components/ui/ui-layout.cy.js b/app/src/components/ui/ui-layout.cy.js index ef42dcb0..0dd184ef 100644 --- a/app/src/components/ui/ui-layout.cy.js +++ b/app/src/components/ui/ui-layout.cy.js @@ -4,7 +4,7 @@ import { Message, Placeholder, Statistic, StatisticGroup, Status, TabBar, Table, tabClassName, TextInput, } from './index'; -import {contrastingColor} from '../Common'; +import {contrastingColor, LoadStatistic} from '../Common'; import {Notifications} from '@mantine/notifications'; import {clearToasts, toast} from './toast'; import {monochromeThemes, themeNames} from '../../themes/names'; @@ -335,26 +335,31 @@ describe('semantic roles read as severity in every theme', () => { describe('a load reading ranks itself in the monochrome themes', () => { /* - * LoadStatistic is the Status page's CPU load, and the clearest payoff of the roles - * outside the library: it used to be `orange` above half the cores and `red` above three - * quarters, and in night `--orange` is byte-identical to `--text`, so a machine under - * load rendered its warning as an ordinary uncoloured number. + * The real `LoadStatistic`, at the real thresholds -- NOT a hand-built `Statistic` with + * the roles typed in. The first version of this did that, and it would have stayed green + * with `LoadStatistic` reverted to `orange`/`red`: it proved only that role tokens resolve + * distinctly, which the roles suite above already covers. * - * Only a browser can see this. jsdom rejects `color: var(--warning)` as invalid and - * drops it, so the inline colour reads back empty whatever the component did. + * This is the Status page's CPU load, and the clearest payoff of the roles outside the + * library: `orange` above half the cores was byte-identical to `--text` in night, so a + * machine under load rendered its warning as an ordinary uncoloured number. + * + * Only a browser can see it. jsdom rejects `color: var(--warning)` as invalid and drops + * it, so the inline colour reads back empty whatever the component did. */ monochromeThemes.forEach((theme) => { it(`separates fine, warning and danger loads in ${theme}`, () => { cy.mountUI( - - - + + + , {theme}, ); cy.get('.wrolpi-statistic-value').should(($values) => { + expect($values.length, 'three readings').to.equal(3); const [fine, warning, danger] = [...$values] .map(el => toRgb(getComputedStyle(el).color)); const panel = toRgb(getComputedStyle(