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.

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

Add tests for the BlockMover Component. #980

Merged
merged 3 commits into from
Nov 13, 2017
Merged

Conversation

BE-Webdesign
Copy link
Contributor

Adds tests for the BlockMover Component. Related to progress on #641.
The tests ensure that the callbacks render properly depending on whether
the block isFirst or isLast.

@BE-Webdesign BE-Webdesign added the [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. label Jun 1, 2017
@@ -15,7 +15,8 @@ import { IconButton } from 'components';
import './style.scss';
import { isFirstBlock, isLastBlock } from '../selectors';

function BlockMover( { onMoveUp, onMoveDown, isFirst, isLast } ) {
// Only exported for testing.
Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit conflicted on including the comment. I think I might be motivated to avoid acknowledging the existence of tests from runtime code. The value of this named export isn't exclusive to tests only, but any consumer who'd want the presentational component. Then again, the distinction between importing a named export and the module default doesn't really clearly communicate this difference. It's rare though that we'd ever want to import the unconnected component in the UI, so this worry is not likely to come to light.

Which is all to say... I don't think we ought to include the comment, even though in all reality it truly is only exported for testing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will stop adding this comment in for connected components or similar things. Is the runtime code not stripping comments out?

Copy link
Member

Choose a reason for hiding this comment

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

Is the runtime code not stripping comments out?

They are stripped in the minified (production) bundle, yes.

expect( moveDown.type().name ).to.equal( 'IconButton' );
expect( moveUp.props() ).to.include( {
className: 'editor-block-mover__control',
onClick: undefined,
Copy link
Member

Choose a reason for hiding this comment

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

The behavior of includes and undefined here is such that even if the IconButton did assign an onClick handler, the test would still pass. You can see this with the following diff applied:

diff --git a/editor/block-mover/index.js b/editor/block-mover/index.js
index f040d239..cba1a09c 100644
--- a/editor/block-mover/index.js
+++ b/editor/block-mover/index.js
@@ -25,7 +25,7 @@ function BlockMover( { onMoveUp, onMoveDown, isFirst, isLast } ) {
                <div className="editor-block-mover">
                        <IconButton
                                className="editor-block-mover__control"
-                               onClick={ isFirst ? null : onMoveUp }
+                               onClick={ isFirst ? function() {} : onMoveUp }
                                icon="arrow-up-alt2"
                                aria-disabled={ isFirst }
                        />

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not fully following this, but I will look it over and hopefully come up with a better solution that will better match the actual logic going on.

Copy link
Member

Choose a reason for hiding this comment

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

More specifically, it interprets the assertion without making any expectations about the value of onClick, so the test would behave the same if the onClick property weren't included at all.

expect( moveUp.props() ).to.include( {
	className: 'editor-block-mover__control',
	icon: 'arrow-up-alt2',
	label: 'Move 2 blocks from position 1 up by one place',
	'aria-disabled': undefined,
} );

Which, of course, is probably not what we want.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It always comes out as undefined because isFirst is undefined. JSX conversion will interpret the value of onClick to be undefined regardless of what is on the other side of ? in our ternary. Do you want to pass in isFirst and isLast as false for each, I added another set of test cases to accomplish what I think you are looking for?

Copy link
Member

Choose a reason for hiding this comment

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

I think we just need to change this from onClick: undefined to onClick: null in the test?

Copy link
Contributor Author

@BE-Webdesign BE-Webdesign Jun 12, 2017

Choose a reason for hiding this comment

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

It will show up undefined because of how I have the JSX and the way I am creating the Element for the test, i.e. with isFirst undefined. Should I add in failing tests for a different expected behavior or should I be matching what is already expected behavior in the component?

Copy link
Member

Choose a reason for hiding this comment

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

Okay, looking at this again, I think I see what's happening. I assumed the test case was asserting on the null value passed to onClick, and that includes assertion wasn't smart enough to differentiate between a value being explicitly undefined, vs. not making an assertion on that value.

Instead, includes will make this assert that the value is explicitly undefined, and it's not the null value we're expecting here, it's onMoveUp, which in this case is undefined because we're not passing this prop to the component in the test.

Which is all to say... this is fine 😄

Copy link
Contributor Author

@BE-Webdesign BE-Webdesign Jun 14, 2017

Choose a reason for hiding this comment

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

Yeah, I probably did a bad job of explaining that. It also took me a while to figure out the conditional as well.

} );

it( 'should render the up arrow with a onMoveUp callback', () => {
const onMoveUp = ( event ) => event;
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably wouldn't hurt.

Copy link
Member

Choose a reason for hiding this comment

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

With the Mocha to Jest migration it would require using jest.fn() instead to achieve similar result.

@BE-Webdesign
Copy link
Contributor Author

Thank you for the review, I will get back to this at some point next weekish. 👍 on all feedback.

@BE-Webdesign
Copy link
Contributor Author

Looks like the BlockMover has changed a bit since I last was on this branch, I will add extra tests as well.

@BE-Webdesign BE-Webdesign force-pushed the add/test/editor/block-mover branch 2 times, most recently from e53d83d to b10ee8c Compare June 14, 2017 16:48
@gziolo
Copy link
Member

gziolo commented Sep 21, 2017

It looks like we updated tests to use Jest and its API in the meantime. We no longer use chai. @BE-Webdesign, do you plan to continue your work on this PR? Let me know if you need help with expect changes to make it work.

BE-Webdesign and others added 3 commits November 13, 2017 08:17
Adds tests for the BlockMover Component.  Related to progress on #641.
The tests ensure that the callbacks render properly depending on whether
the block isFirst or isLast.
Updating the tests to incorporate new changes to BlockMover.
@gziolo gziolo force-pushed the add/test/editor/block-mover branch from b10ee8c to fd3f21a Compare November 13, 2017 07:26
@codecov
Copy link

codecov bot commented Nov 13, 2017

Codecov Report

Merging #980 into master will increase coverage by 0.08%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #980      +/-   ##
==========================================
+ Coverage   33.81%   33.89%   +0.08%     
==========================================
  Files         250      250              
  Lines        6734     6723      -11     
  Branches     1218     1218              
==========================================
+ Hits         2277     2279       +2     
+ Misses       3763     3750      -13     
  Partials      694      694
Impacted Files Coverage Δ
editor/block-mover/index.js 10% <ø> (+10%) ⬆️
editor/block-mover/mover-label.js 100% <0%> (+4.54%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 77ea75a...fd3f21a. Read the comment docs.

@gziolo
Copy link
Member

gziolo commented Nov 13, 2017

Should we do any click simulations with a Sinon spy?

https://github.com/airbnb/enzyme/blob/master/docs/api/ShallowWrapper/simulate.md

@aduth we can open a follow-up PR to add more test cases.

I rebased with master and update code to work with Jest. It looks like ready to merge. We can iterate later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants