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

Updates tests. #946

Merged
merged 2 commits into from
Jun 1, 2017
Merged

Conversation

BE-Webdesign
Copy link
Contributor

Closes #938. The other PRs have been updated for better conventions.
Here is an update to the tests already existing in the repo. Updates
FormToggle, Dashicon, and Button.

Testing Instructions
Verify tests still pass.

@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 May 30, 2017
const formToggle = shallow( <FormToggle checked /> );
expect( formToggle.hasClass( 'is-checked' ) ).to.be.true();
expect( formToggle.find( '.components-form-toggle__input' ).prop( 'value' ) ).to.be.true();
expect( formToggle.find( '.components-form-toggle__hint' ).text() ).to.equal( 'On' );
} );

it( 'with additonal className', () => {
it( 'shouuld render with an additional className', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Typo: shouuld &rarr: should

it( 'should render and additional WordPress prop of value awesome', () => {
const button = <Button WordPress="awesome" />;

expect( button.props.WordPress ).to.equal( 'awesome' );
Copy link
Member

Choose a reason for hiding this comment

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

Should this be button.prop( 'WordPress' )?

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 don't think so. I admit it is probably confusing. This is just pure React, I don't believe Enzyme is touching this in anyway. I believe it would need to be wrapped in shallow for the API to be available. I should probably just wrap it in shallow and do what you said.

const formToggle = shallow( <FormToggle id="test" /> );
expect( formToggle.find( '.components-form-toggle__input' ).prop( 'id' ) ).to.equal( 'test' );
} );

it( 'with onChange handler', () => {
it( 'should render a checkbox with a noop on change and if an onChange function is provided override the noop', () => {
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 this should be two separate tests:

should render a checkbox with a noop onChange
should render a checkbox with a user-provided onChange

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good.

Copy link
Member

@nylen nylen left a comment

Choose a reason for hiding this comment

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

Feel free to merge after addressing inline notes. 👍

@aduth
Copy link
Member

aduth commented May 31, 2017

I can probably be partly to blame for perpetuating the "should" convention for starting test case descriptions. I don't feel so strongly about it these days. I think a more reasonable convention, especially with the method name, is to simply form it as a complete sentence, which could be it( 'should render ..., but could equally be it( 'renders ....

Two cents. Dunno how others feel.

@nylen
Copy link
Member

nylen commented May 31, 2017

My preference is simply that the test names form a complete and readable sentence as well. I usually write should just out of habit and adhering to established citation needed BDD conventions.

Closes #938.  The other PRs have been updated for better conventions.
Here is an update to the tests already existing in the repo.  Updates
FormToggle, Dashicon, and Button.

**Testing Instructions**
Verify tests still pass.
Adding revisions to the tests.  Cleaning up Enzyme API usage, and
breaking larger tests into more test cases.  Spelling corrections added.
@BE-Webdesign BE-Webdesign force-pushed the update/test/components/new-conventions branch from 0075194 to c56aa87 Compare June 1, 2017 16:01
@BE-Webdesign BE-Webdesign merged commit eb929e5 into master Jun 1, 2017
@aduth
Copy link
Member

aduth commented Jun 1, 2017

I usually write should just out of habit and adhering to established citation needed BDD conventions.

I blame Mocha's docs 😛 https://mochajs.org/#getting-started

@BE-Webdesign BE-Webdesign deleted the update/test/components/new-conventions branch June 1, 2017 21:54
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