Skip to content

remove color from test title in mini reporter #618

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Mar 10, 2016
Merged

remove color from test title in mini reporter #618

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 1 addition & 4 deletions lib/reporters/mini.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,19 +70,16 @@ MiniReporter.prototype.prefix = function (str) {
};

MiniReporter.prototype._test = function (test) {
var title;
var title = test.title;

if (test.todo) {
title = colors.todo('- ' + test.title);
this.todoCount++;
Copy link
Member Author

@sindresorhus sindresorhus Mar 8, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we manually count these in the reporter? Shouldn't it be provided by the API?

Copy link
Member

@novemberborn novemberborn Mar 8, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've wondered that too.

Copy link
Contributor

@vadimdemedes vadimdemedes Mar 8, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, that should be changed.

Copy link
Contributor

@jamestalmage jamestalmage Mar 8, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I recall, the API does track this, but it's not provided until the finish event. The mini reporter is the only reporter that cares about the live count.

Copy link
Member

@novemberborn novemberborn Mar 8, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't you keep the - prefix?

Copy link
Member Author

@sindresorhus sindresorhus Mar 8, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No point. The test is not run anyways, so it finishes faster than a human could see.

Copy link
Contributor

@vadimdemedes vadimdemedes Mar 8, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If a human can not see todo and skip tests, shouldn't we also remove color from there?

Copy link
Contributor

@vadimdemedes vadimdemedes Mar 8, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But wait, if the next test is slow, user theoretically can actually see skip/todo tests. So I guess we should keep the prefix and color.

Also, what about changing prefixes to these?

  • if todo test, prefix = To-do:
  • if skip test, prefix = Skipped:

Copy link
Member Author

@sindresorhus sindresorhus Mar 8, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If a human can not see todo and skip tests, shouldn't we also remove color from there?

That's what this PR does.

But wait, if the next test is slow, user theoretically can actually see skip/todo tests. So I guess we should keep the prefix and color.

How about we just not show them all together? So if a slow test is running, the last actual test is shown, not todo/skip.

Copy link
Contributor

@vadimdemedes vadimdemedes Mar 8, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what this PR does.

Oh my God, what an embarrassment, I got confused with the new "Files" view and read the wrong column in diff...

giphy 7

How about we just not show them all together? So if a slow test is running, the last actual test is shown, not todo/skip.

Even better.

Copy link
Member

@novemberborn novemberborn Mar 8, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about we just not show them all together? So if a slow test is running, the last actual test is shown, not todo/skip.

Yep!

} else if (test.skip) {
title = colors.skip('- ' + test.title);
this.skipCount++;
} else if (test.error) {
title = colors.error(test.title);
this.failCount++;
} else {
title = colors.pass(test.title);
this.passCount++;
}

Expand Down
4 changes: 2 additions & 2 deletions test/reporters/mini.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ test('skipped test', function (t) {

var expectedOutput = [
' ',
' ⠋ ' + chalk.yellow('- skipped'),
' ⠋ ' + chalk.yellow('skipped'),
'',
' ' + chalk.yellow('1 skipped')
].join('\n');
Expand All @@ -140,7 +140,7 @@ test('todo test', function (t) {

var expectedOutput = [
' ',
' ⠋ ' + chalk.blue('- todo'),
' ⠋ ' + chalk.blue('todo'),
'',
' ' + chalk.blue('1 todo')
].join('\n');
Expand Down