Add busy indicator to mini reporter #599
Conversation
|
By analyzing the blame information on this pull request, we identified @sotojuan, @naptowncode and @ivogabe to be potential reviewers |
|
I ran all the visual tests and they all look good on iTerm2, OSX El Capitan. |
|
This is awesome, thanks! |
lib/reporters/mini.js
Outdated
| }; | ||
|
|
||
| MiniReporter.prototype.clearInterval = function () { | ||
| if (this.interval) { |
There was a problem hiding this comment.
Is there really any point in guarding this? clearInterval gracefully accepts anything.
|
Now the test title and the total count (71 passed) is vertically unaligned. They should be aligned. |
lib/reporters/mini.js
Outdated
| this.currentStatus = ''; | ||
| this.currentTest = ''; | ||
| this.statusLineCount = 0; | ||
| this.spinnerFrame = 0; |
There was a problem hiding this comment.
spinnerIndex would be a better name. When it says spinnerFrame I thought it was the actual frame.
|
While you're at it, can you put a linebreak above the test title to give it some breathing-room? |
lib/reporters/mini.js
Outdated
| if (this.interval) { | ||
| clearInterval(this.interval); | ||
| } | ||
| this.interval = 0; |
There was a problem hiding this comment.
Reset to null since Node uses object refs?
|
Should pass / fail count be on the same line? A:
vs. B:
|
|
OK, Also, I switched it to option A as described above (counts on separate lines). That is the last commit, so it can be dropped if there is not agreement. |
verifies behavior when there are some passing / some failing
jamestalmage
force-pushed
the
busy-indicator
branch
from
264a493 to
b32c813
Compare
|
One more minor tweak. Move the spinner one char to the right without moving anything else. It's a bit too far from the test title and a bit too much crammed into the left edge.
|
|
I'm good with |
lib/reporters/mini.js
Outdated
| status += ' ' + colors.error(this.failCount, 'failed'); | ||
| if (this.passCount > 0) { | ||
| status += '\n'; | ||
| } |
There was a problem hiding this comment.
Wouldn't you need to do this for skipped and todo tests results too?
There was a problem hiding this comment.
Good call on todo. I implemented this before rebasing, so yeah - that almost certainly needs a fix.
Are skipped tests shown yet? I thought there was still a pending PR for that.
There was a problem hiding this comment.
Oh - and I also missed everything in final
|
I'm also ok with A (assuming all other counters will be updated too). |
|
All changes made. |
| self.spinnerIndex = (self.spinnerIndex + 1) % self.spinnerFrames.length; | ||
| self.write(self.prefix()); | ||
| }, this.spinnerInterval); | ||
| return this.prefix(''); |
There was a problem hiding this comment.
The '' seems unnecessary here.
|
Cool! |
sindresorhus
changed the title
|
The default reporter looks so beautiful now 😍. Thanks James :) |
6 participants
Add a spinner to the mini reporter to indicate ongoing test progress.
Fixes #598.