Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

UnitControl: rewrite tests in TypeScript #40697

Merged
merged 15 commits into from May 3, 2022
Merged

UnitControl: rewrite tests in TypeScript #40697

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
8075dad
Rename file extensions
ciampo Apr 28, 2022
67fae8b
Rewrite `ControlledSyncUnits` part
ciampo Apr 28, 2022
f6c9d62
Use diff snapshot to test classnames
ciampo Apr 28, 2022
6ff8fd1
Use `toBeInTheDocument`
ciampo Apr 28, 2022
46bb12e
Type internal test state setters
ciampo Apr 28, 2022
70cdfee
Use `testing-library` semantic queries instead of `document.querySele…
ciampo Apr 28, 2022
7d1eaec
Tweak optional parameter for some utilities
ciampo Apr 28, 2022
bcbf186
Add type guard to `hasUnits`
ciampo Apr 28, 2022
8317e71
Fix TS errors in utils tests
ciampo Apr 28, 2022
3c90c08
Add types to user-event wrapper function
ciampo Apr 28, 2022
048222a
CHANGELOG
ciampo Apr 28, 2022
ecae33e
Add comment about failing test
ciampo Apr 28, 2022
54379dc
ts-expect-error instead of ts-ignore
ciampo Apr 29, 2022
8929d0b
Fix broken test by passing the missing `value`
ciampo Apr 29, 2022
a237da4
Add temporary stories to check single unit behavior
ciampo Apr 29, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/components/CHANGELOG.md
View file
Open in desktop
Expand Up @@ -2,6 +2,10 @@

## Unreleased

### Internal

- `UnitControl`: migrate unit tests to TypeScript ([#40697](https://github.com/WordPress/gutenberg/pull/40697)).

### Enhancements

- `InputControl`: Add `__next36pxDefaultSize` flag for larger default size ([#40622](https://github.com/WordPress/gutenberg/pull/40622)).
Expand Down
33 changes: 33 additions & 0 deletions packages/components/src/unit-control/test/__snapshots__/index.tsx.snap
View file
Open in desktop
@@ -0,0 +1,33 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`UnitControl Basic rendering should render custom className 1`] = `
Snapshot Diff:
- First value
+ Second value

@@ -1,10 +1,10 @@
<div
class="components-unit-control-wrapper css-aa2xc3-Root e1bagdl33"
>
<div
- class="components-flex components-input-control components-number-control components-unit-control e1bagdl32 ep48uk90 em5sgkm7 css-18wzek1-View-Flex-sx-Base-sx-Items-ItemsColumn-Root-rootLabelPositionStyles-Input-ValueInput-arrowStyles-paddingStyles em57xhy0"
+ class="components-flex components-input-control components-number-control components-unit-control hello e1bagdl32 ep48uk90 em5sgkm7 css-18wzek1-View-Flex-sx-Base-sx-Items-ItemsColumn-Root-rootLabelPositionStyles-Input-ValueInput-arrowStyles-paddingStyles em57xhy0"
data-wp-c16t="true"
data-wp-component="Flex"
>
<div
class="components-flex-item em5sgkm3 css-1fmchc6-View-Item-sx-Base-LabelWrapper em57xhy0"
@@ -15,11 +15,11 @@
class="components-input-control__container css-1sy20aj-Container-containerDisabledStyles-containerMarginStyles-containerWidthStyles em5sgkm6"
>
<input
autocomplete="off"
class="components-input-control__input css-gzm6eu-Input-dragStyles-fontSizeStyles-sizeStyles em5sgkm5"
- id="inspector-input-control-1"
+ id="inspector-input-control-2"
inputmode="numeric"
max="Infinity"
min="-Infinity"
step="1"
type="number"
`;
159 changes: 99 additions & 60 deletions ...components/src/unit-control/test/index.js → ...omponents/src/unit-control/test/index.tsx
View file
Open in desktop
Expand Up @@ -12,10 +12,11 @@ import { useState } from '@wordpress/element';
/**
* Internal dependencies
*/
import UnitControl from '../';
import UnitControl from '..';
import { parseQuantityAndUnitFromRawValue } from '../utils';
import type { UnitControlOnChangeCallback } from '../types';

function render( jsx ) {
function render( jsx: React.ReactElement ) {
return {
user: userEvent.setup( {
// Avoids timeout errors (https://github.com/testing-library/user-event/issues/565#issuecomment-1064579531).
Expand All @@ -25,30 +26,44 @@ function render( jsx ) {
};
}

const getComponent = () =>
document.body.querySelector( '.components-unit-control' );
const getInput = () =>
document.body.querySelector( '.components-unit-control input' );
const getSelect = () =>
document.body.querySelector( '.components-unit-control select' );
const getUnitLabel = () =>
document.body.querySelector( '.components-unit-control__unit-label' );
const getInput = ( {
isInputTypeText = false,
}: {
isInputTypeText?: boolean;
} = {} ) =>
screen.getByRole(
isInputTypeText ? 'textbox' : 'spinbutton'
) as HTMLInputElement;
const getSelect = () => screen.getByRole( 'combobox' ) as HTMLSelectElement;
const getSelectOptions = () =>
screen.getAllByRole( 'option' ) as HTMLOptionElement[];

const ControlledSyncUnits = () => {
const [ state, setState ] = useState( { valueA: '', valueB: '' } );
const [ state, setState ] = useState( {
valueA: '',
valueB: '',
} );

// Keep the unit sync'd between the two `UnitControl` instances.
const onUnitControlChange = ( fieldName, newValue ) => {
// eslint-disable-next-line @wordpress/no-unused-vars-before-return
const [ quantity, newUnit ] = parseQuantityAndUnitFromRawValue(
const onUnitControlChange = (
fieldName: 'valueA' | 'valueB',
newValue?: string | number
) => {
const parsedQuantityAndUnit = parseQuantityAndUnitFromRawValue(
newValue
);
const quantity = parsedQuantityAndUnit[ 0 ];

if ( ! Number.isFinite( quantity ) ) {
return;
}

const nextState = { ...state, [ fieldName ]: newValue };
const newUnit = parsedQuantityAndUnit[ 1 ];

const nextState = {
...state,
[ fieldName ]: newValue,
};

Object.entries( state ).forEach( ( [ stateProp, stateValue ] ) => {
const [
Expand All @@ -57,7 +72,9 @@ const ControlledSyncUnits = () => {
] = parseQuantityAndUnitFromRawValue( stateValue );

if ( stateProp !== fieldName && stateUnit !== newUnit ) {
nextState[ stateProp ] = `${ stateQuantity }${ newUnit }`;
nextState[
stateProp as 'valueA' | 'valueB'
] = `${ stateQuantity }${ newUnit }`;
}
} );

Expand Down Expand Up @@ -87,35 +104,42 @@ describe( 'UnitControl', () => {
const input = getInput();
const select = getSelect();

expect( input ).toBeTruthy();
expect( select ).toBeTruthy();
expect( input ).toBeInTheDocument();
expect( select ).toBeInTheDocument();
} );

it( 'should render custom className', () => {
render( <UnitControl className="hello" /> );
const { container: withoutClassName } = render( <UnitControl /> );

const el = getComponent();
const { container: withClassName } = render(
<UnitControl className="hello" />
);

expect( el.classList.contains( 'hello' ) ).toBe( true );
expect( withoutClassName.firstChild ).toMatchDiffSnapshot(
withClassName.firstChild
);
} );

it( 'should not render select, if units are disabled', () => {
render( <UnitControl value="3em" units={ [] } /> );
const input = getInput();
const select = getSelect();
// Using `queryByRole` instead of `getSelect` because we need to test
// for this element NOT to be in the document.
const select = screen.queryByRole( 'combobox' );

expect( input ).toBeTruthy();
expect( select ).toBeFalsy();
expect( input ).toBeInTheDocument();
expect( select ).not.toBeInTheDocument();
} );

it( 'should render label if single units', () => {
render( <UnitControl units={ [ { value: '%', label: '%' } ] } /> );

const select = getSelect();
const label = getUnitLabel();
const select = screen.queryByRole( 'combobox' );
// The unit is not being displayed!
const label = screen.getByText( '%' );
Copy link
Contributor Author

@ciampo ciampo Apr 28, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently this line is failing in unit tests — that's because we switched from just checking that an element with class components-unit-control__unit-label exists, to actually looking for the unit label being displayed (testing-library FTW!)

I'm not sure if we should call this a bug, though. Technically we're not passing any value to UnitControl, and therefore the component internally has not value to parse a unit from. If we passed value="30%", then this test would pass.

If our expectations are that UnitControl would display the % unit, for how the component is coded right now, it would mean that effectively the selected unit is % (which would also be reflected in the new components's value and probably fired through the change callbacks). Would that be a correct behavior to expect? I'm personally not so sure.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From how I'd interpret it, the purpose of this test is exactly what it says: "should render label if single units". The fact that it's rendered with no value defined is just an oversight, and I think the original PR confirms that (#33634).

I think we should just render it with value="30%" like you suggest.

Copy link
Contributor Author

@ciampo ciampo Apr 29, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should just render it with value="30%" like you suggest.

I went ahead and did this in 8929d0b (which should fix the tests for now).

I still decided to look into this a bit more, and therefore I decided to add a couple of Storybook examples for this scenario:

  • One closely representing the failing unit test (e.g. uncontrolled UnitControl with just one unit)
  • Another one where UnitControl is controlled, but its starting value is undefined (still with just one unit)

In the controlled case, the unit is not visible when value is set to undefined (its starting value). The unit then becomes visible when a number is entered, and stays visible even if those numbers are deleted (likely because the value becomes an empty string rather than undefined):

unit-control-single-unit-controlled.mp4

In the uncontrolled case, the unit is never shown even after the user enters a value:

unit-control-single-unit-uncontrolled.mp4

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh, good investigation! I would expect that when there is only one unit, the unit would be visible at mount and stay visible no matter what. This kind of seems like a common use case so I'd say it's worth fixing if it isn't too hard. What do you think?


expect( select ).toBeFalsy();
expect( label ).toBeTruthy();
expect( select ).not.toBeInTheDocument();
expect( label ).toBeInTheDocument();
} );
} );

Expand All @@ -141,8 +165,9 @@ describe( 'UnitControl', () => {
} );

it( 'should increment value on UP press', async () => {
let state = '50px';
const setState = ( nextState ) => ( state = nextState );
let state: string | undefined = '50px';
const setState: UnitControlOnChangeCallback = ( nextState ) =>
( state = nextState );

const { user } = render(
<UnitControl value={ state } onChange={ setState } />
Expand All @@ -155,8 +180,9 @@ describe( 'UnitControl', () => {
} );

it( 'should increment value on UP + SHIFT press, with step', async () => {
let state = '50px';
const setState = ( nextState ) => ( state = nextState );
let state: string | undefined = '50px';
const setState: UnitControlOnChangeCallback = ( nextState ) =>
( state = nextState );

const { user } = render(
<UnitControl value={ state } onChange={ setState } />
Expand All @@ -169,8 +195,9 @@ describe( 'UnitControl', () => {
} );

it( 'should decrement value on DOWN press', async () => {
let state = 50;
const setState = ( nextState ) => ( state = nextState );
let state: string | number | undefined = 50;
const setState: UnitControlOnChangeCallback = ( nextState ) =>
( state = nextState );

const { user } = render(
<UnitControl value={ state } onChange={ setState } />
Expand All @@ -183,8 +210,9 @@ describe( 'UnitControl', () => {
} );

it( 'should decrement value on DOWN + SHIFT press, with step', async () => {
let state = 50;
const setState = ( nextState ) => ( state = nextState );
let state: string | number | undefined = 50;
const setState: UnitControlOnChangeCallback = ( nextState ) =>
( state = nextState );

const { user } = render(
<UnitControl value={ state } onChange={ setState } />
Expand All @@ -197,8 +225,9 @@ describe( 'UnitControl', () => {
} );

it( 'should cancel change when ESCAPE key is pressed', async () => {
let state = 50;
const setState = ( nextState ) => ( state = nextState );
let state: string | number | undefined = 50;
const setState: UnitControlOnChangeCallback = ( nextState ) =>
( state = nextState );

const { user } = render(
<UnitControl
Expand All @@ -208,7 +237,8 @@ describe( 'UnitControl', () => {
/>
);

const input = getInput();
// Input type is `text` when the `isPressEnterToChange` prop is passed
const input = getInput( { isInputTypeText: true } );
await user.clear( input );
await user.type( input, '300px' );

Expand All @@ -222,10 +252,11 @@ describe( 'UnitControl', () => {
} );

it( 'should run onBlur callback when quantity input is blurred', async () => {
let state = '33%';
const onChangeSpy = jest.fn();
const onBlurSpy = jest.fn();
const setState = ( nextState ) => {

let state: string | undefined = '33%';
const setState: UnitControlOnChangeCallback = ( nextState ) => {
onChangeSpy( nextState );
state = nextState;
};
Expand Down Expand Up @@ -260,12 +291,11 @@ describe( 'UnitControl', () => {
} );

it( 'should invoke onChange and onUnitChange callbacks when isPressEnterToChange is true and the component is blurred with an uncommitted value', async () => {
let state = '15px';

const onUnitChangeSpy = jest.fn();
const onChangeSpy = jest.fn();

const setState = ( nextState ) => {
let state: string | undefined = '15px';
const setState: UnitControlOnChangeCallback = ( nextState ) => {
onChangeSpy( nextState );
state = nextState;
};
Expand All @@ -282,7 +312,8 @@ describe( 'UnitControl', () => {
</>
);

const input = getInput();
// Input type is `text` when the `isPressEnterToChange` prop is passed
const input = getInput( { isInputTypeText: true } );
await user.clear( input );
await user.type( input, '41vh' );

Expand All @@ -309,8 +340,9 @@ describe( 'UnitControl', () => {

describe( 'Unit', () => {
it( 'should update unit value on change', async () => {
let state = '14rem';
const setState = ( nextState ) => ( state = nextState );
let state: string | undefined = '14rem';
const setState: UnitControlOnChangeCallback = ( nextState ) =>
( state = nextState );

const spy = jest.fn();

Expand All @@ -337,8 +369,7 @@ describe( 'UnitControl', () => {

render( <UnitControl units={ units } /> );

const select = getSelect();
const options = select.querySelectorAll( 'option' );
const options = getSelectOptions();

expect( options.length ).toBe( 2 );

Expand All @@ -349,8 +380,9 @@ describe( 'UnitControl', () => {
} );

it( 'should reset value on unit change, if unit has default value', async () => {
let state = 50;
const setState = ( nextState ) => ( state = nextState );
let state: string | number | undefined = 50;
const setState: UnitControlOnChangeCallback = ( nextState ) =>
( state = nextState );

const units = [
{ value: 'pt', label: 'pt', default: 25 },
Expand All @@ -377,8 +409,9 @@ describe( 'UnitControl', () => {
} );

it( 'should not reset value on unit change, if disabled', async () => {
let state = 50;
const setState = ( nextState ) => ( state = nextState );
let state: string | number | undefined = 50;
const setState: UnitControlOnChangeCallback = ( nextState ) =>
( state = nextState );

const units = [
{ value: 'pt', label: 'pt', default: 25 },
Expand All @@ -405,8 +438,9 @@ describe( 'UnitControl', () => {
} );

it( 'should set correct unit if single units', async () => {
let state = '50%';
const setState = ( value ) => ( state = value );
let state: string | undefined = '50%';
const setState: UnitControlOnChangeCallback = ( value ) =>
( state = value );

const { user } = render(
<UnitControl
Expand Down Expand Up @@ -519,7 +553,8 @@ describe( 'UnitControl', () => {
/>
);

const input = getInput();
// Input type is `text` when the `isPressEnterToChange` prop is passed
const input = getInput( { isInputTypeText: true } );
await user.clear( input );
await user.type( input, '55 em' );
user.keyboard( '{Enter}' );
Expand All @@ -536,7 +571,8 @@ describe( 'UnitControl', () => {
/>
);

const input = getInput();
// Input type is `text` when the `isPressEnterToChange` prop is passed
const input = getInput( { isInputTypeText: true } );
await user.clear( input );
await user.type( input, '61 PX' );
user.keyboard( '{Enter}' );
Expand All @@ -553,7 +589,8 @@ describe( 'UnitControl', () => {
/>
);

const input = getInput();
// Input type is `text` when the `isPressEnterToChange` prop is passed
const input = getInput( { isInputTypeText: true } );
await user.clear( input );
await user.type( input, '55 em' );
user.keyboard( '{Enter}' );
Expand All @@ -570,7 +607,8 @@ describe( 'UnitControl', () => {
/>
);

const input = getInput();
// Input type is `text` when the `isPressEnterToChange` prop is passed
const input = getInput( { isInputTypeText: true } );
await user.clear( input );
await user.type( input, '-10 %' );
user.keyboard( '{Enter}' );
Expand All @@ -587,7 +625,8 @@ describe( 'UnitControl', () => {
/>
);

const input = getInput();
// Input type is `text` when the `isPressEnterToChange` prop is passed
const input = getInput( { isInputTypeText: true } );
await user.clear( input );
await user.type( input, '123 rEm ' );
user.keyboard( '{Enter}' );
Expand Down