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(