Add tests for the BlockSwitcher component. #990

BE-Webdesign · 2017-06-02T03:17:10Z

These tests are related to progress on #641. They handle all instance
methods and any conditional rendering logic.

Testing Instructions
Verify that registering of blocks is indeed cleaned up and that tests
pass.

gziolo

Those tests need to be updated to work with Jest. We no longer use chai and sinon. @BE-Webdesign, let me know if I should help you with it.

BE-Webdesign · 2017-09-22T15:24:38Z

Hi Grzegors, Thank you for the information and offer to help. I would be glad to revise this to Jest and use snapshots instead. Are we switching back off of Jest though (#2757)? I don't want to revise something that might be reverted.

gziolo · 2017-09-25T09:56:36Z

I think we are going to stay with Jest after very surprising announcement from Facebook about relicensing under the MIT license :)

In that case I recommend using jest-codemods to update tests. It should work out of the box for everything but Sinon spy. I used those codemods when updating code to work with Jest API, it's amazing!

gziolo · 2017-09-25T10:03:25Z

Sinon support is being worked on in jest-codemods project, see skovhus/jest-codemods#68 - now with your code examples :)

gziolo · 2017-11-13T08:14:22Z

I'm afraid it might be very difficult to make those test work since <BlockSwitcher /> was refactored to be a functional component that uses <Dropdown /> component and works with multiple blocks, not only one as before. I still think it would be a very beneficial to have this logic covered with tests. @youknowriad what is your take on this as you have been involved with the work on <Dropdown /> reusable component?

gziolo · 2017-12-04T07:51:27Z

editor/components/block-switcher/test/__snapshots__/index.js.snap

+</div>
+`;
+
+exports[`BlockSwitcher Test block switcher with multi block of different types. 1`] = `null`;


If it renders nothing, you can simply assert the following:

expect( wrapper.html() ).toBe( null );

when using enzyme to avoid creating snapshot for such test.

See

gutenberg/editor/components/document-outline/test/index.js

Line 34 in bccede6

it( 'should not render when no heading blocks provided', () => {

for reference.

gziolo · 2017-12-04T07:57:14Z

editor/components/block-switcher/test/index.js

+			block,
+		];
+
+		const tree = renderer


This way is promoted by Jest team in their official docs. I personally prefer using enzyme to test React components, but this it might be an easier way in case when you have only snapshot tests in the file. Enzyme is more helpful when doing more complex checks.

Yeah, Enzyme is great for getting into the details. Not sure, how we want to use snapshots etc.

It has new iterations

gziolo · 2017-12-08T14:38:04Z

editor/components/block-switcher/test/multi-blocks-switcher.js

+
+	describe( 'mapStateToProps', () => {
+		test( 'should return the expected selected block uids and whether there is a multiselection.', () => {
+			//const isMultiBlockSelection = true;


Obsolete comment, should be removed.

gziolo · 2017-12-08T14:38:38Z

editor/components/block-switcher/test/multi-blocks-switcher.js

+				},
+			};
+			const expected = {
+				isMultiBlockSelection: selectedBlockUids.length > 1,


It would read better to have explicit value here instead.

gziolo · 2017-12-08T14:39:13Z

editor/components/block-switcher/test/multi-blocks-switcher.js

+		expect( MultiBlocksSwitcher( { isMultiBlockSelection, selectedBlockUids } ) ).toMatchSnapshot();
+	} );
+
+	describe( 'mapStateToProps', () => {


That's interesting, it seems like a good idea to test this, too 👍

I kind of disagree with this, I dislike the fact that we export these functions for test purpose. In general if the behavior of these functions is complex enough to deserve a test, it's complex enough to deserve a selector :)

Would using mock stores be preferable?

No, just testing the wrapped component instead and providing the right props. That's what we do for most components.

gziolo · 2017-12-08T14:40:13Z

editor/components/block-switcher/test/multi-blocks-switcher.js

+ */
+import { MultiBlocksSwitcher, mapStateToProps } from '../multi-blocks-switcher';
+
+describe( 'MultiBlocksSwitcher', () => {


This test suite is excellent. This is exactly what we need 👍

gziolo · 2017-12-08T14:40:35Z

editor/components/block-switcher/test/index.js

+
+		expect( dispatch ).toHaveBeenCalled();
+
+		/**


This comment should be removed.

Ooops. 🙃

gziolo · 2017-12-08T14:41:52Z

editor/components/block-switcher/test/index.js

+	describe( 'mapStateToProps', () => {
+		test( 'should return an object containing the blocks list as a blocks prop.', () => {
+			const blocks = [
+				{


Can we move this block to it's own variable to avoid repetition in state?

gziolo · 2017-12-08T14:43:21Z

editor/components/block-switcher/test/index.js

+	} );
+
+	test( 'Test block switcher with multi block of different types.', () => {
+		const block1 = {


Can we move those blocks level up and reuse in other tests to avoid code duplication? We can also give them names like headingBlock and paragraphBlock if that is relevant.

I would have to double check, but I am pretty sure that would work.

gziolo · 2017-12-08T14:46:14Z

editor/components/block-switcher/test/__snapshots__/index.js.snap

+/>
+`;
+
+exports[`BlockSwitcher Test block switcher with multi block of different types. 1`] = `""`;


