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.

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

Print out warning in verbose & mini reporter when `--fail-fast` is enabled #1160

Merged
merged 1 commit into from Jan 7, 2017

Conversation

Projects
None yet
3 participants
@ThomasBem
Copy link
Contributor

ThomasBem commented Dec 21, 2016

Hi there,

This PR fixes #1134. If --fail-fast is enabled it will print out the following message at the end of the report for both mini and verbose.

--fail-fast is on. Any number of tests may have been skipped

Mini:
screen shot 2016-12-21 at 14 42 39

Verbose:
screen shot 2016-12-21 at 14 44 03

This works for now, but i´m not sure if the way I have done it is the best.

I have been thinking that maybe instead of going through all the results in run-status looking for a stat element that has failFastEnabled=true.

It might be better to send a event or something if the Runner is created with the fail-fast option enabled. Then run-status could pickup on this and just set:
this.failFastEnabled = true

Let me know what you guys think!

Also i appreciate the support in the issue section helping me out with this :)

@novemberborn

This comment has been minimized.

Copy link
Member

novemberborn commented Dec 24, 2016

You should pass the bail option when RunStatus is instantiated: https://github.com/avajs/ava/blob/033d4dcdcbdadbf665c740ff450c2a775a8373dc/api.js#L145:L149

It'd be nice though if this wouldn't report when it knows no other tests didn't run. Which brings us back to the test counting logic.

@ThomasBem

This comment has been minimized.

Copy link
Contributor Author

ThomasBem commented Dec 24, 2016

Thats a much better solution than what i did. Thanks, I'll try and make that change.

You mentioned in the issue that it was probably hard or at least not a guarantee that we could determine the total amount of test to be run by the time failure occurred? So probably out of my league to fix that at this time.

But maybe open that as a separate issue / feature request for after we can get this initial functionality in place?

@ThomasBem ThomasBem force-pushed the ThomasBem:fail-fast branch from dae7fa7 to e542d20 Jan 4, 2017

@ThomasBem

This comment has been minimized.

Copy link
Contributor Author

ThomasBem commented Jan 4, 2017

Updated PR to reflect change suggested by @novemberborn. FailFast is now passed into RunStatus.

api.js Outdated
@@ -145,7 +145,8 @@ Api.prototype._run = function (files, options) {
var runStatus = new RunStatus({
runOnlyExclusive: options.runOnlyExclusive,
prefixTitles: this.options.explicitTitles || files.length > 1,
base: path.relative('.', commonPathPrefix(files)) + path.sep
base: path.relative('.', commonPathPrefix(files)) + path.sep,
failFast: options.failFast

This comment has been minimized.

@sindresorhus

sindresorhus Jan 5, 2017

Member

Use this.options.failFast instead and you don't have to pass it on https://github.com/avajs/ava/pull/1160/files#diff-f8849ae9f0614ba9464fdd14c2d98dc0R163 as it's already passed in the API constructor.

@ThomasBem ThomasBem force-pushed the ThomasBem:fail-fast branch from e542d20 to 8e666d1 Jan 5, 2017

@ThomasBem ThomasBem force-pushed the ThomasBem:fail-fast branch from 8e666d1 to 32d35c9 Jan 5, 2017

@ThomasBem

This comment has been minimized.

Copy link
Contributor Author

ThomasBem commented Jan 5, 2017

Updated PR with your suggestions. Thanks for the assist 👍

@novemberborn

This comment has been minimized.

Copy link
Member

novemberborn commented Jan 6, 2017

It'd be nice though if this wouldn't report when it knows no other tests didn't run. Which brings us back to the test counting logic.

@sindresorhus any thoughts on this? Happy to land this PR as-is and then do this as a follow-up.

@sindresorhus sindresorhus changed the title [WIP] Print out warning in verbose and mini reporter when --fail-fast is en… Print out warning in verbose & mini reporter when `--fail-fast` is enabled Jan 7, 2017

@sindresorhus sindresorhus merged commit 09d23f5 into avajs:master Jan 7, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.06%) to 97.048%
Details
@sindresorhus

This comment has been minimized.

Copy link
Member

sindresorhus commented Jan 7, 2017

Looks great. Thanks for this excellent contribution @ThomasBem :)

@novemberborn Can you open that follow-up issue you talked about?

@novemberborn

This comment has been minimized.

Copy link
Member

novemberborn commented Jan 7, 2017

Thanks @ThomasBem!

Opened #1171 and #1172 for follow-up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.