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 `sillyLogs` option #8

Merged
merged 3 commits into from Jan 19, 2017

Conversation

Projects
None yet
3 participants
@edm00se
Copy link
Contributor

edm00se commented Jan 19, 2017

Provides for configuration of whether to dump the eslintConfig and prettierOptions to the console, defaults to false.

@edm00se

This comment has been minimized.

Copy link
Contributor

edm00se commented Jan 19, 2017

I could use a hand creating a properly formed test for index.test.js. I believe I've created a test to execute the format function correction and then inspect that console.log was indeed called, though it seems to be failing at the check for .toHaveBeenCalledTimes(). It's my first time using jest, which looks pretty awesome, but I'm not 100% sure what I'm screwing up at this point.

test failure output


  ● console receives output of both eslintConfig and prettierOptions when sillyLogs is set

    expect(jest.fn())[.not].toHaveBeenCalledTimes()

    jest.fn() value must be a mock function or spy.
    Received:
      function: [Function bound log]

      at Object.<anonymous> (src/index.test.js:122:23)

 PASS  src/utils.test.js
----------|----------|----------|----------|----------|----------------|
File      |  % Stmts | % Branch |  % Funcs |  % Lines |Uncovered Lines |
----------|----------|----------|----------|----------|----------------|
All files |      100 |      100 |      100 |      100 |                |
 index.js |      100 |      100 |      100 |      100 |                |
 utils.js |      100 |      100 |      100 |      100 |                |
----------|----------|----------|----------|----------|----------------|

Test Suites: 1 failed, 1 passed, 2 total
Tests:       1 failed, 16 passed, 17 total
Snapshots:   0 total
Time:        3.406s
Ran all test suites.
The script called "test" which runs "jest --coverage" failed with exit code 1 https://github.com/kentcdodds/p-s/blob/v3.0.3/other/ERRORS_AND_WARNINGS.md#failed-with-exit-code
npm ERR! Test failed.  See above for more details.

@edm00se edm00se referenced this pull request Jan 19, 2017

Closed

Add sillyLogs #7

feat(options): add `sillyLogs` option
Provides for configuration of whether to dump the eslintConfig and prettierOptions to the console,
defaults to false.

Test is still WIP.

refs #7

@edm00se edm00se force-pushed the edm00se:feat/silly-logs-option branch from d988e8d to 5a92d88 Jan 19, 2017

@kentcdodds
Copy link
Member

kentcdodds left a comment

Looks awesome! Very close! May also try this in your assertion: http://facebook.github.io/jest/docs/api.html#asymmetric-jest-matchers

format.options.sillyLogs = true

format({text: ''})
// TODO: fix this test, since it fails on the matcher toHaveBeenCalledTimes

This comment has been minimized.

@edm00se

edm00se Jan 19, 2017

Contributor

That makes a bit more sense, thanks. I'll try and get it smooth.

@edm00se

This comment has been minimized.

Copy link
Contributor

edm00se commented Jan 19, 2017

I'm apparently having trouble figuring out how to apply the asymmetric matcher. Conceptually, I think I should probably be checking that the run of format ran with the calls to console.log, etc., a la expect(format({text: ''})).toHaveBeenCalledWith(console.log, console.dir) which won't limit the number of calls. This doesn't seem to work as I typed it and I'm sure I'm missing something further.

@kentcdodds

This comment has been minimized.

Copy link
Member

kentcdodds commented Jan 19, 2017

It's more like:

expect(console.log).toHaveBeenCalledWith('the string')
expect(console.dir).toHaveBeenCalledWith(
  expect.objectContaining({ /* stuff */ }),
  null,
  true,
)

I hope that helps. I'm on mobile. Sorry I'm not more helpful!

@edm00se

This comment has been minimized.

Copy link
Contributor

edm00se commented Jan 19, 2017

...aaand I was overcomplicating it. It just seems I just had a different expectation. I'll update the PR and see what you think.

test(options): sillyLogs test now succeeds
The sillyLogs specific test is now more comprehensive, checking both for the specific console.log
and also that console.dir was generally called.

refs #7
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jan 19, 2017

Current coverage is 100% (diff: 100%)

Merging #8 into master will not change coverage

@@           master    #8   diff @@
===================================
  Files           2     2          
  Lines          58    62     +4   
  Methods         0     0          
  Messages        0     0          
  Branches        0     0          
===================================
+ Hits           58    62     +4   
  Misses          0     0          
  Partials        0     0          

Powered by Codecov. Last update e2fdfbf...e443daa

@kentcdodds
Copy link
Member

kentcdodds left a comment

Looks great! Just a few things. Thank you for working on it!

Also, don't forget to add yourself to the contributors table (see the contributing guidelines)

@@ -1,8 +1,9 @@

This comment has been minimized.

@kentcdodds

kentcdodds Jan 19, 2017

Member

Let's remove this.

This comment has been minimized.

@edm00se

edm00se Jan 19, 2017

Contributor

How'd that get in there? 🤔

}) {
const originalLogValue = options.disableLog
options.disableLog = disableLog
if (sillyLogs) {

This comment has been minimized.

@kentcdodds

kentcdodds Jan 19, 2017

Member

Could we put this in a function call?

format({text: ''})
// TODO: fix this test, since it fails on the matcher toHaveBeenCalledTimes
expect(console.log).toHaveBeenCalledWith('😜 logs for eslintConfig and prettierOptions:')
expect(console.dir).toHaveBeenCalled()

This comment has been minimized.

@kentcdodds

kentcdodds Jan 19, 2017

Member

Could we be explicit with what it was called with as well as only having been called once?

// expect(format({text: ''})).toHaveBeenCalledWith(console.log, console.dir)
format({text: ''})
// TODO: fix this test, since it fails on the matcher toHaveBeenCalledTimes
expect(console.log).toHaveBeenCalledWith('😜 logs for eslintConfig and prettierOptions:')

This comment has been minimized.

@kentcdodds

kentcdodds Jan 19, 2017

Member

Could we be explicit that this was only called once?

test(options): sillyLogs increased in specificity
The sillyLogs option specific test now has greater specifics regarding structure of assertions.

refs #7
@kentcdodds
Copy link
Member

kentcdodds left a comment

Perfect!

@kentcdodds kentcdodds merged commit 3922b02 into prettier:master Jan 19, 2017

3 checks passed

codecov/patch 100% of diff hit (target 100%)
Details
codecov/project 100% (+0.00%) compared to e2fdfbf
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@edm00se edm00se deleted the edm00se:feat/silly-logs-option branch Jan 19, 2017

@edm00se

This comment has been minimized.

Copy link
Contributor

edm00se commented Jan 19, 2017

Nuts, I forgot to run addContributor. Hmm, that's what I get for being distracted with scrubbing potatoes for stew in the middle of this. :-/

@kentcdodds

This comment has been minimized.

Copy link
Member

kentcdodds commented Jan 19, 2017

Oh, whoops. Could you open another PR for that?

edm00se added a commit to edm00se/prettier-eslint that referenced this pull request Jan 19, 2017

@edm00se edm00se referenced this pull request Jan 19, 2017

Merged

Updates Contributor Info #9

kentcdodds added a commit that referenced this pull request Jan 19, 2017

@edm00se edm00se changed the title WIP: add `sillyLogs` option add `sillyLogs` option Jan 22, 2017

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