According to this snapshots 3 tests don't render anything. It would read better to check against null in the test instead.

Yup, I need to look back through this to update.

gziolo

I left some comments. Lots of lines added here. I proposed some refactoring to avoid some code duplication.

BE-Webdesign · 2017-12-08T19:30:43Z

Okay, much more condensed now. Not at 100% coverage, but close enough, this can be touched up again later, when all of the other components have a higher coverage ratio.

youknowriad · 2018-01-29T12:46:33Z

Can we test the BlockSwitcher component UI only (like other components). I'd be happy to merge this PR if it's the case :)

…rity.

Updated with the latest changes in the master branch

gziolo · 2018-03-08T09:53:11Z

@youknowriad removed all non UI related tests.

gziolo

This is good to go 👍

gziolo · 2018-03-08T09:55:10Z

Thanks @BE-Webdesign for working on this one. Sorry, it took so long to get it merged.

BE-Webdesign added the [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. label Jun 2, 2017

BE-Webdesign mentioned this pull request Jun 2, 2017

Unit Testing Components #641

Closed

BE-Webdesign force-pushed the add/test/editor/block-switcher branch from 250b744 to 8de49b9 Compare June 14, 2017 16:41

gziolo previously requested changes Sep 22, 2017

View reviewed changes

BE-Webdesign closed this Dec 3, 2017

BE-Webdesign force-pushed the add/test/editor/block-switcher branch from 8de49b9 to 429fb42 Compare December 3, 2017 15:41

BE-Webdesign reopened this Dec 3, 2017

gziolo reviewed Dec 4, 2017

View reviewed changes

BE-Webdesign force-pushed the add/test/editor/block-switcher branch from 7e191a4 to 167d1be Compare December 8, 2017 01:52

gziolo reviewed Dec 8, 2017

View reviewed changes

gziolo previously requested changes Dec 8, 2017

View reviewed changes

BE-Webdesign force-pushed the add/test/editor/block-switcher branch 2 times, most recently from a9b6619 to 3b58b63 Compare December 8, 2017 19:27

BE-Webdesign added 2 commits March 7, 2018 15:59

Add unit tests for BlockSwitcher component.

1be8727

Refactor BlockSwitcher tests to reduce repetition and increase modula…

55d253a

…rity.

Tests: Update block switcher tests to make them up to date

9d7197d

gziolo force-pushed the add/test/editor/block-switcher branch from 3b58b63 to 9d7197d Compare March 8, 2018 09:34

gziolo approved these changes Mar 8, 2018

View reviewed changes

gziolo merged commit 107b0a7 into master Mar 8, 2018

gziolo deleted the add/test/editor/block-switcher branch March 8, 2018 09:54

gziolo self-assigned this Mar 8, 2018

snyk-bot mentioned this pull request Feb 1, 2020

[Snyk] Fix for 1 vulnerabilities SeppPenner/gutenberg#114

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tests for the BlockSwitcher component. #990

Add tests for the BlockSwitcher component. #990

BE-Webdesign commented Jun 2, 2017

gziolo left a comment

BE-Webdesign commented Sep 22, 2017

gziolo commented Sep 25, 2017

gziolo commented Sep 25, 2017

gziolo commented Nov 13, 2017

gziolo Dec 4, 2017 •

edited

Loading

gziolo Dec 4, 2017

BE-Webdesign Dec 4, 2017

gziolo Dec 8, 2017

gziolo Dec 8, 2017

gziolo Dec 8, 2017

youknowriad Jan 29, 2018

BE-Webdesign Jan 29, 2018

youknowriad Jan 29, 2018

gziolo Dec 8, 2017

gziolo Dec 8, 2017

BE-Webdesign Dec 8, 2017

gziolo Dec 8, 2017

gziolo Dec 8, 2017 •

edited

Loading

BE-Webdesign Dec 8, 2017

gziolo Dec 8, 2017

BE-Webdesign Dec 8, 2017

gziolo left a comment

BE-Webdesign commented Dec 8, 2017

youknowriad commented Jan 29, 2018

gziolo commented Mar 8, 2018

gziolo left a comment

gziolo commented Mar 8, 2018

Add tests for the BlockSwitcher component. #990

Add tests for the BlockSwitcher component. #990

Conversation

BE-Webdesign commented Jun 2, 2017

gziolo left a comment

Choose a reason for hiding this comment

BE-Webdesign commented Sep 22, 2017

gziolo commented Sep 25, 2017

gziolo commented Sep 25, 2017

gziolo commented Nov 13, 2017

gziolo Dec 4, 2017 • 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

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 Dec 8, 2017 • 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

gziolo left a comment

Choose a reason for hiding this comment

BE-Webdesign commented Dec 8, 2017

youknowriad commented Jan 29, 2018

gziolo commented Mar 8, 2018

gziolo left a comment

Choose a reason for hiding this comment

gziolo commented Mar 8, 2018

gziolo Dec 4, 2017 •

edited

Loading

gziolo Dec 8, 2017 •

edited

Loading