-
Notifications
You must be signed in to change notification settings - Fork 21
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
refactor: ➖ remove chalk
as dependency
#44
Conversation
@ValentinH , please review Some tests are failing, but could be an issue with tests, not sure. Please, check and feel free to update this PR as you need |
The tests are pretty trustworthy. It seems that only the cases that check the actual behavior of tests failed by failOnConsole are failing. |
as those test cases meant to fail, I guess, |
Unfortunately this isn't that easy. These tests are actually using Jest to test a Jest plugin. That's why we are using this "fixture" pattern. |
1 similar comment
Unfortunately this isn't that easy. These tests are actually using Jest to test a Jest plugin. That's why we are using this "fixture" pattern. |
ok, found issue, will try to debug |
index.js
Outdated
`Expected test not to call ${bold(`console.${methodName}()`)}.\n\n` + | ||
`Expected test not to call ${(`console.${methodName}()`)}.\n\n` + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was the main reason for test failing. Don't know why ¯_(ツ)_/¯
@ValentinH , If you could accept this functionality change - PR can be merged, otherwise some more digging should be done here. I've tried a couple options and can't figure out the root reason
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've reverted this and slightly adjusted the test to be more flexible on the string assertion.
index.js
Outdated
red: (str) => `\u001b[31m${str}\u001b[0m`, | ||
gray: (str) => `\u001b[90m${str}\u001b[0m`, | ||
white: (str) => `\u001b[37m${str}\u001b[0m`, | ||
bold: (str) => `\u001b[1m${str}\u001b[22m` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tests in chalk
repo which confirms current implementation is correct https://github.com/chalk/chalk/blob/f7b29ae8ef4fd2048e08aa361778d290ed10ce7a/test/chalk.js#L23C7-L23C7
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, after looking at it, I've changed the closing sequence to match what was used by Chalk. (from 0m to 39m)
Thanks for this contribution. It was released as https://github.com/ValentinH/jest-fail-on-console/releases/tag/v3.1.2 |
fixes: #43
Some docs: https://stackoverflow.com/questions/4842424/list-of-ansi-color-escape-sequences
Tested on Linux & Mac
Haven't tested on Windows
Simple test