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

test: Fix Cover block color picker test #52943

Merged
merged 2 commits into from
Jul 27, 2023
Merged

test: Fix Cover block color picker test #52943

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7eaba51
test: Mock required styles for Cover block tests
dcalhoun Jul 25, 2023
be989b6
test: Fix Cover block HEX test assertions
dcalhoun Jul 25, 2023
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
36 changes: 17 additions & 19 deletions packages/block-library/src/cover/test/edit.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/**
* External dependencies
*/
import { Image } from 'react-native';
import { Image, Pressable } from 'react-native';
import {
getEditorHtml,
initializeEditor,
Expand All @@ -12,6 +12,7 @@ import {
getBlock,
openBlockSettings,
} from 'test/helpers';
import HsvColorPicker from 'react-native-hsv-color-picker';

/**
* WordPress dependencies
Expand Down Expand Up @@ -65,7 +66,6 @@ const COVER_BLOCK_CUSTOM_HEIGHT_HTML = `<!-- wp:cover {"url":"https://cldup.com/
const COLOR_PINK = '#f78da7';
const COLOR_RED = '#cf2e2e';
const COLOR_GRAY = '#abb8c3';
const COLOR_WHITE = '#ffffff';
const GRADIENT_GREEN =
'linear-gradient(135deg,rgb(122,220,180) 0%,rgb(0,208,130) 100%)';

Expand Down Expand Up @@ -533,38 +533,36 @@ describe( 'color settings', () => {
} );

it( 'displays the hex color value in the custom color picker', async () => {
HsvColorPicker.mockImplementation( ( props ) => {
return <Pressable { ...props } testID="hsv-color-picker" />;
} );
const screen = await initializeEditor( {
initialHtml: COVER_BLOCK_PLACEHOLDER_HTML,
} );

const block = await screen.findByLabelText( 'Cover block. Empty' );
Copy link
Member Author

Choose a reason for hiding this comment

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

This query and a few others appeared (arguably) unnecessary as later queries implicitly assert the presence of this element as children. In general, I try to use as few queries/assertions as necessary to keep test logic simple, but I do understand there is sometimes value in numerous assertions to communicate subtleties.

Please feel free to reinstate this query if you find it valuable, though.

expect( block ).toBeDefined();

// Select a color from the placeholder palette.
const colorPalette = await screen.findByTestId( 'color-palette' );
const colorButton = within( colorPalette ).getByTestId(
'custom-color-picker'
const colorButton = screen.getByA11yHint(
Copy link
Member Author

Choose a reason for hiding this comment

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

When possible, queries relying upon synchronous, user-facing text are generally preferred over asynchronous or test ID queries as they are more stable and simpler to understand. I applied this principle here and a few other places. I also removed the now unused test IDs.

Copy link
Member

Choose a reason for hiding this comment

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

When possible, queries relying upon synchronous, user-facing text are generally preferred over asynchronous or test ID queries as they are more stable and simpler to understand.

This is a good tip, thank you.

'Navigates to custom color picker'
);

expect( colorButton ).toBeDefined();
fireEvent.press( colorButton );

// Wait for Block Settings to be visible.
const blockSettingsModal = screen.getByTestId( 'block-settings-modal' );
await waitForModalVisible( blockSettingsModal );

// Assertion to check the text content of bottomLabelText before tapping color picker
const bottomLabelText = screen.getByTestId(
'color-picker-bottom-label-text'
);
expect( bottomLabelText ).toHaveTextContent( 'Select a color' );
// Assert label text before tapping color picker
expect( screen.getByText( 'Select a color' ) ).toBeVisible();

// Tap color picker
const colorPicker = screen.getByTestId( 'color-picker' );
fireEvent.press( colorPicker );
const colorPicker = screen.getByTestId( 'hsv-color-picker' );
fireEvent( colorPicker, 'onHuePickerPress', {
hue: 120,
saturation: 12,
value: 50,
} );

// Assertion to check the hex value in bottomLabelText
expect( bottomLabelText ).toHaveTextContent( COLOR_WHITE );
// Assert label hex value after tapping color picker
expect( screen.getByText( '#00FF00' ) ).toBeVisible();
Copy link
Member Author

Choose a reason for hiding this comment

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

When selecting white as the "custom" color, the following logic results in absence of a custom color as white is an available preset color. Therefore, I modified this to be a "random" color.

gutenberg/packages/block-library/src/cover/edit.native.js

Line 250 in be989b6

customOverlayColor: ( ! colorValue?.slug && color ) ?? undefined,

I am unsure if the above logic needs to be modified to support white and other preset colors...

Copy link
Member

Choose a reason for hiding this comment

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

I was just using white as a placeholder color here. I had yet to think through how we might actually select a color on the color picker within the test. I think using hsv-color-picker and onHuePickerPress is a good solution. 👍

} );
} );

Expand Down
1 change: 0 additions & 1 deletion packages/components/src/color-palette/index.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,7 +319,6 @@ function ColorPalette( {
selected: isSelectedCustom(),
} }
accessibilityHint={ accessibilityHint }
testID={ 'custom-color-picker' }
>
<View style={ customIndicatorWrapperStyle }>
<ColorIndicator
Expand Down
6 changes: 1 addition & 5 deletions packages/components/src/color-picker/index.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,6 @@ function ColorPicker( {
satValPickerSliderSize={ pickerPointerSize * 2 }
satValPickerBorderRadius={ borderRadius }
huePickerBorderRadius={ borderRadius }
testID="color-picker"
/>
<View style={ footerStyle }>
<TouchableWithoutFeedback
Expand All @@ -176,10 +175,7 @@ function ColorPicker( {
</View>
</TouchableWithoutFeedback>
{ bottomLabelText ? (
<Text
testID="color-picker-bottom-label-text"
style={ selectColorTextStyle }
>
<Text style={ selectColorTextStyle }>
{ bottomLabelText }
</Text>
) : (
Expand Down
2 changes: 2 additions & 0 deletions test/native/__mocks__/styleMock.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,4 +202,6 @@ module.exports = {
embed__icon: {
fill: 'black',
},
picker: {},
pickerPointer: {},
Comment on lines +205 to +206
Copy link
Member Author

@dcalhoun dcalhoun Jul 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Due to the way we rely upon Sass files, reference individual styles from them in JavaScript, and subsequently mock them in tests, we have to add individual style mocks in this file. It is quite cryptic and cumbersome.

An alternative is mocking the values within the test file itself, which is arguably less cryptic. The inline mock is a recent practice introduced by @geriux, one that is likely worth embracing IMO.

Copy link
Member

Choose a reason for hiding this comment

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

Thank you for pointing this out. This is the part I was missing.

};
8 changes: 5 additions & 3 deletions test/native/setup.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,9 +201,11 @@ jest.mock( 'react-native-linear-gradient', () => () => 'LinearGradient', {
virtual: true,
} );

jest.mock( 'react-native-hsv-color-picker', () => () => 'HsvColorPicker', {
virtual: true,
} );
jest.mock(
'react-native-hsv-color-picker',
() => jest.fn( () => 'HsvColorPicker' ),
{ virtual: true }
);

jest.mock( '@react-native-community/blur', () => () => 'BlurView', {
virtual: true,
Expand Down
Loading