-
-
Notifications
You must be signed in to change notification settings - Fork 93
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
feat($cli): Add support for --no-scripts #150
Conversation
src/index.test.js
Outdated
scripts: ['test'], | ||
options, | ||
}).then(() => { | ||
// expect(stdout).toEqual(...) |
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.
@kentcdodds Could you advise on how you would capture stdout to test this? Currently no precedent in tests (that I've seen).
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.
With looking at the code, I think we could use the infoSpy
from setup()
.
const {runPackageScript, infoSpy} = setup()
// and in the test
expect(infoSpy.calledWith(...)) // or something familiar
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 @tdeschryver will try this tonight.
This is great! Thanks for working on it! I'll try to get to it as soon as I can (at a conference right now). |
src/index.test.js
Outdated
scripts: ['test'], | ||
options, | ||
}).then(() => { | ||
expect(infoSpy.firstCall.args[0]).toMatch( |
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.
@tdeschryver @kentcdodds Let me know if this is acceptable for a test case.
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.
Looking awesome!
Added some notes. Also, could you add some docs to the README?
src/index.test.js
Outdated
scripts: ['test'], | ||
options, | ||
}).then(() => { | ||
expect(infoSpy.firstCall.args[0]).toMatch( |
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.
Why don't we do toMatchSnapshot
instead? Will be easier to update in the future.
src/index.test.js
Outdated
options, | ||
}).then(() => { | ||
expect(infoSpy.firstCall.args[0]).toMatch( | ||
'\u001b[90mnps is executing\u001b[39m `\u001b[1mtest\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.
Let's do toMatchSnapshot
here as well.
Codecov Report
@@ Coverage Diff @@
## master #150 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 11 11
Lines 345 346 +1
=====================================
+ Hits 345 346 +1
Continue to review full report at Codecov.
|
…output Running a script will by default output the command text of the script to stdout, passing --no-scripts will hide the command text. sezna#143
@kentcdodds Done!
|
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.
Awesome. Thanks!
Thank you so much! |
Awesome! Can't wait to use it. Next time I'll try my hand at my own feature request. 👍 |
#143
What: Running a script will by default output the command text of the script to stdout, passing
--no-scripts will hide the command text.
Why: Some script commands are long and clutter console output when logged.
How: Added
{ scripts: ... }
to options. Defaultstrue
, pass--no-scripts
sets tofalse
.