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

Post Status: Add component tests #8128

Merged
merged 1 commit into from
Sep 19, 2016
Merged

Post Status: Add component tests #8128

merged 1 commit into from
Sep 19, 2016

Conversation

mattm
Copy link
Contributor

@mattm mattm commented Sep 17, 2016

This PR adds tests for the Post Status component as described in #7942.

To test, run npm run test-client client/blocks/post-status/ and verify that all of the tests pass.

Additionally, review the Post Status component and ensure that the test covers all of the relevant conditions.

@mattm mattm added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. Components labels Sep 17, 2016
@mattm mattm added this to the GM Course - Testing milestone Sep 17, 2016
@mattm mattm assigned mattm and yoavf Sep 17, 2016
@matticbot
Copy link
Contributor

import useFakeDom from 'test/helpers/use-fake-dom';

describe( 'PostStatus', ( ) => {
useFakeDom();
Copy link
Contributor

@yoavf yoavf Sep 17, 2016

Choose a reason for hiding this comment

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

Seems like we don't actually need this, so this and the matching import can be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call, removed.

@yoavf yoavf added [Status] Needs Author Reply and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Sep 17, 2016
@mattm mattm force-pushed the add/post-status-tests branch from 5b3e73b to 88c368c Compare September 19, 2016 18:03
@mattm mattm added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] Needs Author Reply labels Sep 19, 2016
@yoavf yoavf added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Sep 19, 2016
@yoavf
Copy link
Contributor

yoavf commented Sep 19, 2016

LGTM 👍

@mattm mattm merged commit 5df52c5 into master Sep 19, 2016
@mattm mattm deleted the add/post-status-tests branch September 19, 2016 21:20
describe( 'PostStatus', ( ) => {
let PostStatus;

before( ( ) => {
Copy link
Contributor

@aduth aduth Sep 26, 2016

Choose a reason for hiding this comment

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

No filler spaces in empty constructs (e.g., {}, [], fn()).

https://github.com/Automattic/wp-calypso/blob/master/docs/coding-guidelines/javascript.md#spacing

Will have to see if we can get an ESLint rule to enforce this.

There's several instances of this through the file.

let PostStatus;

before( ( ) => {
PostStatus = require( '..' ).PostStatus;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can import this at top-of-file now. You'd only have needed it if you had some other test helpers which were prerequisites to be defined before the component itself (e.g. fake DOM, mockery, etc). Should help clean things up a bit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants