Print out warning in verbose & mini reporter when .only() tests are used #1177

ThomasBem · 2017-01-11T08:43:53Z

This PR fixes #1135. If all test files have at least one .only test in them, it will print out the following message at the end of the report for both mini and verbose.

The .only() modifier is used in some tests. All other tests have been skipped.

If there are test files without .only in them, it will print out the following.

The .only() modifier is used in some tests. At least', remainingTests, 'tests were not run.

remainingTests will then be the number of tests in the test files that don´t have any .only tests.

It currently looks like this:

Feedback appreciated :)

mightyiam · 2017-01-12T09:08:41Z

If there are test files without .only in them, it will print out the following.

The .only() modifier is used in some tests. At least', remainingTests, 'tests were not run.

Are you sure you got this part correct? I don't see the sense in it.

ThomasBem · 2017-01-12T10:30:07Z

You mean you think the implementation is wrong? Or do you not think there is any value in showing the remaining count?

What is happening in the case of the screenshots is that there are two test files being run, test.js and test2.js. There is one .only() test in test.js, and none in test2.js.

So now you are at least notified of all the tests in test2.js that has not been run because of the .only() tests.

If there was at least one .only in test2.js also. It would just report "all other tests have been skipped".

mightyiam · 2017-01-12T10:37:11Z

I get it now. The first quote is the current message (result of your previous PR). I did not understand this, at first.

BTW, why do you put quotes around the -v?

mightyiam

Shallow review, according to my unfamiliarity with the code.

mightyiam · 2017-01-12T10:38:43Z

lib/reporters/mini.js

