From eadf3f27c7d8d649817910cb2da33406b6873ead Mon Sep 17 00:00:00 2001 From: Milan Raj Date: Tue, 19 Sep 2023 12:49:29 -0500 Subject: [PATCH] Updated parameterized test pattern (#1379) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # Pull Request ## ๐Ÿคจ Rationale Updates the parametrized test pattern to address a few shortcomings in the previous pattern: - Avoids a lot of duplicated boilerplate - Doesn't encourage an approach that results in eslint disables being used often (false positive of await used in a loop) - Prevents accidentally focusing a test and checking it in - Gives decent typing, may take run-time to see if it needs improvements ## ๐Ÿ‘ฉโ€๐Ÿ’ป Implementation Exposed new parameterization helpers named `parameterize` and `parameterizeNamedList` and marked the existing method deprecated. Created a tech debt task to track migrating to the new pattern: https://github.com/ni/nimble/issues/1551 ## ๐Ÿงช Testing Added some tests for the new helpers and migrated a couple of existing tests to the new pattern. ## โœ… Checklist - [x] I have updated the project documentation to reflect my changes or determined no changes are needed. Added detailed comments to the helper like the deprecated helper did. --- ...-5528f3eb-a2ce-4e80-9df4-6ddb04f8b015.json | 7 + .../anchor-button/tests/anchor-button.spec.ts | 26 +- .../src/anchor-tabs/tests/anchor-tabs.spec.ts | 73 ++--- .../src/utilities/tests/parameterized.ts | 159 ++++++++--- .../utilities/tests/tests/parametized.spec.ts | 259 ++++++++++++++++++ 5 files changed, 438 insertions(+), 86 deletions(-) create mode 100644 change/@ni-nimble-components-5528f3eb-a2ce-4e80-9df4-6ddb04f8b015.json create mode 100644 packages/nimble-components/src/utilities/tests/tests/parametized.spec.ts diff --git a/change/@ni-nimble-components-5528f3eb-a2ce-4e80-9df4-6ddb04f8b015.json b/change/@ni-nimble-components-5528f3eb-a2ce-4e80-9df4-6ddb04f8b015.json new file mode 100644 index 0000000000..4f80d95663 --- /dev/null +++ b/change/@ni-nimble-components-5528f3eb-a2ce-4e80-9df4-6ddb04f8b015.json @@ -0,0 +1,7 @@ +{ + "type": "none", + "comment": "Parameterize test pattern", + "packageName": "@ni/nimble-components", + "email": "rajsite@users.noreply.github.com", + "dependentChangeType": "none" +} diff --git a/packages/nimble-components/src/anchor-button/tests/anchor-button.spec.ts b/packages/nimble-components/src/anchor-button/tests/anchor-button.spec.ts index 171230620f..6a152617f8 100644 --- a/packages/nimble-components/src/anchor-button/tests/anchor-button.spec.ts +++ b/packages/nimble-components/src/anchor-button/tests/anchor-button.spec.ts @@ -2,7 +2,7 @@ import { html } from '@microsoft/fast-element'; import { AnchorButton } from '..'; import { waitForUpdatesAsync } from '../../testing/async-helpers'; import { fixture, Fixture } from '../../utilities/tests/fixture'; -import { getSpecTypeByNamedList } from '../../utilities/tests/parameterized'; +import { parameterizeNamedList } from '../../utilities/tests/parameterized'; async function setup(): Promise> { return fixture( @@ -49,7 +49,7 @@ describe('AnchorButton', () => { expect(element.control!.getAttribute('href')).toBeNull(); }); - const attributeNames: { name: string }[] = [ + const attributeNames = [ { name: 'download' }, { name: 'hreflang' }, { name: 'ping' }, @@ -77,27 +77,17 @@ describe('AnchorButton', () => { { name: 'aria-owns' }, { name: 'aria-relevant' }, { name: 'aria-roledescription' } - ]; + ] as const; describe('should reflect value to the internal control', () => { - const focused: string[] = []; - const disabled: string[] = []; - for (const attribute of attributeNames) { - const specType = getSpecTypeByNamedList( - attribute, - focused, - disabled - ); - // eslint-disable-next-line @typescript-eslint/no-loop-func - specType(`for attribute ${attribute.name}`, async () => { + parameterizeNamedList(attributeNames, (spec, name) => { + spec(`for attribute ${name}`, async () => { await connect(); - element.setAttribute(attribute.name, 'foo'); + element.setAttribute(name, 'foo'); await waitForUpdatesAsync(); - expect(element.control!.getAttribute(attribute.name)).toBe( - 'foo' - ); + expect(element.control!.getAttribute(name)).toBe('foo'); }); - } + }); }); }); diff --git a/packages/nimble-components/src/anchor-tabs/tests/anchor-tabs.spec.ts b/packages/nimble-components/src/anchor-tabs/tests/anchor-tabs.spec.ts index 1bfb03c2fc..62fadfc126 100644 --- a/packages/nimble-components/src/anchor-tabs/tests/anchor-tabs.spec.ts +++ b/packages/nimble-components/src/anchor-tabs/tests/anchor-tabs.spec.ts @@ -13,7 +13,7 @@ import '../../anchor-tab'; import type { AnchorTab } from '../../anchor-tab'; import { waitForUpdatesAsync } from '../../testing/async-helpers'; import { fixture, Fixture } from '../../utilities/tests/fixture'; -import { getSpecTypeByNamedList } from '../../utilities/tests/parameterized'; +import { parameterizeNamedList } from '../../utilities/tests/parameterized'; describe('AnchorTabs', () => { let element: AnchorTabs; @@ -182,46 +182,51 @@ describe('AnchorTabs', () => { await disconnect(); }); - const navigationTests: { - name: string, - disabledIndex?: number, - hiddenIndex?: number, - initialFocusIndex: number, - keyName: string, - expectedFinalFocusIndex: number - }[] = [ + const navigationTests = [ { name: 'should focus next tab when right arrow key pressed', + disabledIndex: undefined, + hiddenIndex: undefined, initialFocusIndex: 0, keyName: keyArrowRight, expectedFinalFocusIndex: 1 }, { name: 'should focus previous tab when left arrow key pressed', + disabledIndex: undefined, + hiddenIndex: undefined, initialFocusIndex: 1, keyName: keyArrowLeft, expectedFinalFocusIndex: 0 }, { name: 'should wrap to first tab when arrowing right on last tab', + disabledIndex: undefined, + hiddenIndex: undefined, initialFocusIndex: 2, keyName: keyArrowRight, expectedFinalFocusIndex: 0 }, { name: 'should wrap to last tab when arrowing left on first tab', + disabledIndex: undefined, + hiddenIndex: undefined, initialFocusIndex: 0, keyName: keyArrowLeft, expectedFinalFocusIndex: 2 }, { name: 'should focus first tab when Home key pressed', + disabledIndex: undefined, + hiddenIndex: undefined, initialFocusIndex: 1, keyName: keyHome, expectedFinalFocusIndex: 0 }, { name: 'should focus last tab when End key pressed', + disabledIndex: undefined, + hiddenIndex: undefined, initialFocusIndex: 1, keyName: keyEnd, expectedFinalFocusIndex: 2 @@ -229,6 +234,7 @@ describe('AnchorTabs', () => { { name: 'should skip disabled tab when arrowing right', disabledIndex: 1, + hiddenIndex: undefined, initialFocusIndex: 0, keyName: keyArrowRight, expectedFinalFocusIndex: 2 @@ -236,6 +242,7 @@ describe('AnchorTabs', () => { { name: 'should skip disabled tab when arrowing left', disabledIndex: 1, + hiddenIndex: undefined, initialFocusIndex: 2, keyName: keyArrowLeft, expectedFinalFocusIndex: 0 @@ -243,6 +250,7 @@ describe('AnchorTabs', () => { { name: 'should skip disabled when arrowing right on last tab', disabledIndex: 0, + hiddenIndex: undefined, initialFocusIndex: 2, keyName: keyArrowRight, expectedFinalFocusIndex: 1 @@ -250,6 +258,7 @@ describe('AnchorTabs', () => { { name: 'should skip disabled when arrowing left on first tab', disabledIndex: 2, + hiddenIndex: undefined, initialFocusIndex: 0, keyName: keyArrowLeft, expectedFinalFocusIndex: 1 @@ -257,6 +266,7 @@ describe('AnchorTabs', () => { { name: 'should focus first enabled tab when Home key pressed', disabledIndex: 0, + hiddenIndex: undefined, initialFocusIndex: 2, keyName: keyHome, expectedFinalFocusIndex: 1 @@ -264,12 +274,14 @@ describe('AnchorTabs', () => { { name: 'should focus last enabled tab when End key pressed', disabledIndex: 2, + hiddenIndex: undefined, initialFocusIndex: 0, keyName: keyEnd, expectedFinalFocusIndex: 1 }, { name: 'should skip hidden tab when arrowing right', + disabledIndex: undefined, hiddenIndex: 1, initialFocusIndex: 0, keyName: keyArrowRight, @@ -277,6 +289,7 @@ describe('AnchorTabs', () => { }, { name: 'should skip hidden tab when arrowing left', + disabledIndex: undefined, hiddenIndex: 1, initialFocusIndex: 2, keyName: keyArrowLeft, @@ -284,6 +297,7 @@ describe('AnchorTabs', () => { }, { name: 'should skip hidden when arrowing right on last tab', + disabledIndex: undefined, hiddenIndex: 0, initialFocusIndex: 2, keyName: keyArrowRight, @@ -291,6 +305,7 @@ describe('AnchorTabs', () => { }, { name: 'should skip hidden when arrowing left on first tab', + disabledIndex: undefined, hiddenIndex: 2, initialFocusIndex: 0, keyName: keyArrowLeft, @@ -298,6 +313,7 @@ describe('AnchorTabs', () => { }, { name: 'should focus first visible tab when Home key pressed', + disabledIndex: undefined, hiddenIndex: 0, initialFocusIndex: 2, keyName: keyHome, @@ -305,45 +321,38 @@ describe('AnchorTabs', () => { }, { name: 'should focus last visible tab when End key pressed', + disabledIndex: undefined, hiddenIndex: 2, initialFocusIndex: 0, keyName: keyEnd, expectedFinalFocusIndex: 1 } - ]; + ] as const; describe('navigation', () => { - const focused: string[] = []; - const disabled: string[] = []; - for (const test of navigationTests) { - const specType = getSpecTypeByNamedList( - test, - focused, - disabled - ); - // eslint-disable-next-line @typescript-eslint/no-loop-func - specType(test.name, async () => { + parameterizeNamedList(navigationTests, (spec, name, value) => { + spec(name, async () => { await connect(); - if (test.disabledIndex !== undefined) { - tab(test.disabledIndex).disabled = true; + if (value.disabledIndex !== undefined) { + tab(value.disabledIndex).disabled = true; await waitForUpdatesAsync(); } - if (test.hiddenIndex !== undefined) { - tab(test.hiddenIndex).hidden = true; + if (value.hiddenIndex !== undefined) { + tab(value.hiddenIndex).hidden = true; await waitForUpdatesAsync(); } - tab(test.initialFocusIndex).focus(); - tab(test.initialFocusIndex).dispatchEvent( - new KeyboardEvent('keydown', { key: test.keyName }) + tab(value.initialFocusIndex).focus(); + tab(value.initialFocusIndex).dispatchEvent( + new KeyboardEvent('keydown', { key: value.keyName }) ); await waitForUpdatesAsync(); expect(document.activeElement).toBe( - tab(test.expectedFinalFocusIndex) - ); - expect(tab(test.expectedFinalFocusIndex).ariaSelected).toBe( - 'true' + tab(value.expectedFinalFocusIndex) ); + expect( + tab(value.expectedFinalFocusIndex).ariaSelected + ).toBe('true'); }); - } + }); }); it('should skip past other tabs when pressing tab key after click', async () => { diff --git a/packages/nimble-components/src/utilities/tests/parameterized.ts b/packages/nimble-components/src/utilities/tests/parameterized.ts index e8d2f8be95..c452ade592 100644 --- a/packages/nimble-components/src/utilities/tests/parameterized.ts +++ b/packages/nimble-components/src/utilities/tests/parameterized.ts @@ -1,23 +1,7 @@ // eslint-disable-next-line no-restricted-globals type SpecTypes = typeof fit | typeof xit | typeof it; /** - * Note: you should probably use one of the specialized versions of this function instead - * - * Used to focus or disable specific tests in a parameterized test run. - * In the following example the test for the `cats` case is focused for debugging - * and no tests are disabled: - * @example - * const rainTypes = ['cats', 'dogs', 'frogs', 'men']; - * describe('Different rains', () => { - * const isFocused = rainType => rainType === 'cats'; - * const isDisabled = () => false; - * for (const rainType of rainTypes) { - * const specType = getSpecType(rainType, isFocused, isDisabled); - * specType(`of type ${rainType} exist`, () => { - * expect(rainType).toBeTruthy(); - * }); - * } - * }); + * @deprecated switch to `parameterize` or `parameterizeNamedList` instead */ const getSpecType = ( value: T, @@ -35,25 +19,7 @@ const getSpecType = ( }; /** - * Used to focus or disable specific tests in a parameterized test run using a list of named objects. - * In the following example the test for the `cats-and-dogs` case is focused for debugging - * and no tests are disabled: - * @example - * const rainTypes = [ - * { name: 'cats-and-dogs', type: 'idiom' }, - * { name: 'frogs' type: 'idiom'}, - * { name: 'men', type: 'lyrics'} - * ] as const; - * describe('Different rains', () => { - * const focused = ['cats-and-dogs']; - * const disabled = []; - * for (const rainType of rainTypes) { - * const specType = getSpecTypeByNamedList(rainType, focused, disabled); - * specType(`of type ${rainType.name} exist`, () => { - * expect(rainType.type).toBeDefined(); - * }); - * } - * }); + * @deprecated switch to `parameterize` or `parameterizeNamedList` instead */ export const getSpecTypeByNamedList = ( value: T, @@ -64,3 +30,124 @@ export const getSpecTypeByNamedList = ( (x: T) => focusList.includes(x.name), (x: T) => disabledList.includes(x.name) ); +// The following aliases are just to reduce the number +// of eslint disables in this source file. In normal +// test code use the globals directly so eslint can +// guard accidental check-ins of fit, etc. +// eslint-disable-next-line no-restricted-globals +type Fit = typeof fit; +type Xit = typeof xit; +type It = typeof it; +/** + * One of the jasmine spec functions: fit, xit, or it + */ +type Spec = Fit | Xit | It; +/** + * One of the jasmine spec functions: fit or xit + */ +type SpecOverride = Fit | Xit; + +/** + * Used to create a parameterized test using an object of test names and arbitrary test values. + * In the following example: + * - the test named `catsAndDogs` is focused for debugging + * - the test named `frogs` is configured to always be disabled + * - the test named `men` will run normally as it has no override + * @example + * const rainTests = { + * catsAndDogs: 'idiom', + * frogs: 'idiom', + * men: 'lyrics' + * } as const; + * describe('Different rains', () => { + * parameterize(rainTests, (spec, name, value) => { + * spec(`of type ${name} exist`, () => { + * expect(value).toBeDefined(); + * }); + * }, { + * catsAndDogs: fit, + * frogs: xit + * }); + * }); + */ +export const parameterize = ( + testCases: T, + test: (spec: Spec, name: keyof T, value: T[keyof T]) => void, + specOverrides?: { + [P in keyof T]?: SpecOverride; + } +): void => { + const testCaseNames = Object.keys(testCases) as (keyof T)[]; + if (specOverrides) { + const overrideNames = Object.keys( + specOverrides + ) as (keyof typeof specOverrides)[]; + if ( + !overrideNames.every(overrideName => testCaseNames.includes(overrideName)) + ) { + throw new Error( + 'Parameterized test override names must match test case name' + ); + } + if ( + // eslint-disable-next-line no-restricted-globals + !overrideNames.every(overrideName => [fit, xit].includes(specOverrides[overrideName]!)) + ) { + throw new Error( + 'Must configure override with one of the jasmine spec functions: fit or xit' + ); + } + } + testCaseNames.forEach(testCaseName => { + const spec = specOverrides?.[testCaseName] ?? it; + test(spec, testCaseName, testCases[testCaseName]); + }); +}; + +type ObjectFromNamedList = { + [K in T extends readonly { name: infer U }[] ? U : never]: T[number]; +}; + +/** + * Used to create a parameterized test using an array of tests with names. + * In the following example: + * - the test named `cats-and-dogs` is focused for debugging + * - the test named `frogs` is configured to always be disabled + * - the test named `men` will run normally as it has no override + * @example + * const rainTests = [ + * { name: 'cats-and-dogs', type: 'idiom' }, + * { name: 'frogs' type: 'idiom'}, + * { name: 'men', type: 'lyrics'} + * ] as const; + * describe('Different rains', () => { + * parameterizeNamedList(rainTests, (spec, name, value) => { + * spec(`of type ${name} exist`, () => { + * expect(value.type).toBeDefined(); + * }); + * }, { + * 'cats-and-dogs': fit, + * frogs: xit + * }); + * }); + */ +export const parameterizeNamedList = ( + list: T, + test: ( + spec: Spec, + name: keyof ObjectFromNamedList, + value: T[number] + ) => void, + specOverrides?: { + [P in keyof ObjectFromNamedList]?: SpecOverride; + } +): void => { + const testCases = list.reduce<{ [key: string]: { name: string } }>( + (result, entry) => { + result[entry.name] = entry; + return result; + }, + {} + ) as ObjectFromNamedList; + parameterize>(testCases, test, specOverrides); +}; diff --git a/packages/nimble-components/src/utilities/tests/tests/parametized.spec.ts b/packages/nimble-components/src/utilities/tests/tests/parametized.spec.ts new file mode 100644 index 0000000000..de74974c7b --- /dev/null +++ b/packages/nimble-components/src/utilities/tests/tests/parametized.spec.ts @@ -0,0 +1,259 @@ +import { parameterize, parameterizeNamedList } from '../parameterized'; + +// The following aliases are just to reduce the number +// of eslint disables in this test file. In normal +// test code use the globals directly so eslint can +// guard accidental check-ins of fit, etc. +// eslint-disable-next-line no-restricted-globals +const FIT = fit; +const IT = it; +const XIT = xit; + +interface ParameterizeTestArgs { + spec: typeof IT | typeof XIT | typeof FIT; + name: string; + value: unknown; +} +const paramertizeTestArgs = ([ + spec, + name, + value +]: unknown[]): ParameterizeTestArgs => ({ + spec, + name, + value +} as ParameterizeTestArgs); + +describe('Funtion parameterize', () => { + describe('can parameterize simple objects', () => { + it('with test enabled', () => { + const testcases = { + case1: 'one' + } as const; + const spy = jasmine.createSpy(); + parameterize(testcases, spy); + + expect(spy).toHaveBeenCalledTimes(1); + const { spec, name, value } = paramertizeTestArgs( + spy.calls.argsFor(0) + ); + expect(spec).toBe(IT); + expect(name).toBe('case1'); + expect(value).toBe('one'); + }); + + it('with test focused', () => { + const testcases = { + case1: 'one' + } as const; + const spy = jasmine.createSpy(); + parameterize(testcases, spy, { + case1: FIT + }); + + expect(spy).toHaveBeenCalledTimes(1); + const { spec, name, value } = paramertizeTestArgs( + spy.calls.argsFor(0) + ); + expect(spec).toBe(FIT); + expect(name).toBe('case1'); + expect(value).toBe('one'); + }); + + it('with test disabled', () => { + const testcases = { + case1: 'one' + } as const; + const spy = jasmine.createSpy(); + parameterize(testcases, spy, { + case1: XIT + }); + + expect(spy).toHaveBeenCalledTimes(1); + const { spec, name, value } = paramertizeTestArgs( + spy.calls.argsFor(0) + ); + expect(spec).toBe(XIT); + expect(name).toBe('case1'); + expect(value).toBe('one'); + }); + + it('with various test cases enabled and disabled', () => { + const testcases = { + case1: 'one', + case2: 'two', + case3: 'three' + } as const; + const spy = jasmine.createSpy(); + parameterize(testcases, spy, { + case2: XIT, + case3: FIT + }); + + expect(spy).toHaveBeenCalledTimes(3); + { + const { spec, name, value } = paramertizeTestArgs( + spy.calls.argsFor(0) + ); + expect(spec).toBe(IT); + expect(name).toBe('case1'); + expect(value).toBe('one'); + } + { + const { spec, name, value } = paramertizeTestArgs( + spy.calls.argsFor(1) + ); + expect(spec).toBe(XIT); + expect(name).toBe('case2'); + expect(value).toBe('two'); + } + { + const { spec, name, value } = paramertizeTestArgs( + spy.calls.argsFor(2) + ); + expect(spec).toBe(FIT); + expect(name).toBe('case3'); + expect(value).toBe('three'); + } + }); + }); + describe('errors', () => { + it('for override not in test cases', () => { + const testcases = { + case1: 'one' + } as { [key: string]: string }; + + expect(() => { + parameterize(testcases, () => {}, { + unknown: XIT + }); + }).toThrowError(/override names must match test case name/); + }); + it('for override not referencing supported xit or fit', () => { + const testcases = { + case1: 'one' + } as const; + + expect(() => { + parameterize(testcases, () => {}, { + case1: IT + }); + }).toThrowError(/jasmine spec functions: fit or xit/); + }); + }); +}); + +interface ParameterizeListTestArgs { + spec: typeof IT | typeof XIT | typeof FIT; + name: string; +} +const paramertizeListTestArgs = ([ + spec, + name +]: unknown[]): ParameterizeListTestArgs => ({ + spec, + name +} as ParameterizeListTestArgs); + +describe('Funtion parameterizeNamedList', () => { + describe('can parameterize simple lists', () => { + it('with test enabled', () => { + const testcases = [{ name: 'case1' }] as const; + const spy = jasmine.createSpy(); + parameterizeNamedList(testcases, spy); + + expect(spy).toHaveBeenCalledTimes(1); + const { spec, name } = paramertizeListTestArgs( + spy.calls.argsFor(0) + ); + expect(spec).toBe(IT); + expect(name).toBe('case1'); + }); + + it('with test focused', () => { + const testcases = [{ name: 'case1' }] as const; + const spy = jasmine.createSpy(); + parameterizeNamedList(testcases, spy, { + case1: FIT + }); + + expect(spy).toHaveBeenCalledTimes(1); + const { spec, name } = paramertizeListTestArgs( + spy.calls.argsFor(0) + ); + expect(spec).toBe(FIT); + expect(name).toBe('case1'); + }); + + it('with test disabled', () => { + const testcases = [{ name: 'case1' }] as const; + const spy = jasmine.createSpy(); + parameterizeNamedList(testcases, spy, { + case1: XIT + }); + + expect(spy).toHaveBeenCalledTimes(1); + const { spec, name } = paramertizeListTestArgs( + spy.calls.argsFor(0) + ); + expect(spec).toBe(XIT); + expect(name).toBe('case1'); + }); + + it('with various test cases enabled and disabled', () => { + const testcases = [ + { name: 'case1' }, + { name: 'case2' }, + { name: 'case3' } + ] as const; + const spy = jasmine.createSpy(); + parameterizeNamedList(testcases, spy, { + case2: XIT, + case3: FIT + }); + + expect(spy).toHaveBeenCalledTimes(3); + { + const { spec, name } = paramertizeListTestArgs( + spy.calls.argsFor(0) + ); + expect(spec).toBe(IT); + expect(name).toBe('case1'); + } + { + const { spec, name } = paramertizeListTestArgs( + spy.calls.argsFor(1) + ); + expect(spec).toBe(XIT); + expect(name).toBe('case2'); + } + { + const { spec, name } = paramertizeListTestArgs( + spy.calls.argsFor(2) + ); + expect(spec).toBe(FIT); + expect(name).toBe('case3'); + } + }); + }); + describe('errors', () => { + it('for override not in test cases', () => { + const testcases = [{ name: 'case1' }] as { name: string }[]; + + expect(() => { + parameterizeNamedList(testcases, () => {}, { + unknown: XIT + }); + }).toThrowError(/override names must match test case name/); + }); + it('for override not referencing supported xit or fit', () => { + const testcases = [{ name: 'case1' }] as const; + + expect(() => { + parameterizeNamedList(testcases, () => {}, { + case1: IT + }); + }).toThrowError(/jasmine spec functions: fit or xit/); + }); + }); +});