-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Improve watch logging #737
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
Conversation
By analyzing the blame information on this pull request, we identified @BarryThePenguin, @sotojuan and @naptowncode to be potential reviewers |
var tapReporter = tap(); | ||
var logger = new Logger(tapReporter); | ||
tapReporter.clear = undefined; | ||
logger.write = t.fail; |
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.
Are we certain t.fail
has it's this
value bound in node-tap
?
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.
Needs a rebase, but LGTM |
5de9f39
to
196b643
Compare
Rebased. |
@sindresorhus I'm not sure newlines will help. Just pushed a commit that inserts a horizontal sectioning line. No tests yet. Could you have play with that? |
@novemberborn Perfect. Much clearer separation now. |
@@ -3,6 +3,7 @@ var prettyMs = require('pretty-ms'); | |||
var figures = require('figures'); | |||
var chalk = require('chalk'); | |||
var plur = require('plur'); | |||
var repeatString = require('repeat-string'); |
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.
Can you use https://github.com/sindresorhus/repeating? We already depend on modules using it, so it would mean no extra dependency.
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.
Ha! I knew you'd written one! It didn't show up when searching for string repeat 😄
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.
The npm search sucks... Try http://node-modules.com.
Tests are failing. |
Yea, probably due to the wip commit. Will patch that up tomorrow. |
I'm using toLocaleTimeString() which works even back to Node.js 0.10. Forcing a 24-hour clock because we're geeks.
The CLI starts the logger so the watcher shouldn't reset it on its first run. Restart the logger after resetting, this allows the mini reporter to render its spinner.
Return an object for the runStatus, add assertions to verify this object is passed to logger.finish()
Clear the mini reporter unless the previous run had failures, or "r\n" was entered on stdin.
Always print two empty lines before each error/rejection/exception when the mini and verbose reporters finish. Remove trailing whitespace from stack traces. Always print an empty line after the finish output. Add a test helper to more easily compare line output, with useful debug information. At the moment this new helper is only used for failing tests.
118b8e0
to
08cd7ed
Compare
Updated! I changed the This means that if the horizontal sectioning line is printed there will always be a single empty line before it, and a single empty line after it. Oh so very tidy! 😜 |
Watch mode won't clear the mini reporter if there were errors, or "r\n" was entered on stdin. The verbose reporter can't be cleared at all. To improve the separation between multiple test runs, write a horizontal line when starting a new test run and the reporter was not cleared.
08cd7ed
to
f2cb3bd
Compare
👍 The output looks really slick now! |
Thanks for all your hard work on this, what a pleasant surprise to see in the console this morning. Looks great 👍 |
Fixes #670.
r\n
was entered on stdin (because that inserts a line which can't be cleared)Also includes some test improvements.