Add missing tests for Format API code #11562

Pixelrobin · 2018-11-07T05:13:46Z

Description

Addressed #11110 by adding various tests. I used tests from other parts of the code as a reference. I am new to Jest and Gutenberg development though.

Types of changes

Adds tests to several functions in the Format API, which should hopefully not break anything?

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.

ellatrix · 2018-11-07T08:16:31Z

Thanks for the tests @Pixelrobin!

ellatrix · 2018-11-07T08:18:11Z

packages/rich-text/src/test/get-format-type.js

+
+describe( 'getFormatType', () => {
+	// Initialize format store.
+	require( '../store' );


Shouldn't this be wrapped in a beforeAll?

Oh, my bad. I think I did it like this all the way though. I'll fix it.

ellatrix · 2018-11-07T08:19:38Z

packages/rich-text/src/test/insert-object.js

+	const obj = { type: 'obj' };
+	const em = { type: 'em' };
+
+	const OBJECT_REPLACEMENT_CHARACTER = '\ufffc';


There's a special characters file with these constants that you can import.

Oh yeah, I noticed that. I was working off an older version that didn't have that implemented yet. I assume updating the branch would require me to take care of the merge conflict myself?

After a rebase you could use the constants. :)

I usually do the following on my branch:

git fetch git rebase origin/master

you fix conflicts and:

git add . git rebase --continue

ellatrix · 2018-11-07T08:22:37Z

packages/rich-text/src/test/insert-object.js

+		};
+		const expected = {
+			formats: [ , , [ { ...obj, object: true } ], [ em ], , , , , , , ],
+			text: 'on' + OBJECT_REPLACEMENT_CHARACTER + 'o three',


Minor: you could use template literals here:

`on${ OBJECT_REPLACEMENT_CHARACTER }o three`

It reads a bit easier to me.

ellatrix · 2018-11-07T08:25:55Z

Looks good! Needs a rebase. There were some tests added for registerFormatType which will duplicate some tests here.

gziolo · 2018-11-07T08:28:53Z

packages/rich-text/src/test/get-format-types.js

+
+describe( 'getFormatTypes', () => {
+	// Initialize format store.
+	require( '../store' );


The same thing with beforeAll.

gziolo · 2018-11-07T08:31:02Z

packages/rich-text/src/test/get-format-types.js

+	} );
+
+	it( 'should return all registered formats', () => {
+		const formatType1 = { edit: noop, title: 'format title' };


I personally like more descriptive names for variables.
How about:

testFormat

testFormatWithSettings
to align with the names used as first param in registerFormatType?

gziolo · 2018-11-07T08:32:46Z

packages/rich-text/src/test/insert-object.js

+	const obj = { type: 'obj' };
+	const em = { type: 'em' };
+
+	const OBJECT_REPLACEMENT_CHARACTER = '\ufffc';


I usually do the following on my branch:

git fetch git rebase origin/master

you fix conflicts and:

git add . git rebase --continue

gziolo · 2018-11-07T08:33:57Z

packages/rich-text/src/store/test/selectors.js

+import { getFormatTypes, getFormatType } from '../selectors';
+
+describe( 'selectors', () => {
+	const defaultState = {


It's a good practice to wrap state with deepFreeze to ensure that selectors never mutate state.

gziolo · 2018-11-07T08:34:32Z

packages/rich-text/src/test/register-format-type.js

+	const defaultFormatSettings = { edit: noop, title: 'format title' };
+
+	// Initialize format store.
+	require( '../store' );


beforeAll again :)
I also noticed that comment isn't 100% accurate. It is rich text store.

In general, we need to come up with a nicer way of using stores with tests.

gziolo · 2018-11-07T08:36:39Z

packages/rich-text/src/test/unregister-format-type.js

+	const defaultFormatSettings = { edit: noop, title: 'format title' };
+
+	// Initialize format store.
+	require( '../store' );


beforeAll :)

gziolo · 2018-11-07T08:37:51Z

packages/rich-text/src/test/unregister-format-type.js

+
+	it( 'should unregister existing formats', () => {
+		registerFormatType( 'core/test-format', defaultFormatSettings );
+		expect( getFormatTypes() ).toEqual( [


It might read better when getFormatType is used here as core/test-format' is the only thing we care about here..

gziolo · 2018-11-07T08:41:20Z

This was my first Gutenberg PR and first time using Jest, so I learned a lot 😄, thanks for that opportunity. I hope it's good to go!

You learn really fast, awesome work! :)
We need rebase and a few tweaks and we should be good to go.

There were some tests added for registerFormatType which will duplicate some tests here.

Yes, this is the only tricky part. I left some hint how I usually do it. Just make sure we don't have duplicated tests in that file. A few of tests you added are missing in the test suite added by @iseulde yesterday.

youknowriad · 2018-11-07T10:18:52Z

Moved to 4.4 as it's not a blocker for 4.3. Let's merge it if it's ready though.

Pixelrobin · 2018-11-07T21:57:43Z

Ok, I think I applied all the feedback, thanks!

Used beforeAll on initializing rich-text store
Used deepFreeze on state objects.
Imported OBJECT_REPLACEMENT_CHARACTER
Fixed some variable naming
Used getFormatType instead of getFormatTypes in test/unregister-format-type.js. I do think it should be noted that the original way was how it's done in the block API tests.
test/register-format-type.js has changed quite a bit as I mashed the two versions together.
Changes to registerFormatType object requirements led to some other changes to format registration in certain tests.
Fixed minor documentation bug in insert-object.js, where it incorrectly labeled an object property as a string.

Rebasing added the gutenberg-mobile submodule to the branch for some reason, so I removed it in another commit. So this should definitely be squashed :D

gziolo

Awesome work, thanks for addressing all feedback 💯

gziolo · 2018-11-08T08:27:26Z

@Pixelrobin congrats on your first code contribution! 🎉

gziolo · 2018-11-08T08:29:13Z

packages/rich-text/src/test/get-format-type.js

+			title: 'format title',
+			keywords: [ 'one', 'two', 'three' ],
+			formatTestSetting: 'settingTestValue',
+			tagName: 'test',


@iseulde - should we add validation for tagName property? Values like testandtest 2` made me think about it.

…rnmobile/fix-merge-content-not-refreshed-UI * 'master' of https://github.com/WordPress/gutenberg: Add missing tests for Format API code (#11562) chore: Improve undo/redo no-op (#11428) Fix: Contrast checker: Consider fontSize large when size >= 24px instead of >= 18px (#9268)

Add missing tests for Format API code

2973ece

Pixelrobin mentioned this pull request Nov 7, 2018

Tests: Add missing tests for Format API code #11110

Closed

10 tasks

gziolo added [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable labels Nov 7, 2018

gziolo added this to the 4.3 milestone Nov 7, 2018

gziolo requested review from gziolo and ellatrix November 7, 2018 07:44

gziolo assigned Pixelrobin Nov 7, 2018

ellatrix reviewed Nov 7, 2018

View reviewed changes

gziolo reviewed Nov 7, 2018

View reviewed changes

youknowriad modified the milestones: 4.3, 4.4 Nov 7, 2018

gziolo mentioned this pull request Nov 7, 2018

Testing: Add automated testing for Format API #11583

Closed

Add missing tests for Format API code

fd7a191

Pixelrobin force-pushed the add/missing-format-api-tests branch from 2973ece to fd7a191 Compare November 7, 2018 19:49

Pixelrobin added 2 commits November 7, 2018 11:56

Remove submodule

fe02444

Rebase, fix, and apply feedback

ec88346

gziolo approved these changes Nov 8, 2018

View reviewed changes

gziolo modified the milestones: 4.4, 4.3 Nov 8, 2018

gziolo merged commit a9b0742 into WordPress:master Nov 8, 2018

gziolo reviewed Nov 8, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add missing tests for Format API code #11562

Add missing tests for Format API code #11562

Pixelrobin commented Nov 7, 2018 •

edited by gziolo

Loading

ellatrix commented Nov 7, 2018

ellatrix Nov 7, 2018

Pixelrobin Nov 7, 2018 •

edited

Loading

ellatrix Nov 7, 2018

Pixelrobin Nov 7, 2018

ellatrix Nov 7, 2018

gziolo Nov 7, 2018

ellatrix Nov 7, 2018

ellatrix commented Nov 7, 2018

gziolo Nov 7, 2018

gziolo Nov 7, 2018

gziolo Nov 7, 2018

gziolo Nov 7, 2018

gziolo Nov 7, 2018

gziolo Nov 7, 2018

gziolo Nov 7, 2018

gziolo commented Nov 7, 2018 •

edited

Loading

youknowriad commented Nov 7, 2018

Pixelrobin commented Nov 7, 2018 •

edited

Loading

gziolo left a comment

gziolo commented Nov 8, 2018 •

edited

Loading

gziolo Nov 8, 2018

Add missing tests for Format API code #11562

Add missing tests for Format API code #11562

Conversation

Pixelrobin commented Nov 7, 2018 • edited by gziolo Loading

Description

Types of changes

Checklist:

ellatrix commented Nov 7, 2018

Choose a reason for hiding this comment

Pixelrobin Nov 7, 2018 • edited Loading

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

ellatrix commented Nov 7, 2018

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

Choose a reason for hiding this comment

gziolo commented Nov 7, 2018 • edited Loading

youknowriad commented Nov 7, 2018

Pixelrobin commented Nov 7, 2018 • edited Loading

gziolo left a comment

Choose a reason for hiding this comment

gziolo commented Nov 8, 2018 • edited Loading

Choose a reason for hiding this comment

Pixelrobin commented Nov 7, 2018 •

edited by gziolo

Loading

Pixelrobin Nov 7, 2018 •

edited

Loading

gziolo commented Nov 7, 2018 •

edited

Loading

Pixelrobin commented Nov 7, 2018 •

edited

Loading

gziolo commented Nov 8, 2018 •

edited

Loading