Add end 2 end test to block icons #7589

jorgefilipecosta · 2018-06-27T19:51:15Z

Description

Our logic for block icons is complex and contains many possible ways to set an icon.
This PR adds set of the end 2 end tests to block icons in order to make it harder to regress in the future.

How has this been tested?

I executed the tests and verified they pass with success.

gziolo

Those tests are very granular. I’m aware that it’s a lot of work to setup blocks to validate how they fit into inserter. However, do you see any of those tests to be converted into unit tests? Maybe it could be possible to test Inserter’s item from search results with a set of props that mirror what we have here? My point is that e2e tests are super slow comparing to unit tests. We test here some interactions and integration with custom blocks which is awesome, but maybe some details could be covered as component tests. What do you think?

gziolo · 2018-06-29T05:06:24Z

test/e2e/test-plugins/block-icons/index.js

+	var registerBlockType = wp.blocks.registerBlockType;
+	var el = wp.element.createElement;
+	var InnerBlocks = wp.editor.InnerBlocks;
+    var circle = el( 'circle', { cx: 10, cy: 10, r: 10, fill: 'red', stroke: 'blue', strokeWidth: '10' } );


Whitespaces need some tweaks.

It still mixes spaces and tabs.

gziolo · 2018-06-29T05:10:29Z

test/e2e/specs/block-icons.test.js

+	} );
+
+	afterAll( async () => {
+		await newDesktopBrowserPage();


Is this necessary? I can’t think of any reason to open a page just before test suite ends.

I agree we don't need to open a new page when the test is ending. I followed what we are doing in

gutenberg/test/e2e/specs/plugins-api.test.js

Lines 20 to 23 in ed8e261

afterAll( async () => {

await newDesktopBrowserPage();

await deactivatePlugin( 'gutenberg-test-plugin-plugins-api' );

} );

I thought we might have a reason to disable plugins in a new page, but I removed this line, and it seems everything is fine.

gziolo · 2018-06-29T05:12:47Z

test/e2e/specs/block-icons.test.js

+	return await getInnerHTML( INSERTER_ICON_SELECTOR );
+}
+
+describe( 'Correctly Renders Block Icons on Inserter and Inspector', () => {


Why not use Block Icons here to make snapshots smaller? There is some duplication with the name of individual tests.

I removed the snapshots. We now use another validatiom mechanism so we don't have duplication of snapshots.

gziolo · 2018-06-29T05:22:14Z

test/e2e/support/utils.js

+/**
+ * Inserts first inserter block. Inserter needs to be open.
+ */
+export async function insertFirstBlockInInserter() {


Should it be insertFirstBlockFromInserter or insetFirstBlockFoundInInserter instead? It is a bit confusing at the moment. Does it need to be exposed? It isn’t referenced outside.

Updated during the rebase.

gziolo · 2018-06-29T05:25:09Z

test/e2e/test-plugins/block-icons/index.js

+	var el = wp.element.createElement;
+	var InnerBlocks = wp.editor.InnerBlocks;
+    var circle = el( 'circle', { cx: 10, cy: 10, r: 10, fill: 'red', stroke: 'blue', strokeWidth: '10' } );
+    var svg = el( 'svg', { width: 20, height: 20, viewBox: '0 0 20 20'}, circle)


; is missing at the end of the line.

gziolo · 2018-06-29T05:26:12Z

test/e2e/test-plugins/block-icons/index.js

+		},
+		category: 'common',
+
+		edit() {


It’s ES6 syntax - object method shorthand 😃

gziolo · 2018-06-29T05:29:41Z

test/e2e/test-plugins/block-icons/index.js

@@ -0,0 +1,157 @@
+( function() {


Is fine as is, but this plugin might be useful also to test other things. I see it applicable with very slight tweaks to verify custom block categories hook.

jorgefilipecosta · 2018-06-29T13:47:48Z

Hi @gziolo we already have unit tests for the blocks registration, the inserter, and the icon rendering component. The problem is that each part has its own logic and may transform the way an icon is represented/rendered. Each part may work correctly alone but we may have problems on the integration.
I'm not seeing how we can test the flow of a custom block setting an icon until that icon passes by the transformations and appears on the UI without an end to end test. It is similar to our end to end test of PluginPostStatusInfo we can unit test each part but to test all the flow we used an end 2 end test.

That said I agree this test right now is very granular. If I'm not in error this is our first end2end test for the registration of a custom block :). In this case for a very specific test. I think we can see this test case as a start. Right now it is only checking the icons but after we can add other checks, e.g: if the categories appear as they should, or if we can add blocks inside. Of course, when needed we should rename the test case file and the name of the tests if we add other checks.

gziolo · 2018-07-10T12:27:13Z

test/e2e/test-plugins/block-icons.php

+		'wp-hooks',
+		'wp-i18n'
+	),
+	filemtime( plugin_dir_path( __FILE__ ) . 'hooks-api/index.js' ),


Copy and paste issue, it should be:
filemtime( plugin_dir_path( __FILE__ ) . 'block-icons/index.js' ), :)

gziolo · 2018-07-10T12:28:06Z

test/e2e/test-plugins/block-icons/index.js

+			);
+		},
+	} );
+


it has obsolete empty line.

gziolo

Let's follow your judgment get it in as is. It's a very nice addition to our suite of tests. I'm looking forward to see how it evolves.

