From b4cc76eb7112668149abaa8ace508898236ff479 Mon Sep 17 00:00:00 2001 From: Alex Reardon Date: Fri, 7 Aug 2020 15:30:36 +1000 Subject: [PATCH 1/3] using name as key rather than order based unique id --- .nvmrc | 2 +- src/decorator/task-harness.tsx | 10 +- src/events.ts | 4 +- src/panel/machine.ts | 2 +- src/panel/task-group.tsx | 6 +- src/panel/task-result/error-result.tsx | 1 - src/panel/task-result/expanding-result.tsx | 5 +- src/panel/task-result/index.tsx | 9 +- src/panel/task-result/static-result.tsx | 1 - src/panel/task-result/timed-result.tsx | 1 - src/panel/use-panel-machine.tsx | 10 +- src/task-runner/get-error-result.ts | 4 +- src/task-runner/run-group.ts | 4 +- src/task-runner/run-static-task.ts | 2 +- src/task-runner/run-timed-task.ts | 2 +- src/tasks/get-groups.ts | 24 +++- src/tasks/get-interaction-group.ts | 2 +- src/tasks/get-tasks-map.ts | 4 +- src/tasks/preset/client.tsx | 36 +++--- src/tasks/preset/create.ts | 23 ---- src/tasks/preset/server.tsx | 31 +++--- src/types.ts | 13 +-- src/util/get-result-map.ts | 6 + src/util/to-result-map.ts | 6 - test-util/mocks.ts | 116 +++++++++++--------- test/panel/task-running-guards.spec.tsx | 10 +- test/tasks/mark.spec.ts | 2 - test/tasks/run-all.spec.ts | 8 +- test/tasks/run-one-static.spec.ts | 3 +- test/tasks/run-one-timed.spec.ts | 3 +- test/tasks/story-function.spec.tsx | 16 ++- test/tasks/time-controls.spec.tsx | 1 - test/tasks/timed-task-bail-on-error.spec.ts | 1 - test/tasks/unsupported-errors.spec.ts | 6 +- 34 files changed, 195 insertions(+), 179 deletions(-) delete mode 100644 src/tasks/preset/create.ts create mode 100644 src/util/get-result-map.ts delete mode 100644 src/util/to-result-map.ts diff --git a/.nvmrc b/.nvmrc index d16c3a3..4150e8a 100644 --- a/.nvmrc +++ b/.nvmrc @@ -1 +1 @@ -10.15.3 \ No newline at end of file +12.18.3 \ No newline at end of file diff --git a/src/decorator/task-harness.tsx b/src/decorator/task-harness.tsx index c89d9b4..0e75aeb 100644 --- a/src/decorator/task-harness.tsx +++ b/src/decorator/task-harness.tsx @@ -66,10 +66,10 @@ export default function TaskHarness({ getNode, channel, interactions, allowedGro }, { eventName: eventNames.START_ONE, - fn: async function onStartOne({ taskId, copies, samples }: RunOne['Params']) { - const task = tasks[taskId]; + fn: async function onStartOne({ taskName, copies, samples }: RunOne['Params']) { + const task = tasks[taskName]; if (task == null) { - throw new Error(`Could not find task with id: ${taskId}`); + throw new Error(`Could not find task with id: ${taskName}`); } if (task.type === 'timed' || task.type === 'interaction') { @@ -79,7 +79,7 @@ export default function TaskHarness({ getNode, channel, interactions, allowedGro samples, copies, }); - safeEmit(eventNames.FINISH_ONE, { taskId, result }); + safeEmit(eventNames.FINISH_ONE, { taskName, result }); return; } if (task.type === 'static') { @@ -88,7 +88,7 @@ export default function TaskHarness({ getNode, channel, interactions, allowedGro getNode, copies, }); - safeEmit(eventNames.FINISH_ONE, { taskId, result }); + safeEmit(eventNames.FINISH_ONE, { taskName, result }); return; } }, diff --git a/src/events.ts b/src/events.ts index eb9c2c7..0ce2060 100644 --- a/src/events.ts +++ b/src/events.ts @@ -8,8 +8,8 @@ export default { }; export type RunOne = { - Params: { taskId: string; copies: number; samples: number }; - Result: { taskId: string; result: TimedResult | StaticResult }; + Params: { taskName: string; copies: number; samples: number }; + Result: { taskName: string; result: TimedResult | StaticResult }; }; export type RunAll = { diff --git a/src/panel/machine.ts b/src/panel/machine.ts index a8f467a..f5f6e32 100644 --- a/src/panel/machine.ts +++ b/src/panel/machine.ts @@ -5,7 +5,7 @@ export type MachineEvents = | { type: 'WAIT' } | { type: 'LOADED'; storyName: string; pinned: Nullable } | { type: 'START_ALL' } - | { type: 'START_ONE'; taskId: string } + | { type: 'START_ONE'; taskName: string } | { type: 'FINISH'; results: TaskGroupResult[] } | { type: 'PIN' } | { type: 'UNPIN' } diff --git a/src/panel/task-group.tsx b/src/panel/task-group.tsx index a4f482d..8b87b84 100644 --- a/src/panel/task-group.tsx +++ b/src/panel/task-group.tsx @@ -51,10 +51,10 @@ export default React.memo(function TaskGroup({ group, result, pinned }: Props) { {group.tasks.map((task: Task) => { return ( ); })} diff --git a/src/panel/task-result/error-result.tsx b/src/panel/task-result/error-result.tsx index 20d50bc..f85bafb 100644 --- a/src/panel/task-result/error-result.tsx +++ b/src/panel/task-result/error-result.tsx @@ -45,7 +45,6 @@ export default function ErrorResultView({ task, result }: { task: Task; result: return ( diff --git a/src/panel/task-result/expanding-result.tsx b/src/panel/task-result/expanding-result.tsx index fb43956..3730a38 100644 --- a/src/panel/task-result/expanding-result.tsx +++ b/src/panel/task-result/expanding-result.tsx @@ -13,7 +13,6 @@ export type ExpandedArgs = { }; type Props = { - taskId: string; name: string; result: React.ReactNode; getExpanded: (args: ExpandedArgs) => React.ReactNode; @@ -77,7 +76,7 @@ function ExpandIcon({ isExpanded }: ExpandedArgs) { ); } -export function ExpandingResult({ taskId, name, result, getExpanded }: Props) { +export function ExpandingResult({ name, result, getExpanded }: Props) { const [isExpanded, setIsExpanded] = useState(false); const toggle = useCallback(() => setIsExpanded((value) => !value), [setIsExpanded]); const service = useRequiredContext(ServiceContext); @@ -101,7 +100,7 @@ export function ExpandingResult({ taskId, name, result, getExpanded }: Props) { secondary small disabled={!nextEventsInclude('START_ONE', state.nextEvents)} - onClick={() => send({ type: 'START_ONE', taskId })} + onClick={() => send({ type: 'START_ONE', taskName: name })} > Run task{' '} diff --git a/src/panel/task-result/index.tsx b/src/panel/task-result/index.tsx index 7260076..efc5bd3 100644 --- a/src/panel/task-result/index.tsx +++ b/src/panel/task-result/index.tsx @@ -19,25 +19,24 @@ export default function TaskResult({ task, result, pinned }: ResultProps) { } if (result.type === 'error') { - return ; + return ; } if (result.type === 'static') { invariant(task.type === 'static', `Unexpected task type: ${task.type}`); // Sometimes a pinned value can be an error. We don't want to compare against that const pin: Nullable = pinned && pinned.type === 'static' ? pinned : null; - return ; + return ; } if (result.type === 'timed') { invariant( task.type === 'timed' || task.type === 'interaction', - `Mismatched task -> result type - (Task [${task.taskId}:${task.type}] : Result [${result.taskId}:${result.type}]`, + `Unexpected task type: ${task.type}`, ); // Sometimes a pinned value can be an error. We don't want to compare against that const pin: Nullable = pinned && pinned.type === 'timed' ? pinned : null; - return ; + return ; } // eslint-disable-next-line no-console diff --git a/src/panel/task-result/static-result.tsx b/src/panel/task-result/static-result.tsx index 8c4afba..69c0c3b 100644 --- a/src/panel/task-result/static-result.tsx +++ b/src/panel/task-result/static-result.tsx @@ -89,7 +89,6 @@ export default function StaticResultView({ return ( diff --git a/src/panel/task-result/timed-result.tsx b/src/panel/task-result/timed-result.tsx index 2434404..72563b3 100644 --- a/src/panel/task-result/timed-result.tsx +++ b/src/panel/task-result/timed-result.tsx @@ -165,7 +165,6 @@ export default function TimedResultView({ task, pinned, result }: TimedProps) { return ( diff --git a/src/panel/use-panel-machine.tsx b/src/panel/use-panel-machine.tsx index 98d726c..52dbbb3 100644 --- a/src/panel/use-panel-machine.tsx +++ b/src/panel/use-panel-machine.tsx @@ -10,17 +10,16 @@ import { clearPinned, getPinned, savePinned } from './pinned-storage'; type MergeArgs = { existing: TaskGroupResult[]; - taskId: string; result: TimedResult | StaticResult; }; -function mergeWithResults({ existing, taskId, result }: MergeArgs): TaskGroupResult[] { +function mergeWithResults({ existing, result }: MergeArgs): TaskGroupResult[] { return existing.map((groupResult: TaskGroupResult) => { return { ...groupResult, map: { ...groupResult.map, - [taskId]: result, + [result.taskName]: result, }, }; }); @@ -54,12 +53,11 @@ export default function usePanelMachine(machine: MachineType, channel: Channel) service.send('FINISH', { results }); } - function finishOne({ taskId, result }: RunOne['Result']) { + function finishOne({ result }: RunOne['Result']) { const results: TaskGroupResult[] = mergeWithResults({ // we are using a state machine guard to prevent this existing: service.state.context.current.results!, result, - taskId, }); service.send('FINISH', { results }); } @@ -104,7 +102,7 @@ export default function usePanelMachine(machine: MachineType, channel: Channel) if (event.type === 'START_ONE') { channel.emit(eventNames.START_ONE, { - taskId: event.taskId, + taskName: event.taskName, samples, copies, }); diff --git a/src/task-runner/get-error-result.ts b/src/task-runner/get-error-result.ts index 8abed55..27a158d 100644 --- a/src/task-runner/get-error-result.ts +++ b/src/task-runner/get-error-result.ts @@ -5,14 +5,14 @@ export default function getErrorResult({ task, error }: { task: Task; error: any if (error instanceof UnsupportedError) { return { type: 'error', - taskId: task.taskId, + taskName: task.name, reason: 'unsupported', message: error.message, }; } return { type: 'error', - taskId: task.taskId, + taskName: task.name, reason: 'unhandled', message: null, }; diff --git a/src/task-runner/run-group.ts b/src/task-runner/run-group.ts index 7c96ed0..1bc69f3 100644 --- a/src/task-runner/run-group.ts +++ b/src/task-runner/run-group.ts @@ -10,7 +10,7 @@ import { TimedTask, ErrorResult, } from '../types'; -import toResultMap from '../util/to-result-map'; +import getResultMap from '../util/get-result-map'; import { asyncMap } from './async'; import { getResultForStaticTask } from './run-static-task'; import { getResultForTimedTask } from './run-timed-task'; @@ -58,7 +58,7 @@ export default async function runGroup({ const results: TaskGroupResult = { groupId: group.groupId, - map: toResultMap([...timedResults, ...staticResults, ...interactionResults]), + map: getResultMap([...timedResults, ...staticResults, ...interactionResults]), }; return results; diff --git a/src/task-runner/run-static-task.ts b/src/task-runner/run-static-task.ts index 34532b0..4f1c85d 100644 --- a/src/task-runner/run-static-task.ts +++ b/src/task-runner/run-static-task.ts @@ -20,7 +20,7 @@ export async function getResultForStaticTask({ const value: string = await runStaticTask({ task, getElement }); const result: StaticResult = { type: 'static', - taskId: task.taskId, + taskName: task.name, value, }; return result; diff --git a/src/task-runner/run-timed-task.ts b/src/task-runner/run-timed-task.ts index 9b634e2..fc6ecba 100644 --- a/src/task-runner/run-timed-task.ts +++ b/src/task-runner/run-timed-task.ts @@ -89,7 +89,7 @@ export async function getResultForTimedTask({ const result: TimedResult = { type: 'timed', - taskId: task.taskId, + taskName: task.name, averageMs: average, samples, variance: { diff --git a/src/tasks/get-groups.ts b/src/tasks/get-groups.ts index cf0b344..808b072 100644 --- a/src/tasks/get-groups.ts +++ b/src/tasks/get-groups.ts @@ -1,7 +1,22 @@ -import { PublicInteractionTask, AllowedGroup, TaskGroup } from '../types'; +import { PublicInteractionTask, AllowedGroup, TaskGroup, Task } from '../types'; import client from './preset/client'; import server from './preset/server'; import { getInteractionGroup } from './get-interaction-group'; +import flatten from '../util/flatten'; +import invariant from 'tiny-invariant'; + +function getDuplicateTaskNames(groups: TaskGroup[]): string[] { + const tasks: Task[] = flatten(groups.map((group) => group.tasks)); + const allNames: string[] = tasks.map((task) => task.name); + + const duplicates: string[] = allNames.filter((name: string) => { + return allNames.filter((value) => value === name).length > 1; + }); + + // duplicates will include two entries for every duplicate + // using a Set to trip out the duplicates + return [...new Set(duplicates)]; +} export function getGroups({ allowedGroups, @@ -21,5 +36,12 @@ export function getGroups({ result.push(getInteractionGroup(interactions)); + const duplicateNames: string[] = getDuplicateTaskNames(result); + + invariant( + !duplicateNames.length, + `Tasks found with duplicate names: [${duplicateNames.join(',')}]`, + ); + return result; } diff --git a/src/tasks/get-interaction-group.ts b/src/tasks/get-interaction-group.ts index 9c69369..e592697 100644 --- a/src/tasks/get-interaction-group.ts +++ b/src/tasks/get-interaction-group.ts @@ -8,7 +8,7 @@ export function getInteractionGroup(interactions: PublicInteractionTask[]): Task return { ...item, type: 'interaction', - taskId: `interaction::(index:${index})(name:${item.name})`, + name: item.name, description: item.description || '(None provided)', }; }, diff --git a/src/tasks/get-tasks-map.ts b/src/tasks/get-tasks-map.ts index 14159c3..9298d8f 100644 --- a/src/tasks/get-tasks-map.ts +++ b/src/tasks/get-tasks-map.ts @@ -2,8 +2,8 @@ import { Task, TaskGroup, TaskMap } from '../types'; import { flatten } from 'xstate/lib/utils'; export default function getTaskMap(groups: TaskGroup[]): TaskMap { - return flatten(groups.map((group) => group.tasks)).reduce((acc: TaskMap, item: Task) => { - acc[item.taskId] = item; + return flatten(groups.map((group) => group.tasks)).reduce((acc: TaskMap, task: Task) => { + acc[task.name] = task; return acc; }, {}); } diff --git a/src/tasks/preset/client.tsx b/src/tasks/preset/client.tsx index 6fd8285..5eebe8a 100644 --- a/src/tasks/preset/client.tsx +++ b/src/tasks/preset/client.tsx @@ -10,18 +10,19 @@ import { Nullable, AllowedGroup, } from '../../types'; -import { staticTask, timedTask } from './create'; import { UnsupportedError } from '../../task-runner/custom-errors'; -const render: TimedTask = timedTask({ +const render: TimedTask = { + type: 'timed', name: 'Initial render', description: `This task records how long ReactDOM.render() takes with your component`, run: async ({ getElement, container }: RunTimedTaskArgs): Promise => { ReactDOM.render(getElement(), container); }, -}); +}; -const hydrate: TimedTask = timedTask({ +const hydrate: TimedTask = { + type: 'timed', name: 'Hydrate', description: ` This task records how long a ReactDOMServer.hydrate() call @@ -39,9 +40,10 @@ const hydrate: TimedTask = timedTask({ ReactDOM.hydrate(getElement(), container); }); }, -}); +}; -const reRender: TimedTask = timedTask({ +const reRender: TimedTask = { + type: 'timed', name: 'Re render', description: ` This task records how long it takes to re-render the component with no prop changes. @@ -55,13 +57,14 @@ const reRender: TimedTask = timedTask({ ReactDOM.render(getElement(), container); }); }, -}); +}; function getAllChildren(container: HTMLElement): Element[] { return Array.from(container.querySelectorAll('*')); } -const domElementCount: StaticTask = staticTask({ +const domElementCount: StaticTask = { + type: 'static', name: 'DOM element count', description: ` The more DOM element your component creates, the more work the browser needs to do @@ -73,9 +76,10 @@ const domElementCount: StaticTask = staticTask({ return String(allChildren.length); }, -}); +}; -const domElementCountWithoutSvg: StaticTask = staticTask({ +const domElementCountWithoutSvg: StaticTask = { + type: 'static', name: 'DOM element count (no nested inline svg elements)', description: ` The count of DOM elements excluding inner SVG elements @@ -102,9 +106,10 @@ const domElementCountWithoutSvg: StaticTask = staticTask({ return String(allChildren.length); }, -}); +}; -const completeRender: TimedTask = timedTask({ +const completeRender: TimedTask = { + type: 'timed', name: 'Complete render (mount + layout + paint)', description: ` Time taken for the CPU to become idle after starting ReactDOM.render. @@ -123,7 +128,7 @@ const completeRender: TimedTask = timedTask({ idle(() => resolve()); }); }, -}); +}; interface Fiber { // Singly Linked List Tree Structure. @@ -149,7 +154,8 @@ export function traverse(rootNode: Fiber, callback: (node: Fiber) => void) { walk(rootNode); } -const reactFiberNodeCount: StaticTask = staticTask({ +const reactFiberNodeCount: StaticTask = { + type: 'static', name: 'React Fiber node count', description: ` The number of React Elements or internal objects ("fibers") that hold information about the component tree state. @@ -164,7 +170,7 @@ const reactFiberNodeCount: StaticTask = staticTask({ return String(count); }, -}); +}; const group: TaskGroup = { groupId: 'Client', diff --git a/src/tasks/preset/create.ts b/src/tasks/preset/create.ts deleted file mode 100644 index 6aec180..0000000 --- a/src/tasks/preset/create.ts +++ /dev/null @@ -1,23 +0,0 @@ -import { StaticTask, TimedTask } from '../../types'; - -let count: number = 0; - -function getUniqueId(): string { - return `preset::unique-id:${count++}`; -} - -export function timedTask(args: Omit): TimedTask { - return { - taskId: getUniqueId(), - type: 'timed', - ...args, - }; -} - -export function staticTask(args: Omit): StaticTask { - return { - taskId: getUniqueId(), - type: 'static', - ...args, - }; -} diff --git a/src/tasks/preset/server.tsx b/src/tasks/preset/server.tsx index d67d6fc..e309fc9 100644 --- a/src/tasks/preset/server.tsx +++ b/src/tasks/preset/server.tsx @@ -2,17 +2,18 @@ import ReactDOMServer from 'react-dom/server'; import gzip from 'gzip-js'; import { StaticTask, TimedTask, TaskGroup, RunStaticTaskArgs, RunTimedTaskArgs } from '../../types'; import { bytesToKiloBytes } from '../../util/convert-bytes-to'; -import { timedTask, staticTask } from './create'; -const renderToString: TimedTask = timedTask({ +const renderToString: TimedTask = { + type: 'timed', name: 'Render to string', description: `This task records how long a ReactDOM.renderToString() call takes`, run: async ({ getElement }: RunTimedTaskArgs): Promise => { ReactDOMServer.renderToString(getElement()); }, -}); +}; -const renderToStaticMarkup: TimedTask = timedTask({ +const renderToStaticMarkup: TimedTask = { + type: 'timed', name: 'Render to static markup (cannot be hydrated)', description: ` This task records how long a ReactDOM.renderToStaticMarkup() call takes. @@ -21,9 +22,10 @@ const renderToStaticMarkup: TimedTask = timedTask({ run: async ({ getElement }: RunTimedTaskArgs): Promise => { ReactDOMServer.renderToStaticMarkup(getElement()); }, -}); +}; -const getRawStringSizeInKB: StaticTask = staticTask({ +const getRawStringSizeInKB: StaticTask = { + type: 'static', name: 'String output size', description: ` The size of the string generated by ReactDOM.renderToString(). @@ -34,9 +36,10 @@ const getRawStringSizeInKB: StaticTask = staticTask({ const blob: Blob = new Blob([output]); return bytesToKiloBytes(blob.size); }, -}); +}; -const getGzipStringSizeInKB: StaticTask = staticTask({ +const getGzipStringSizeInKB: StaticTask = { + type: 'static', name: 'String output size (gzip)', description: ` The gzipped size of the string generated by ReactDOM.renderToString(). @@ -50,9 +53,10 @@ const getGzipStringSizeInKB: StaticTask = staticTask({ return bytesToKiloBytes(bytes); }, -}); +}; -const getRawStaticMarkupSizeInKB: StaticTask = staticTask({ +const getRawStaticMarkupSizeInKB: StaticTask = { + type: 'static', name: 'Static markup output size', description: ` The size of the string generated by ReactDOM.renderToStaticMarkup(). @@ -63,9 +67,10 @@ const getRawStaticMarkupSizeInKB: StaticTask = staticTask({ const blob: Blob = new Blob([output]); return bytesToKiloBytes(blob.size); }, -}); +}; -const getGzipStaticMarkupSizeInKB: StaticTask = staticTask({ +const getGzipStaticMarkupSizeInKB: StaticTask = { + type: 'static', name: 'Static markup output size (gzip)', description: ` The gzipped size of the string generated by ReactDOM.renderToStaticMarkup(). @@ -79,7 +84,7 @@ const getGzipStaticMarkupSizeInKB: StaticTask = staticTask({ return bytesToKiloBytes(bytes); }, -}); +}; const group: TaskGroup = { groupId: 'Server', diff --git a/src/types.ts b/src/types.ts index eb4ea4a..b585659 100644 --- a/src/types.ts +++ b/src/types.ts @@ -5,7 +5,6 @@ export type Nullable = T | null; export type Combine = Omit & Second; type BaseTask = { - taskId: string; name: string; description: string; }; @@ -53,7 +52,7 @@ export type InteractionTask = BaseTask & { }; // This is what is provided as interactions to the addon by a consumer -export type PublicInteractionTask = Omit & { +export type PublicInteractionTask = Omit & { description?: string; }; @@ -73,7 +72,7 @@ export type Variance = { export type TimedResult = { type: 'timed'; - taskId: string; + taskName: string; averageMs: number; samples: number; variance: Variance; @@ -81,13 +80,13 @@ export type TimedResult = { export type StaticResult = { type: 'static'; - taskId: string; + taskName: string; value: string; }; export type ErrorResult = { type: 'error'; - taskId: string; + taskName: string; reason: 'unsupported' | 'unhandled'; message: Nullable; }; @@ -95,11 +94,11 @@ export type ErrorResult = { export type Result = TimedResult | StaticResult | ErrorResult; export type ResultMap = { - [taskId: string]: Result; + [taskName: string]: Result; }; export type TaskMap = { - [taskId: string]: Task; + [taskName: string]: Task; }; export type TaskGroupResult = { diff --git a/src/util/get-result-map.ts b/src/util/get-result-map.ts new file mode 100644 index 0000000..f4ba70c --- /dev/null +++ b/src/util/get-result-map.ts @@ -0,0 +1,6 @@ +export default function getResultMap(list: T[]): Record { + return list.reduce((acc: Record, item: T) => { + acc[item.taskName] = item; + return acc; + }, {}); +} diff --git a/src/util/to-result-map.ts b/src/util/to-result-map.ts deleted file mode 100644 index 183dc96..0000000 --- a/src/util/to-result-map.ts +++ /dev/null @@ -1,6 +0,0 @@ -export default function toResultMap(list: T[]): Record { - return list.reduce((acc: Record, item: T) => { - acc[item.taskId] = item; - return acc; - }, {}); -} diff --git a/test-util/mocks.ts b/test-util/mocks.ts index 9151096..cf27222 100644 --- a/test-util/mocks.ts +++ b/test-util/mocks.ts @@ -9,102 +9,118 @@ export const groupResults: TaskGroupResult[] = [ { groupId: 'Server', map: { - 'preset::unique-id:7': { + 'Render to string': { type: 'timed', - taskId: 'preset::unique-id:7', - averageMs: 16.347500003757887, + taskName: 'Render to string', + averageMs: 18.437499995343387, samples: 10, variance: { - upperPercentage: 23.74980877795315, - lowerPercentage: 15.981036783323018, - standardDeviation: 2.3455247299536865, + upperPercentage: 15.037288040689816, + lowerPercentage: 16.474576229128438, + standardDeviation: 2.024594097840528, }, }, - 'preset::unique-id:8': { + 'Render to static markup (cannot be hydrated)': { type: 'timed', - taskId: 'preset::unique-id:8', - averageMs: 14.008499993360601, + taskName: 'Render to static markup (cannot be hydrated)', + averageMs: 14.930999994976446, samples: 10, variance: { - upperPercentage: 14.39483169140364, - lowerPercentage: 7.413356264227891, - standardDeviation: 1.011266660364238, + upperPercentage: 14.258924315167054, + lowerPercentage: 6.43627349358861, + standardDeviation: 0.9656339848818386, }, }, - 'preset::unique-id:9': { type: 'static', taskId: 'preset::unique-id:9', value: '14.33' }, - 'preset::unique-id:10': { type: 'static', taskId: 'preset::unique-id:10', value: '0.84' }, - 'preset::unique-id:11': { + 'String output size': { type: 'static', taskName: 'String output size', value: '14.33' }, + 'String output size (gzip)': { type: 'static', - taskId: 'preset::unique-id:11', + taskName: 'String output size (gzip)', + value: '0.84', + }, + 'Static markup output size': { + type: 'static', + taskName: 'Static markup output size', value: '14.33', }, - 'preset::unique-id:12': { type: 'static', taskId: 'preset::unique-id:12', value: '0.84' }, + 'Static markup output size (gzip)': { + type: 'static', + taskName: 'Static markup output size (gzip)', + value: '0.84', + }, }, }, { groupId: 'Client', map: { - 'preset::unique-id:0': { + 'Initial render': { type: 'timed', - taskId: 'preset::unique-id:0', - averageMs: 33.7085000006482, + taskName: 'Initial render', + averageMs: 45.291499997256324, samples: 10, variance: { - upperPercentage: 53.58144085302733, - lowerPercentage: 28.32668317966413, - standardDeviation: 9.431718042927884, + upperPercentage: 10.39599039861205, + lowerPercentage: 11.79360363822285, + standardDeviation: 3.5473680738394884, }, }, - 'preset::unique-id:2': { + 'Re render': { type: 'timed', - taskId: 'preset::unique-id:2', - averageMs: 17.56850000238046, + taskName: 'Re render', + averageMs: 22.232000005897135, samples: 10, variance: { - upperPercentage: 17.084554688937313, - lowerPercentage: 21.450322945551903, - standardDeviation: 1.8218507749456447, + upperPercentage: 15.486685880162348, + lowerPercentage: 12.243612775999072, + standardDeviation: 1.7607302461130605, }, }, - 'preset::unique-id:1': { + Hydrate: { type: 'timed', - taskId: 'preset::unique-id:1', - averageMs: 24.524500002735294, + taskName: 'Hydrate', + averageMs: 38.209999998798594, samples: 10, variance: { - upperPercentage: 25.079002564046572, - lowerPercentage: 8.703541373848548, - standardDeviation: 2.242791172841424, + upperPercentage: 5.626799229970686, + lowerPercentage: 5.770740636265473, + standardDeviation: 1.3656793121343955, }, }, - 'preset::unique-id:5': { + 'Complete render (mount + layout + paint)': { type: 'timed', - taskId: 'preset::unique-id:5', - averageMs: 36.793999999645166, + taskName: 'Complete render (mount + layout + paint)', + averageMs: 48.659499996574596, samples: 10, variance: { - upperPercentage: 34.87253360515639, - lowerPercentage: 11.656791839848179, - standardDeviation: 6.15493493492205, + upperPercentage: 3.2480810239156224, + lowerPercentage: 2.2390283530976323, + standardDeviation: 0.6435621538816768, }, }, - 'preset::unique-id:3': { type: 'static', taskId: 'preset::unique-id:3', value: '130' }, - 'preset::unique-id:4': { type: 'static', taskId: 'preset::unique-id:4', value: '120' }, - 'preset::unique-id:6': { type: 'static', taskId: 'preset::unique-id:6', value: '461' }, + 'DOM element count': { type: 'static', taskName: 'DOM element count', value: '130' }, + 'DOM element count (no nested inline svg elements)': { + type: 'static', + taskName: 'DOM element count (no nested inline svg elements)', + value: '120', + }, + 'React Fiber node count': { + type: 'static', + taskName: 'React Fiber node count', + value: '461', + }, }, }, { groupId: 'Interactions', map: { - 'interaction::(index:0)(name:Display dropdown)': { + 'Display dropdown': { type: 'timed', - taskId: 'interaction::(index:0)(name:Display dropdown)', - averageMs: 177.05949999799486, + taskName: 'Display dropdown', + averageMs: 250.1429999974789, samples: 10, variance: { - upperPercentage: 8.810879960011455, - lowerPercentage: 7.282580151564158, - standardDeviation: 9.851067060814993, + upperPercentage: 22.240078670701017, + lowerPercentage: 7.624838587831113, + standardDeviation: 21.225114623903043, }, }, }, diff --git a/test/panel/task-running-guards.spec.tsx b/test/panel/task-running-guards.spec.tsx index 43a6118..c01aaba 100644 --- a/test/panel/task-running-guards.spec.tsx +++ b/test/panel/task-running-guards.spec.tsx @@ -10,6 +10,7 @@ import WithStorybookTheme from '../../test-util/with-storybook-theme'; import * as mocks from '../../test-util/mocks'; import eventNames from '../../src/events'; import allowAllGroups from '../../src/tasks/allow-all-groups'; +import getResultMap from '../../src/util/get-result-map'; beforeAll(() => localStorage.clear()); afterEach(() => localStorage.clear()); @@ -44,7 +45,9 @@ it('should prevent starting another task when running a task', () => { assertTopbar({ container, isEnabled: true }); // 5: Run a single task - fireEvent.click(getByText('Render to string')); + const taskName: string = 'Render to string'; + + fireEvent.click(getByText(taskName)); const runSingle: HTMLElement = getByText('Run task'); expect(runSingle.matches(':enabled')).toBe(true); fireEvent.click(runSingle); @@ -53,11 +56,10 @@ it('should prevent starting another task when running a task', () => { // top bar now disabled assertTopbar({ container, isEnabled: false }); // emit a result - const taskId: string = 'preset::unique-id:6'; act(() => channel.emit(eventNames.FINISH_ONE, { - taskId, - result: mocks.groupResults.map['preset::unique-id:6'], + taskName, + result: mocks.groupResults[0].map[taskName], }), ); // topbar and run button now enabled diff --git a/test/tasks/mark.spec.ts b/test/tasks/mark.spec.ts index 5558c8f..08da517 100644 --- a/test/tasks/mark.spec.ts +++ b/test/tasks/mark.spec.ts @@ -9,7 +9,6 @@ beforeEach(() => { it('should wrap a task in a performance.mark', async () => { const task: StaticTask = { - taskId: 'task', type: 'static', description: 'task', name: 'task', @@ -32,7 +31,6 @@ it('should wrap a task in a performance.mark', async () => { it('should still use performance.mark even when there is an error in the task', async () => { const task: StaticTask = { - taskId: 'task', type: 'static', description: 'task', name: 'task', diff --git a/test/tasks/run-all.spec.ts b/test/tasks/run-all.spec.ts index 5d7e3db..8d9e2f1 100644 --- a/test/tasks/run-all.spec.ts +++ b/test/tasks/run-all.spec.ts @@ -1,7 +1,7 @@ import { runAll } from '../../src/task-runner'; import preset from '../../src/tasks/preset'; import { Task, TaskGroup, TaskGroupResult, TimedResult, AllowedGroup } from '../../src/types'; -import toResultMap from '../../src/util/to-result-map'; +import getResultMap from '../../src/util/get-result-map'; import { StaticResult } from '../../src/types'; it('should run all the standard tests', async () => { @@ -20,7 +20,7 @@ it('should run all the standard tests', async () => { (task: Task): StaticResult => { return { type: 'static', - taskId: task.taskId, + taskName: task.name, value: expect.any(String) as string, }; }, @@ -31,7 +31,7 @@ it('should run all the standard tests', async () => { (task: Task): TimedResult => { return { type: 'timed', - taskId: task.taskId, + taskName: task.name, averageMs: expect.any(Number) as number, samples: 3, variance: { @@ -45,7 +45,7 @@ it('should run all the standard tests', async () => { return { groupId: group.groupId, - map: toResultMap([...staticResults, ...timedResults]), + map: getResultMap([...staticResults, ...timedResults]), }; }, ); diff --git a/test/tasks/run-one-static.spec.ts b/test/tasks/run-one-static.spec.ts index 9a274b9..9c4b453 100644 --- a/test/tasks/run-one-static.spec.ts +++ b/test/tasks/run-one-static.spec.ts @@ -7,7 +7,6 @@ it('should run static tests', async () => { const runMock = jest.fn().mockImplementation(() => Promise.resolve(returnValue)); const task: StaticTask = { - taskId: 'task', type: 'static', description: 'task', name: 'task', @@ -22,7 +21,7 @@ it('should run static tests', async () => { const expected: StaticResult = { type: 'static', - taskId: task.taskId, + taskName: task.name, value: returnValue, }; diff --git a/test/tasks/run-one-timed.spec.ts b/test/tasks/run-one-timed.spec.ts index db2554e..0d1023f 100644 --- a/test/tasks/run-one-timed.spec.ts +++ b/test/tasks/run-one-timed.spec.ts @@ -8,7 +8,6 @@ it('should run one timed task', async () => { const task: TimedTask = { name: 'task', type: 'timed', - taskId: 'task', description: 'task', run: ({ getElement }: RunTimedTaskArgs) => Promise.resolve().then(() => mock(getElement())), }; @@ -22,7 +21,7 @@ it('should run one timed task', async () => { }); const expected: TimedResult = { - taskId: task.taskId, + taskName: task.name, type: 'timed', averageMs: expect.any(Number) as number, samples, diff --git a/test/tasks/story-function.spec.tsx b/test/tasks/story-function.spec.tsx index b7f3810..f8ec246 100644 --- a/test/tasks/story-function.spec.tsx +++ b/test/tasks/story-function.spec.tsx @@ -1,10 +1,10 @@ import React, { useState } from 'react'; import ReactDOMServer from 'react-dom/server'; import { runOneStatic } from '../../src/task-runner'; -import { StaticResult, StaticTask } from '../../src/types'; +import { StaticResult, StaticTask, ErrorResult } from '../../src/types'; +import invariant from 'tiny-invariant'; const task: StaticTask = { - taskId: 'task', type: 'static', description: 'task', name: 'task', @@ -19,17 +19,18 @@ it('should support story functions that return components (default)', async () = return
{count}
; } - const result: StaticResult = await runOneStatic({ + const result: StaticResult | ErrorResult = await runOneStatic({ task, getNode: () => , copies: 1, }); + invariant(result.type === 'static'); expect(result.value).toBe('
0
'); }); it('should support story functions that have hooks', async () => { - const result: StaticResult = await runOneStatic({ + const result: StaticResult | ErrorResult = await runOneStatic({ task, getNode: () => { // eslint-disable-next-line react-hooks/rules-of-hooks @@ -39,6 +40,7 @@ it('should support story functions that have hooks', async () => { copies: 1, }); + invariant(result.type === 'static'); expect(result.value).toBe('
0
'); }); @@ -48,21 +50,23 @@ it('should support story functions are just components', async () => { return
{count}
; } - const result: StaticResult = await runOneStatic({ + const result: StaticResult | ErrorResult = await runOneStatic({ task, getNode: App, copies: 1, }); + invariant(result.type === 'static'); expect(result.value).toBe('
0
'); }); it('should support a story function that returns null', async () => { - const result: StaticResult = await runOneStatic({ + const result: StaticResult | ErrorResult = await runOneStatic({ task, getNode: () => null, copies: 1, }); + invariant(result.type === 'static'); expect(result.value).toBe(''); }); diff --git a/test/tasks/time-controls.spec.tsx b/test/tasks/time-controls.spec.tsx index 5bea73e..d9e8a9d 100644 --- a/test/tasks/time-controls.spec.tsx +++ b/test/tasks/time-controls.spec.tsx @@ -13,7 +13,6 @@ function getArgs({ run }: { run: TimedTask['run'] }): RunOneTimedTaskArgs { type: 'timed', name: 'task', description: '', - taskId: 'task', run, }; diff --git a/test/tasks/timed-task-bail-on-error.spec.ts b/test/tasks/timed-task-bail-on-error.spec.ts index 3b20755..5b5d3a9 100644 --- a/test/tasks/timed-task-bail-on-error.spec.ts +++ b/test/tasks/timed-task-bail-on-error.spec.ts @@ -7,7 +7,6 @@ it('should not repeat a timed task if an error occurred in one', async () => { type: 'timed', name: 'task', description: '', - taskId: 'task', run: async (): Promise => { mock(); throw new Error('boom'); diff --git a/test/tasks/unsupported-errors.spec.ts b/test/tasks/unsupported-errors.spec.ts index 0f5c7f6..ee0599b 100644 --- a/test/tasks/unsupported-errors.spec.ts +++ b/test/tasks/unsupported-errors.spec.ts @@ -5,7 +5,6 @@ import { ErrorResult, StaticResult, StaticTask, TimedTask, TimedResult } from '. it('should list the error as unsupported in an UnsupportedError is thrown (static task)', async () => { const message: string = 'My custom unsupported error message'; const task: StaticTask = { - taskId: 'task', type: 'static', description: 'task', name: 'task', @@ -22,7 +21,7 @@ it('should list the error as unsupported in an UnsupportedError is thrown (stati const expected: ErrorResult = { type: 'error', - taskId: task.taskId, + taskName: task.name, reason: 'unsupported', message, }; @@ -33,7 +32,6 @@ it('should list the error as unsupported in an UnsupportedError is thrown (stati it('should list the error as unsupported in an UnsupportedError is thrown (timed task)', async () => { const message: string = 'My custom unsupported error message'; const task: TimedTask = { - taskId: 'task', type: 'timed', description: 'task', name: 'task', @@ -51,7 +49,7 @@ it('should list the error as unsupported in an UnsupportedError is thrown (timed const expected: ErrorResult = { type: 'error', - taskId: task.taskId, + taskName: task.name, reason: 'unsupported', message, }; From f38e0b85d7a9faafcfe204f6c6173f5e9cab1bb9 Mon Sep 17 00:00:00 2001 From: Alex Reardon Date: Fri, 7 Aug 2020 16:20:21 +1000 Subject: [PATCH 2/3] adding some tests for local storage usage --- src/panel/pinned-storage.ts | 67 ++++++++++++++++++++++++++----------- test/pinned-storage.spec.ts | 50 +++++++++++++++++++++++++++ 2 files changed, 97 insertions(+), 20 deletions(-) create mode 100644 test/pinned-storage.spec.ts diff --git a/src/panel/pinned-storage.ts b/src/panel/pinned-storage.ts index 01918e6..270ba2c 100644 --- a/src/panel/pinned-storage.ts +++ b/src/panel/pinned-storage.ts @@ -1,27 +1,47 @@ import { RunContext } from './machine'; import { packageName } from '../addon-constants'; -import { Nullable, TaskGroupResult, Combine, TaskGroup } from '../types'; +import { Nullable, TaskGroupResult, Combine, TaskGroup, ResultMap } from '../types'; -function upgrade(context: RunContext): Nullable { - if (!context.results) { - return context; +function hasProperty(value: Record, key: string): boolean { + return Object.prototype.hasOwnProperty.call(value, key); +} + +function isValidContext(value: unknown): value is RunContext { + if (typeof value !== 'object') { + return false; + } + if (value == null) { + return false; + } + if (Array.isArray(value)) { + return false; + } + + // Duck typing: + const hasAllProperties: boolean = ['results', 'samples', 'copies'].every((key) => + hasProperty(value, key), + ); + + if (!hasAllProperties) { + return false; } - // Changed TaskGroup.groupName to TaskGroup.groupId - return { - ...context, - results: context.results.map( - (result: any): TaskGroupResult => { - if (result.groupName) { - return { - groupId: result.groupName, - map: result.map, - }; - } - return result; - }, - ), - }; + // Now going to get any result and ensure it doesn't have a 'taskId' + // if it does have a taskId then the entry is out of date + const map: ResultMap | undefined = + // @ts-ignore + value && value.results && value.results[0] ? value.results[0].map : undefined; + + if (map == null) { + return false; + } + + const hasTaskId: boolean = Object.keys(map).some((key) => { + const entry = map[key]; + return hasProperty(entry, 'taskId'); + }); + + return hasTaskId ? false : true; } function getKey(storyName: string) { @@ -43,5 +63,12 @@ export function getPinned(storyName: string): Nullable { return null; } - return upgrade(JSON.parse(raw)); + const value: any = JSON.parse(raw); + if (!isValidContext(value)) { + console.warn('Unsupported value found in localStorage. Value cleared'); + clearPinned(storyName); + return null; + } + + return value; } diff --git a/test/pinned-storage.spec.ts b/test/pinned-storage.spec.ts new file mode 100644 index 0000000..a2da521 --- /dev/null +++ b/test/pinned-storage.spec.ts @@ -0,0 +1,50 @@ +import { Nullable } from '../src/types'; +import { savePinned, getPinned } from '../src/panel/pinned-storage'; +import * as mock from '../test-util/mocks'; +import { RunContext } from '../src/panel/machine'; + +beforeEach(() => { + localStorage.clear(); + jest.spyOn(console, 'warn').mockImplementation(() => {}); +}); +afterEach(() => { + localStorage.clear(); + // @ts-ignore + console.warn.mockRestore(); +}); + +it('should load supported pinned values', () => { + const first: Nullable = getPinned(mock.storyName); + + expect(first).toBe(null); + + savePinned(mock.storyName, mock.runContext); + const second: Nullable = getPinned(mock.storyName); + + expect(second).toEqual(mock.runContext); + expect(console.warn).not.toHaveBeenCalled(); +}); + +it('should not load unsupported pinned values', () => { + const first: Nullable = getPinned(mock.storyName); + + savePinned(mock.storyName, { foo: 'bar' } as any); + const second: Nullable = getPinned(mock.storyName); + + expect(second).toBe(null); + expect(console.warn).toHaveBeenCalled(); +}); + +it('should not load unsupported pinned values: old value', () => { + const first: Nullable = getPinned(mock.storyName); + + const clone: RunContext = JSON.parse(JSON.stringify(mock.runContext)); + // @ts-ignore + clone.results[0].map['Render to string'].taskId = 'unsupported old taskId'; + + savePinned(mock.storyName, clone); + const second: Nullable = getPinned(mock.storyName); + + expect(second).toBe(null); + expect(console.warn).toHaveBeenCalled(); +}); From 60ee42af0064c0471efd4df282497be8b42a4da0 Mon Sep 17 00:00:00 2001 From: Alex Reardon Date: Fri, 7 Aug 2020 16:41:55 +1000 Subject: [PATCH 3/3] fixing lint violations --- src/panel/pinned-storage.ts | 1 + test/pinned-storage.spec.ts | 1 + 2 files changed, 2 insertions(+) diff --git a/src/panel/pinned-storage.ts b/src/panel/pinned-storage.ts index 270ba2c..8a1264d 100644 --- a/src/panel/pinned-storage.ts +++ b/src/panel/pinned-storage.ts @@ -65,6 +65,7 @@ export function getPinned(storyName: string): Nullable { const value: any = JSON.parse(raw); if (!isValidContext(value)) { + // eslint-disable-next-line no-console console.warn('Unsupported value found in localStorage. Value cleared'); clearPinned(storyName); return null; diff --git a/test/pinned-storage.spec.ts b/test/pinned-storage.spec.ts index a2da521..b71e67e 100644 --- a/test/pinned-storage.spec.ts +++ b/test/pinned-storage.spec.ts @@ -1,3 +1,4 @@ +/* eslint-disable no-console */ import { Nullable } from '../src/types'; import { savePinned, getPinned } from '../src/panel/pinned-storage'; import * as mock from '../test-util/mocks';