Skip to content
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

Jump to bottom

Show how many tests were skipped by --fail-fast #1221

Merged
merged 1 commit into from
Feb 6, 2017
Merged

Show how many tests were skipped by --fail-fast #1221

merged 1 commit into from
Feb 6, 2017

Conversation

sotojuan
Copy link
Contributor

@sotojuan sotojuan commented Feb 3, 2017

I'm back @avajs/core!

Closes #1172.

Let me know if you want to change the text.

novemberborn
novemberborn requested changes Feb 5, 2017
View reviewed changes
Copy link
Member

@novemberborn novemberborn left a comment

Choose a reason for hiding this comment

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

Welcome back!

lib/reporters/mini.js Outdated
@@ -233,7 +233,8 @@ class MiniReporter {
}

if (runStatus.failFastEnabled === true && runStatus.remainingCount > 0 && runStatus.failCount > 0) {
status += '\n\n ' + colors.information('`--fail-fast` is on. Any number of tests may have been skipped');
const remaining = runStatus.remainingCount + ' skipped ' + plur('test', runStatus.remainingCount) + '.';
Copy link
Member

Choose a reason for hiding this comment

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

We don't necessarily know how many tests would have executed when running in --concurrency mode. So to be accurate this should be At least 1 test was skipped..

test/reporters/mini.js Outdated
@@ -643,7 +643,7 @@ test('results when fail-fast is enabled', t => {
compareLineOutput(t, output, [
'',
'',
' ' + colors.information('`--fail-fast` is on. Any number of tests may have been skipped')
' ' + colors.information('`--fail-fast` is on. 1 skipped test.')
Copy link
Member

Choose a reason for hiding this comment

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

Should have a test for multiple skipped tests.

@sotojuan
Copy link
Contributor Author

sotojuan commented Feb 6, 2017

I've updated the text to At least 2 tests skipped. plur doesn't handle was vs were so I decided to omit it unless it's fine for me to make a small function in both reporters to handle that. More tests added too.

@novemberborn
Copy link
Member

plur doesn't handle was vs were so I decided to omit it unless it's fine for me to make a small function in both reporters to handle that.

I think the current text is fine…? Second-guessing my language skills now.

You can use plur('test was', 'tests were', runStatus.remainingCount) by the way, no need for an extra function.

@sotojuan
Copy link
Contributor Author

sotojuan commented Feb 6, 2017

All right, sorry about that. Text fixed!

@sotojuan sotojuan merged commit 6a8ed2c into avajs:master Feb 6, 2017
@sotojuan sotojuan deleted the fail-fast-remainder branch February 6, 2017 15:48
@renovate-bot renovate-bot mentioned this pull request Feb 18, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@novemberborn novemberborn novemberborn approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@sotojuan @novemberborn