There are 2 or 3 nitpicks to address before you merge it.

gziolo · 2018-07-10T12:31:16Z

#7589 (comment) - regarding this one, we probably should add some basic linting which doesn't expect ES.Next. I will get to it at some point :)

aduth · 2018-07-11T14:34:27Z

test/e2e/specs/block-icons.test.js

+}
+
+describe( 'Correctly Renders Block Icons on Inserter and Inspector', () => {
+	const dashIconRegex = /<svg.*class=".*dashicons-cart.*?">.*<\/svg>/;


Across many branches, I'm getting test failures in this suite when running locally.

Example:

Expected value to match: /<svg.*class=".*dashicons-cart.*?">.*<\/svg>/ Received: "<svg aria-hidden=\"true\" role=\"img\" focusable=\"false\" class=\"dashicon dashicons-editor-paragraph\" xmlns=\"http://www.w3.org/2000/svg\" width=\"20\" height=\"20\" viewBox=\"0 0 20 20\"><path d=\"M15 2H7.54c-.83 0-1.59.2-2.28.6-.7.41-1.25.96-1.65 1.65C3.2 4.94 3 5.7 3 6.52s.2 1.58.61 2.27c.4.69.95 1.24 1.65 1.64.69.41 1.45.61 2.28.61h.43V17c0 .27.1.51.29.71.2.19.44.29.71.29.28 0 .51-.1.71-.29.2-.2.3-.44.3-.71V5c0-.27.09-.51.29-.71.2-.19.44-.29.71-.29s.51.1.71.29c.19.2.29.44.29.71v12c0 .27.1.51.3.71.2.19.43.29.71.29.27 0 .51-.1.71-.29.19-.2.29-.44.29-.71V4H15c.27 0 .5-.1.7-.3.2-.19.3-.43.3-.7s-.1-.51-.3-.71C15.5 2.1 15.27 2 15 2z\"></path></svg>"

Also, the regular expression is prone to greediness.

I tried to debug the problem locally and if I execute the test normally they always pass. If make them slow with PUPPETEER_SLOWMO=30 they start to fail because instead of the expected block being selected the paragraph is selected. That matches with the error message you are seeing. I'm researching why the paragraph is getting selected.

jorgefilipecosta added [Feature] Blocks Overall functionality of blocks [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. labels Jun 27, 2018

jorgefilipecosta self-assigned this Jun 27, 2018

jorgefilipecosta force-pushed the add/end-2-end-test-block-icons branch from 598413a to 5c4200e Compare June 27, 2018 19:51

jorgefilipecosta requested a review from gziolo June 27, 2018 20:35

jorgefilipecosta added this to the 3.2 milestone Jun 28, 2018

gziolo reviewed Jun 29, 2018

View reviewed changes

jorgefilipecosta force-pushed the add/end-2-end-test-block-icons branch 2 times, most recently from 01d8416 to ed8e261 Compare June 29, 2018 12:36

jorgefilipecosta modified the milestones: 3.2, 3.3 Jul 5, 2018

gziolo reviewed Jul 10, 2018

View reviewed changes

test/e2e/test-plugins/block-icons/index.js

);

},

} );

Copy link

Member

gziolo Jul 10, 2018

Choose a reason for hiding this comment

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

it has obsolete empty line.

jorgefilipecosta reacted with thumbs up emoji

gziolo approved these changes Jul 10, 2018

View reviewed changes

Add end 2 end test to block icons

956ab99

jorgefilipecosta force-pushed the add/end-2-end-test-block-icons branch from ed8e261 to 956ab99 Compare July 10, 2018 13:43

jorgefilipecosta merged commit 6ce81bf into master Jul 10, 2018

jorgefilipecosta deleted the add/end-2-end-test-block-icons branch July 10, 2018 15:51

aduth reviewed Jul 11, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add end 2 end test to block icons #7589

Add end 2 end test to block icons #7589

jorgefilipecosta commented Jun 27, 2018

gziolo left a comment

gziolo Jun 29, 2018

jorgefilipecosta Jun 29, 2018

gziolo Jul 10, 2018

gziolo Jun 29, 2018

jorgefilipecosta Jun 29, 2018

gziolo Jun 29, 2018

jorgefilipecosta Jun 29, 2018

gziolo Jun 29, 2018

jorgefilipecosta Jun 29, 2018

gziolo Jun 29, 2018

jorgefilipecosta Jun 29, 2018

gziolo Jun 29, 2018

gziolo Jun 29, 2018

jorgefilipecosta commented Jun 29, 2018 •

edited

Loading

gziolo Jul 10, 2018

gziolo Jul 10, 2018

gziolo left a comment

gziolo commented Jul 10, 2018

aduth Jul 11, 2018

jorgefilipecosta Jul 11, 2018

	afterAll( async () => {
	await newDesktopBrowserPage();
	await deactivatePlugin( 'gutenberg-test-plugin-plugins-api' );
	} );

Add end 2 end test to block icons #7589

Add end 2 end test to block icons #7589

Conversation

jorgefilipecosta commented Jun 27, 2018

Description

How has this been tested?

gziolo 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

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

Choose a reason for hiding this comment

jorgefilipecosta commented Jun 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo left a comment

Choose a reason for hiding this comment

gziolo commented Jul 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorgefilipecosta commented Jun 29, 2018 •

edited

Loading