+	if (runStatus.hasExclusive === true) {
+		var remainingTests = runStatus.testCount - (runStatus.passCount + runStatus.failCount);
+		if (remainingTests > 0) {
+			status += '\n\n  ' + colors.information('The .only() modifier is used in some tests. At least', remainingTests, 'tests were not run.');


Use native JavaScript template string instead of some library's API, I think.

Meh, it still needs to be wrapped with colors.information(), so this combination seems fine to me.

Previously we were targeting Node.js 0.10, which meant that we couldn't use template strings. Now we can, but it's no big deal if new code follows the pattern already used in the file.

mightyiam · 2017-01-12T10:39:20Z

lib/reporters/mini.js

+		if (remainingTests > 0) {
+			status += '\n\n  ' + colors.information('The .only() modifier is used in some tests. At least', remainingTests, 'tests were not run.');
+		} else {
+			status += '\n\n  ' + colors.information('The .only() modifier is used in some tests. All other tests have been skipped.');


DRY first sentence.

mightyiam · 2017-01-12T10:40:27Z

lib/reporters/verbose.js

+	if (runStatus.hasExclusive === true) {
+		var remainingTests = runStatus.testCount - (runStatus.passCount + runStatus.failCount);
+		if (remainingTests > 0) {
+			output += '\n\n\n  ' + colors.information('The .only() modifier is used in some tests. At least', remainingTests, 'tests were not run.');


You have some shared strings here. Shared with the mini reporter.

And template string.

You have some shared strings here. Shared with the mini reporter.

I find that to be the kind of repetition that is too bothersome to dry up.

mightyiam · 2017-01-12T10:40:38Z

lib/reporters/verbose.js

+		if (remainingTests > 0) {
+			output += '\n\n\n  ' + colors.information('The .only() modifier is used in some tests. At least', remainingTests, 'tests were not run.');
+		} else {
+			output += '\n\n\n  ' + colors.information('The .only() modifier is used in some tests. All other tests have been skipped.');


Shared string.

ThomasBem · 2017-01-12T14:15:48Z

Thanks for the review! 👍

I am not familiar with template strings, but I think the implementation I have done with regards to string formatting is in-line / same styles as what was already in place in the reporters.

I personally don´t think there is a problem with having the same output strings repeated across the two different reporters. They are two separate pieces that in theory could output different things. It also makes it easy to read what the reporter outputs without having to check some global configuration property.

But I am up for making changes if more people think its a better solution :)

@mightyiam Could you provide an example of how my two lines of output would look using template strings?

novemberborn

This is great!

Please check in with @jarlehansen regarding the remainingCount computation (see inline comment).

novemberborn · 2017-01-12T16:33:21Z

lib/reporters/mini.js

+	if (runStatus.hasExclusive === true) {
+		var remainingTests = runStatus.testCount - (runStatus.passCount + runStatus.failCount);
+		if (remainingTests > 0) {
+			status += '\n\n  ' + colors.information('The .only() modifier is used in some tests. At least', remainingTests, 'tests were not run.');


Meh, it still needs to be wrapped with colors.information(), so this combination seems fine to me.

Previously we were targeting Node.js 0.10, which meant that we couldn't use template strings. Now we can, but it's no big deal if new code follows the pattern already used in the file.

novemberborn · 2017-01-12T16:34:24Z

lib/reporters/verbose.js

+	if (runStatus.hasExclusive === true) {
+		var remainingTests = runStatus.testCount - (runStatus.passCount + runStatus.failCount);
+		if (remainingTests > 0) {
+			output += '\n\n\n  ' + colors.information('The .only() modifier is used in some tests. At least', remainingTests, 'tests were not run.');


You have some shared strings here. Shared with the mini reporter.

I find that to be the kind of repetition that is too bothersome to dry up.

novemberborn · 2017-01-12T16:37:31Z

lib/reporters/mini.js

+	}
+
+	if (runStatus.hasExclusive === true) {
+		var remainingTests = runStatus.testCount - (runStatus.passCount + runStatus.failCount);


This should be computed in lib/run-status.js, which @jarlehansen is working on in #1179. Note that as I commented there (#1179 (comment)) you need to include other test states in this count, see the highlighted lines in https://github.com/avajs/ava/blob/033d4dcdcbdadbf665c740ff450c2a775a8373dc/lib/run-status.js#L41:L45.

Will sync up with @jarlehansen regarding this and update PR.

ThomasBem · 2017-01-12T18:08:33Z

@novemberborn updated PR with the changes to remainingCount in run-status.js and added a test for it like you suggested to @jarlehansen in his PR. Had a quick chat with him and he would get on later today to take a look.

So you guys can review my suggestion and see what you think :)

jarlehansen · 2017-01-12T18:31:15Z

Thanks @ThomasBem, this improves on what I created in my pull request. When this is merged I will update #1179

novemberborn · 2017-01-13T11:26:50Z

@ThomasBem @jarlehansen thanks for your collaboration 💯

I found one edge case. Tests in the file with .only() are not counted. This is because the test count is based on the scheduled tests (main.js), and non-exclusive tests are discarded (test-collection.js). This is why, before this PR, testCount was reset (run-status.js).

I think we should count all tests in test-collection.js and use that value in main.js.

ThomasBem · 2017-01-13T11:46:16Z

Yeah, I knew about that behaviour and I tried explaining it in my original PR text. But guess I did not get that through properly, or at least i should have mentioned it more specific :)

If all test files have at least one .only test in them, it will print out the following message at the end of the report for both mini and verbose.

The .only() modifier is used in some tests. All other tests have been skipped.

Thats the reason its split into two. Because when .only is present it did not count all the other tests.

Did not feel comfortable with changing some of the deeper behaviour of how tests where added and removed from collections.

But if you think its a good idea to change the behaviour to count all the tests even if there is .only tests present. Then i guess we can simplify the logic you linked to in test-collection.js similar to what was done run-status.js regarding exclusive?

novemberborn · 2017-01-13T12:26:48Z

Did not feel comfortable with changing some of the deeper behaviour of how tests where added and removed from collections.

But if you think its a good idea to change the behaviour to count all the tests even if there is .only tests present. Then i guess we can simplify the logic you linked to in test-collection.js similar to what was done run-status.js regarding exclusive?

Fair enough. But yes, let's change it. We should keep the counting consistent, regardless of whether .only was encountered in the test file being collected, or in a later test file.

ThomasBem · 2017-01-13T20:42:37Z

@novemberborn I got some results that confused me a bit when trying to fix some failing tests after implementing what I think would be a solution for test-collection.js.

Then I read this in the description of one of the api.js tests that was failing,

ava/test/api.js

Line 22 in ce42fcb

    
           test('Without Pool: test file with exclusive tests causes non-exclusive tests in other files to be ignored', t => {

Is this correct? That it will still run tests that are NOT tagged with .only() if they are in a file that has at least one .only() test?

I am still reading up on the code and trying to see if its just me that´s missing something obvious and thats why things are acting seemingly a bit weird. But figured I would at least ask for a clarification regarding this one thing :)

ThomasBem · 2017-01-13T21:20:20Z

Think I figured out what i was doing wrong on my own :) Was just doing things way too complicated...

But this wording is still somewhat confusing to me. Because it does not run any other tests than the .only() as far as i can tell?

ThomasBem · 2017-01-13T22:52:38Z

@novemberborn PR updated with requested changes. It now properly counts all tests, exclusive or not. Also made some changes to the reporters output to be more in line with #1179 that @jarlehansen is working on.

Now it only reports on .only() / exclusive if there are tests that have not been run.

Any thoughts on the output text?

The .only() modifier is used in some tests. 1 test(s) were not run.

A bit clumsy with the "test(s)", but i couldn't think of anything better to cover 1 or multiple tests remaining.

mightyiam · 2017-01-14T11:21:32Z

"1 tests were not run" is fine with me.

novemberborn · 2017-01-14T14:09:01Z

@ThomasBem

… I read this in the description of one of the api.js tests that was failing,

ava/test/api.js

Line 22 in ce42fcb

test('Without Pool: test file with exclusive tests causes non-exclusive tests in other files to be ignored', t => {

Is this correct? That it will still run tests that are NOT tagged with .only() if they are in a file that has at least one .only() test?

… this wording is still somewhat confusing to me. Because it does not run any other tests than the .only() as far as i can tell?

That's not how I read it. It's meant to say that if there are two test files, and only one contains exclusive tests, then none of the tests from the other file are run.

novemberborn

Awesome, so very nearly there 👍

novemberborn · 2017-01-14T14:11:15Z

lib/reporters/mini.js

+	}
+
+	if (runStatus.hasExclusive === true && runStatus.remainingCount > 0) {
+		status += '\n\n  ' + colors.information('The .only() modifier is used in some tests.', runStatus.remainingCount, 'test(s) were not run.');


Use plur(), so "1 test was" and "2 tests were".

ThomasBem · 2017-01-14T15:50:41Z

PR updated again :)

novemberborn · 2017-01-15T13:13:56Z

Great work @ThomasBem, keep it up 👍

ThomasBem · 2017-01-15T13:23:01Z

Thanks for the support! 👍

novemberborn · 2017-01-16T09:09:30Z

lib/reporters/mini.js

+	}
+
+	if (runStatus.hasExclusive === true && runStatus.remainingCount > 0) {
+		status += '\n\n  ' + colors.information('The .only() modifier is used in some tests.', runStatus.remainingCount, plur('test', runStatus.remainingCount), plur('was', 'were', runStatus.remainingCount), 'not run.');


@ThomasBem this is very petty, but I just noticed there's a period at the end of the message here (and in the verbose reporter). None of the other messages seem to end in a period. If you've got the time could you do a PR to remove them? 😄

@novemberborn Sure, I can fix that :)

mightyiam suggested changes Jan 12, 2017

View reviewed changes

novemberborn requested changes Jan 12, 2017

View reviewed changes

ThomasBem force-pushed the notify-on-exclusive branch from 15f477f to 63fc44a Compare January 12, 2017 17:57

jarlehansen mentioned this pull request Jan 12, 2017

Only print fail-fast when there are remaining tests #1179

Merged

ThomasBem force-pushed the notify-on-exclusive branch from 63fc44a to 23ffce3 Compare January 13, 2017 22:44

novemberborn requested changes Jan 14, 2017

View reviewed changes

Print out warning in verbose & mini reporter when .only() tests are used

a2b5608

ThomasBem force-pushed the notify-on-exclusive branch from 23ffce3 to a2b5608 Compare January 14, 2017 15:30

novemberborn changed the title ~~[WIP] Print out warning in verbose & mini reporter when .only() tests are used~~ Print out warning in verbose & mini reporter when .only() tests are used Jan 15, 2017

novemberborn approved these changes Jan 15, 2017

View reviewed changes

novemberborn merged commit 22a6081 into avajs:master Jan 15, 2017

novemberborn reviewed Jan 16, 2017

View reviewed changes

ThomasBem mentioned this pull request Jan 16, 2017

Removed period at end of only()/exclusive output in mini and verbose reporters #1189

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Print out warning in verbose & mini reporter when .only() tests are used #1177

Print out warning in verbose & mini reporter when .only() tests are used #1177

ThomasBem commented Jan 11, 2017

mightyiam commented Jan 12, 2017

ThomasBem commented Jan 12, 2017

mightyiam commented Jan 12, 2017

mightyiam left a comment

mightyiam Jan 12, 2017

novemberborn Jan 12, 2017

mightyiam Jan 12, 2017

mightyiam Jan 12, 2017

mightyiam Jan 12, 2017

novemberborn Jan 12, 2017

mightyiam Jan 12, 2017

ThomasBem commented Jan 12, 2017

novemberborn left a comment

novemberborn Jan 12, 2017

novemberborn Jan 12, 2017

novemberborn Jan 12, 2017

ThomasBem Jan 12, 2017

ThomasBem commented Jan 12, 2017

jarlehansen commented Jan 12, 2017

novemberborn commented Jan 13, 2017

ThomasBem commented Jan 13, 2017

novemberborn commented Jan 13, 2017

ThomasBem commented Jan 13, 2017

ThomasBem commented Jan 13, 2017

ThomasBem commented Jan 13, 2017

mightyiam commented Jan 14, 2017

novemberborn commented Jan 14, 2017

novemberborn left a comment

novemberborn Jan 14, 2017

ThomasBem commented Jan 14, 2017

novemberborn commented Jan 15, 2017

ThomasBem commented Jan 15, 2017

novemberborn Jan 16, 2017

ThomasBem Jan 16, 2017

Print out warning in verbose & mini reporter when .only() tests are used #1177

Print out warning in verbose & mini reporter when .only() tests are used #1177

Conversation

ThomasBem commented Jan 11, 2017

mightyiam commented Jan 12, 2017

ThomasBem commented Jan 12, 2017

mightyiam commented Jan 12, 2017

mightyiam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThomasBem commented Jan 12, 2017

novemberborn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThomasBem commented Jan 12, 2017

jarlehansen commented Jan 12, 2017

novemberborn commented Jan 13, 2017

ThomasBem commented Jan 13, 2017

novemberborn commented Jan 13, 2017

ThomasBem commented Jan 13, 2017

ThomasBem commented Jan 13, 2017

ThomasBem commented Jan 13, 2017

mightyiam commented Jan 14, 2017

novemberborn commented Jan 14, 2017

novemberborn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThomasBem commented Jan 14, 2017

novemberborn commented Jan 15, 2017

ThomasBem commented Jan 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment