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

Convert FocalPointPicker tests to TypeScript #61373

Merged
merged 6 commits into from
May 10, 2024
Merged

Convert FocalPointPicker tests to TypeScript #61373

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
dfdc258
chore: migrate focal-point-picker media tests to typescript
jpstevens May 3, 2024
42ad69f
chore: migrate focal-point-picker index tests to typescript
jpstevens May 3, 2024
5eb0ba8
fix: remove duplicate internal dependencies comment
jpstevens May 3, 2024
bd57b12
chore: use `unknown[]` instead of `any[]` for test types
jpstevens May 9, 2024
c2315a8
chore: extract data-testid to common prop object
jpstevens May 9, 2024
1f0812d
Merge branch 'WordPress:trunk' into update/type-focal-point-picker-tests
jpstevens May 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 44 additions & 17 deletions ...ents/src/focal-point-picker/test/index.js → ...nts/src/focal-point-picker/test/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,19 @@ import userEvent from '@testing-library/user-event';
* Internal dependencies
*/
import Picker from '..';
import type { FocalPointPickerProps } from '../types';

type Log = { name: string; args: any[] };
type EventLogger = ( name: string, args: any[] ) => void;

const props: FocalPointPickerProps = {
onChange: jest.fn(),
url: 'test-url',
value: {
x: 0,
y: 0,
},
};

describe( 'FocalPointPicker', () => {
describe( 'focus and blur', () => {
Expand All @@ -16,7 +29,7 @@ describe( 'FocalPointPicker', () => {

const mockOnChange = jest.fn();

render( <Picker onChange={ mockOnChange } /> );
render( <Picker { ...props } onChange={ mockOnChange } /> );

const draggableArea = screen.getByRole( 'button' );

Expand All @@ -32,6 +45,7 @@ describe( 'FocalPointPicker', () => {

render(
<Picker
{ ...props }
onChange={ mockOnChange }
onDrag={ mockOnDrag }
onDragEnd={ mockOnDragEnd }
Expand All @@ -57,15 +71,16 @@ describe( 'FocalPointPicker', () => {

describe( 'drag gestures', () => {
it( 'should call onDragStart, onDrag, onDragEnd and onChange in that order', () => {
const logs = [];
const eventLogger = ( name, args ) => logs.push( { name, args } );
const logs: Log[] = [];
const eventLogger: EventLogger = ( name, args ) =>
logs.push( { name, args } );
const events = [ 'onDragStart', 'onDrag', 'onDragEnd', 'onChange' ];
const handlers = {};
const handlers: { [ key: string ]: EventLogger } = {};
events.forEach( ( name ) => {
handlers[ name ] = ( ...all ) => eventLogger( name, all );
} );

render( <Picker { ...handlers } /> );
render( <Picker { ...props } { ...handlers } /> );

const dragArea = screen.getByRole( 'button' );

Expand All @@ -92,6 +107,7 @@ describe( 'FocalPointPicker', () => {

render(
<Picker
{ ...props }
value={ { x: 0.25, y: 0.25 } }
onChange={ spyChange }
resolvePoint={ () => {
Expand All @@ -118,20 +134,24 @@ describe( 'FocalPointPicker', () => {
describe( 'controllability', () => {
it( 'should update value from props', () => {
const { rerender } = render(
<Picker value={ { x: 0.25, y: 0.5 } } />
<Picker { ...props } value={ { x: 0.25, y: 0.5 } } />
);
const xInput = screen.getByRole( 'spinbutton', {
name: 'Focal point left position',
} );
rerender( <Picker value={ { x: 0.93, y: 0.5 } } /> );
} ) as HTMLButtonElement;
rerender( <Picker { ...props } value={ { x: 0.93, y: 0.5 } } /> );
expect( xInput.value ).toBe( '93' );
} );
it( 'call onChange with the expected values', async () => {
const user = userEvent.setup();

const spyChange = jest.fn();
render(
<Picker value={ { x: 0.14, y: 0.62 } } onChange={ spyChange } />
<Picker
{ ...props }
value={ { x: 0.14, y: 0.62 } }
onChange={ spyChange }
/>
);
// Focus and press arrow up
const dragArea = screen.getByRole( 'button' );
Expand All @@ -151,20 +171,27 @@ describe( 'FocalPointPicker', () => {
const onChangeSpy = jest.fn();
render(
<Picker
value={ { x: '0.1', y: '0.2' } }
{ ...props }
value={ {
x: '0.1' as unknown as number,
y: '0.2' as unknown as number,
} }
onChange={ onChangeSpy }
/>
);

expect(
screen.getByRole( 'spinbutton', {
name: 'Focal point left position',
} ).value
(
screen.getByRole( 'spinbutton', {
name: 'Focal point left position',
} ) as HTMLButtonElement
).value
).toBe( '10' );
expect(
screen.getByRole( 'spinbutton', {
name: 'Focal point top position',
} ).value
(
screen.getByRole( 'spinbutton', {
name: 'Focal point top position',
} ) as HTMLButtonElement
).value
).toBe( '20' );
expect( onChangeSpy ).not.toHaveBeenCalled();
} );
Expand Down
40 changes: 33 additions & 7 deletions ...ents/src/focal-point-picker/test/media.js → ...nts/src/focal-point-picker/test/media.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,19 +7,25 @@ import { render, screen } from '@testing-library/react';
* Internal dependencies
*/
import Media from '../media';
import type { FocalPointPickerMediaProps } from '../types';

const props: FocalPointPickerMediaProps = {
alt: '',
src: '',
Copy link
Member

Choose a reason for hiding this comment

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

Literally every test will have the data-testid="media" prop, should we add this to props and remove that prop from the tests?

We might need to add the data-testid prop to FocalPointPickerMediaProps since that prop doesn't exist there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question - I've updated the default prop object to include 'data-testid': 'media'

The change is only required for testing, so I opted not to change the underlying FocalPointPickerMediaProps type definition.

The current implementation of Media uses pass-through props, which allow any prop to be passed to the child component, including data-testid

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interestingly, I couldn't find any tests in the codebase where the data-testid prop is not repeated on each component:

  • Elevation repeats data-testid="elevation"
  • Grid repeats data-testid="grid"
  • Panel repeats data-testid="inner-content"

There is one example that extracts the data-testid value to a const, but it doesn't prevent the prop repetition on the components:

  • Icon repeats data-testid={ testId }

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for taking such a thorough look! This was a very minor detail and TBH I'm good with you've come up with 👍

};

describe( 'FocalPointPicker/Media', () => {
describe( 'Basic rendering', () => {
it( 'should render', () => {
render( <Media data-testid="media" /> );
render( <Media { ...props } data-testid="media" /> );

expect( screen.getByTestId( 'media' ) ).toBeVisible();
} );
} );

describe( 'Media types', () => {
it( 'should render a placeholder by default', () => {
render( <Media data-testid="media" /> );
render( <Media { ...props } data-testid="media" /> );

expect( screen.getByTestId( 'media' ) ).toHaveClass(
'components-focal-point-picker__media--placeholder'
Expand All @@ -28,33 +34,53 @@ describe( 'FocalPointPicker/Media', () => {

it( 'should render an video if src is a video file', () => {
const { rerender } = render(
<Media src="file.mp4" muted={ false } data-testid="media" />
<Media
{ ...props }
src="file.mp4"
muted={ false }
data-testid="media"
/>
);

expect( screen.getByTestId( 'media' ).tagName ).toBe( 'VIDEO' );

rerender(
<Media src="file.ogg" muted={ false } data-testid="media" />
<Media
{ ...props }
src="file.ogg"
muted={ false }
data-testid="media"
/>
);

expect( screen.getByTestId( 'media' ).tagName ).toBe( 'VIDEO' );

rerender(
<Media src="file.webm" muted={ false } data-testid="media" />
<Media
{ ...props }
src="file.webm"
muted={ false }
data-testid="media"
/>
);

expect( screen.getByTestId( 'media' ).tagName ).toBe( 'VIDEO' );
} );

it( 'should render an image file, if not video', () => {
const { rerender } = render(
<Media src="file.gif" data-testid="media" />
<Media { ...props } src="file.gif" data-testid="media" />
);

expect( screen.getByTestId( 'media' ).tagName ).toBe( 'IMG' );

rerender(
<Media src="file.png" muted={ false } data-testid="media" />
<Media
{ ...props }
src="file.png"
muted={ false }
data-testid="media"
/>
);

expect( screen.getByTestId( 'media' ).tagName ).toBe( 'IMG' );
Expand Down
Loading