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

Test: Introduce jest matchers for console object #4285

Merged
merged 2 commits into from Jan 5, 2018

Conversation

Projects
None yet
3 participants
@gziolo
Member

gziolo commented Jan 4, 2018

Description

Implements #4251, where @aduth raised the following:

In a few places we're asserting that console logging is called, which requires a bit of finesse to properly temporarily disable ESLint rules and reset mock calls (in order to avoid tripping the global unintended console logging assertions):

We should plan to refactor these into a custom Jest matcher.

Possible usage:

// expect( console ).toHaveLogged( [ level[, expectedMessage ] ] );
expect( console ).toHaveLogged( 'warn', 'Should not do that!' );
expect( console ).toHaveLogged( 'error', 'Cannot do that!' );

See: https://facebook.github.io/jest/docs/en/expect.html#expectextendmatchers

I implemented the following API:

expect( console ).toHaveWarnedWith( 'Should not do that!' );
expect( console ).toHaveErroredWith( 'Cannot do that!' );

I think we will need also toHaveWarned and toHaveErrored to replace other existing checks and README file for the API introduced. I will work on it next week unless someone wants to do it earlier. Feel free to contribute :)

How Has This Been Tested?

npm test + Travis run.

Types of changes

Test changes only, no production code updated.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows has proper inline documentation.
Show outdated Hide outdated test/unit/console/index.js Outdated
@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Jan 4, 2018

Member

I added more matchers and refactored all remaining occurrences listed in the description.

Member

gziolo commented Jan 4, 2018

I added more matchers and refactored all remaining occurrences listed in the description.

@ntwb

ntwb approved these changes Jan 5, 2018

Neat, LGTM 👍

@ntwb

This comment has been minimized.

Show comment
Hide comment
@ntwb

ntwb Jan 5, 2018

Member

p.s. I thought about adding some tests for each of the matchers, this is as far as I got adding tests in /test/unit/console/test/index.test.js before giving up, it's 9:30pm Friday night and the tests need to be called with some mock functions, I need to go and read about Jest and mocking functions:

describe( '.toHaveWarned', () => {
	it( 'passes when console.warn is given', () => {
		expect( console ).toHaveWarned();
	} );
} );

describe( '.toHaveWarnedWith', () => {
	it( 'passes when console.warn is given', () => {
		expect( console ).toHaveWarnedWith();
	} );
} );

describe( '.toHaveErrored', () => {
	it( 'passes when console.error is given', () => {
		expect( console ).toHaveErrored();
	} );
} );

describe( '.toHaveErroredWith', () => {
	it( 'passes when console.error is given', () => {
		expect( console ).toHaveErroredWith();
	} );
} );
Member

ntwb commented Jan 5, 2018

p.s. I thought about adding some tests for each of the matchers, this is as far as I got adding tests in /test/unit/console/test/index.test.js before giving up, it's 9:30pm Friday night and the tests need to be called with some mock functions, I need to go and read about Jest and mocking functions:

describe( '.toHaveWarned', () => {
	it( 'passes when console.warn is given', () => {
		expect( console ).toHaveWarned();
	} );
} );

describe( '.toHaveWarnedWith', () => {
	it( 'passes when console.warn is given', () => {
		expect( console ).toHaveWarnedWith();
	} );
} );

describe( '.toHaveErrored', () => {
	it( 'passes when console.error is given', () => {
		expect( console ).toHaveErrored();
	} );
} );

describe( '.toHaveErroredWith', () => {
	it( 'passes when console.error is given', () => {
		expect( console ).toHaveErroredWith();
	} );
} );
@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Jan 5, 2018

Member

Yes, tests would be useful. We can borrow some ideas from Jest repository: https://github.com/facebook/jest/blob/master/packages/expect/src/__tests__/spy_matchers.test.js

Member

gziolo commented Jan 5, 2018

Yes, tests would be useful. We can borrow some ideas from Jest repository: https://github.com/facebook/jest/blob/master/packages/expect/src/__tests__/spy_matchers.test.js

@aduth

aduth approved these changes Jan 5, 2018

@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Jan 5, 2018

Member

I will merge it as it is today, and open a follow-up PR with docs and tests on Monday, thanks for reviews 👍

Member

gziolo commented Jan 5, 2018

I will merge it as it is today, and open a follow-up PR with docs and tests on Monday, thanks for reviews 👍

@gziolo gziolo merged commit d1df878 into master Jan 5, 2018

3 checks passed

codecov/project 39.2% remains the same compared to 55c5a79
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@gziolo gziolo deleted the add/jest-matcher-console branch Jan 5, 2018

@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Jan 8, 2018

Member

I opened a follow-up in WordPress/packages repository: WordPress/packages#60. I think those matchers have much wider application.

Member

gziolo commented Jan 8, 2018

I opened a follow-up in WordPress/packages repository: WordPress/packages#60. I think those matchers have much wider application.

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