FontSizePicker: Rewrite unit tests to use userEvent and be more comprehensive #45298

noisysocks · 2022-10-26T05:39:38Z

What?

Rewrites the FontSizePicker unit tests to use userEvent where possible and be more comprehensive.

Why?

I want to make @ciampo happy 🙂 see #44891 (review).

How?

I looked at the FontSizePicker component storybook, listed all of the functionality that I could think of, and then wrote new tests from scratch that covered everything that I listed.

Testing Instructions

npm run test, obviously, but the more important thing is to review the code and check that I haven't missed anything.

Screenshots or screencast

…hensive

ramonjd

Thanks for improving the unit tests. Good coverage 👍

I left some questions about testing the header hint and labels.

ramonjd · 2022-10-28T00:31:32Z

packages/components/src/font-size-picker/test/index.tsx

-			it( 'should fallback to font size `slug` for header hint label if `name` is undefined', () => {
-				const fontSizes = [
-					{
-						slug: 'gigantosaurus',


Do you have something against gigantosauruses? 🦖

You're right, I'll put her back.

ramonjd · 2022-10-28T01:27:51Z

packages/components/src/font-size-picker/test/index.tsx

+						value={ value }
+					/>
+				);
+				// TODO: There's no accessible way to select the label.


There's been a label since #45180 (slightly modified in #45298) as far as I'm aware.

How did I miss that! I'll query for the existence of an element with that label 👍

I was also going to point out that in case you need to do exact test matching across multiple DOM elements, you can pass a custom matcher to screen.getByText:

function textContentMatcher( textMatch: string | RegExp ) { const hasText = typeof textMatch === 'string' ? ( node: Element | null ) => node?.textContent === textMatch : ( node: Element | null ) => textMatch.test( node?.textContent ?? '' ); const matcherFn: MatcherFunction = ( _content, node ) => { if ( ! hasText( node ) ) { return false; } const childrenDontHaveText = Array.from( node?.children || [] ).every( ( child ) => ! hasText( child ) ); return childrenDontHaveText; }; return matcherFn; } // later, in a test: const label = screen.getByText( textContentMatcher( expectedHint ) );

ramonjd · 2022-10-28T01:31:02Z

packages/components/src/font-size-picker/test/index.tsx

+				const label = document.querySelector(
+					'.components-base-control__label'
+				);
+				expect( label ).toHaveTextContent( expectedHint );


The expectedHint in this test case doesn't look right to me.

Could it be that the test is passing because toHaveTextContent does partial matches?

If we're only interested in testing whether the accessible label and text hint are set correctly, then another option might be to just pass a single fontSize for the label you want to test:

test.each( [ { value: undefined, expectedHint: 'Size Default', fontSizePresets: [ fontSizes[ 0 ] ], }, { value: '8px', expectedHint: 'Size Tiny(px)', fontSizePresets: [ fontSizes[ 0 ] ], }, { value: '3px', expectedHint: 'Size (Custom)', fontSizePresets: [ fontSizes[ 0 ] ], }, ] )( 'displays $expectedHint as header hint when value is $value', ( { value, expectedHint, fontSizePresets } ) => { render( <FontSizePicker __nextHasNoMarginBottom fontSizes={ fontSizePresets } value={ value } /> ); const label = screen.getByLabelText( expectedHint ); expect( label ).toBeInTheDocument(); expect( label ).toHaveTextContent( // Removes whitespaces to match text interspersed in HTML elements. expectedHint.replace( /\s*/g, '' ) ); } );

This avoids have to fire an event to click on preset buttons and what not.

By fixing your other comment I think I inadvertently fixed this, good catch 😀

noisysocks · 2022-10-28T05:31:05Z

Thanks @ramonjd! Updated this per your feedback and added a changelog entry.

ciampo · 2022-10-28T07:35:41Z

About to post my review as, sorry for the delay!

ciampo

Thank you @noisysocks for working on this!!

Apart from the inline comments, it looks like we're not adding tests for:

"complex" CSS values (e.g clamp(1.75rem, 3vw, 2.25rem))
font sizes with different units (i.e some in px, some in rem, ...)

Those are probably some of the harder to test edge cases, and therefore it would be nice to have unit tests for those too!

I want to make @ciampo happy

I'm flattered 😊

packages/components/src/font-size-picker/test/index.tsx

ciampo · 2022-10-28T07:37:47Z

packages/components/src/font-size-picker/test/index.tsx

+						value={ value }
+					/>
+				);
+				// TODO: There's no accessible way to select the label.


I was also going to point out that in case you need to do exact test matching across multiple DOM elements, you can pass a custom matcher to screen.getByText:

function textContentMatcher( textMatch: string | RegExp ) { const hasText = typeof textMatch === 'string' ? ( node: Element | null ) => node?.textContent === textMatch : ( node: Element | null ) => textMatch.test( node?.textContent ?? '' ); const matcherFn: MatcherFunction = ( _content, node ) => { if ( ! hasText( node ) ) { return false; } const childrenDontHaveText = Array.from( node?.children || [] ).every( ( child ) => ! hasText( child ) ); return childrenDontHaveText; }; return matcherFn; } // later, in a test: const label = screen.getByText( textContentMatcher( expectedHint ) );

ciampo · 2022-10-28T07:45:02Z

packages/components/src/font-size-picker/test/index.tsx

+			);
+			expect( screen.getByLabelText( 'Custom' ) ).toBeInTheDocument();
+			// TODO: onChange() shouldn't be called.
+			//expect( onChange ).not.toHaveBeenCalled();


Good find! Adding a comment here so that we don't miss this as a follow-up

It's one of the things we fixed in #44891 :)

packages/components/src/font-size-picker/test/index.tsx

noisysocks · 2022-10-31T06:46:45Z

Addressed some of the feedback. I'll add more tests for non-px values tomorrow.

noisysocks · 2022-11-01T05:16:12Z

Okey dokey, I've added tests for non-px and clamp() values and added assertions for the number of times onChange is called. Should be good to go 👍

noisysocks · 2022-11-01T06:20:39Z

Merging this so I can unblock #44891 and #44966. Thanks for the feedback @ciampo!

ciampo · 2022-11-01T08:55:13Z

Thank YOU for always taking the time to make significant improvements to the components you work on!

Rewrite FontSizePicker unit tests to use userEvent and be more compre…

299d680

…hensive

noisysocks added [Type] Code Quality Issues or PRs that relate to code quality [Package] Components /packages/components labels Oct 26, 2022

noisysocks requested a review from ciampo October 26, 2022 05:39

noisysocks requested a review from ajitbohra as a code owner October 26, 2022 05:39

ciampo requested review from mirka and chad1008 October 26, 2022 08:34

ciampo assigned noisysocks Oct 26, 2022

ramonjd approved these changes Oct 28, 2022

View reviewed changes

noisysocks added 4 commits October 28, 2022 15:50

Use header label's aria-label

b0314d2

Put back the Gigantosaurus 🦖

c2b4da6

Update changelog

d6c4d65

Merge branch 'trunk' into update/font-size-picker-unit-tests

9b9fc00

ciampo reviewed Oct 28, 2022

View reviewed changes

noisysocks added 3 commits October 31, 2022 17:46

Remove editorialising comment

a939507

Use getByLabelText( 'Custom Size' ) to select slider

22c9ab3

Remove redundant test

8a0fc89

ciampo mentioned this pull request Oct 31, 2022

Global styles: convert preset font size values to CSS vars #44967

Merged

noisysocks added 4 commits November 1, 2022 16:08

Add tests for non-px font sizes

1673ba3

Assert length of options/radios

ab89dde

Assert number of calls to onChange

4032f0f

Merge branch 'trunk' into update/font-size-picker-unit-tests

091aeea

noisysocks mentioned this pull request Nov 1, 2022

FontSizePicker: Use components instead of helper functions #44891

Merged

noisysocks merged commit fe31535 into trunk Nov 1, 2022

noisysocks deleted the update/font-size-picker-unit-tests branch November 1, 2022 06:20

github-actions bot added this to the Gutenberg 14.5 milestone Nov 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FontSizePicker: Rewrite unit tests to use userEvent and be more comprehensive #45298

FontSizePicker: Rewrite unit tests to use userEvent and be more comprehensive #45298

noisysocks commented Oct 26, 2022

ramonjd left a comment

ramonjd Oct 28, 2022

noisysocks Oct 28, 2022

ramonjd Oct 28, 2022

noisysocks Oct 28, 2022

ciampo Oct 28, 2022

ramonjd Oct 28, 2022 •

edited

Loading

noisysocks Oct 28, 2022

noisysocks commented Oct 28, 2022

ciampo commented Oct 28, 2022

ciampo left a comment

ciampo Oct 28, 2022

ciampo Oct 28, 2022

noisysocks Oct 31, 2022

noisysocks commented Oct 31, 2022

noisysocks commented Nov 1, 2022

noisysocks commented Nov 1, 2022

ciampo commented Nov 1, 2022

FontSizePicker: Rewrite unit tests to use userEvent and be more comprehensive #45298

FontSizePicker: Rewrite unit tests to use userEvent and be more comprehensive #45298

Conversation

noisysocks commented Oct 26, 2022

What?

Why?

How?

Testing Instructions

Screenshots or screencast

ramonjd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramonjd Oct 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noisysocks commented Oct 28, 2022

ciampo commented Oct 28, 2022

ciampo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noisysocks commented Oct 31, 2022

noisysocks commented Nov 1, 2022

noisysocks commented Nov 1, 2022

ciampo commented Nov 1, 2022

ramonjd Oct 28, 2022 •

edited

Loading