unittest: A unittest with 0 test cases should PASS, fixed incorrect unittest-suite-* messages

unittest: A unittest with 0 test cases should PASS, fixed incorrect unittest-suite-* messages

[Jennifer Messerly, 2013-11-18 21:32:39]

+Bob, he might have more context.

IIRC, we've had bugs before where tests fail to get registered, resulting in an
apparent "PASS" but no tests were run. This change seems to make that worse. I'm
curious what is the motivation for the change?

[Bob Nystrom, 2013-11-18 21:44:19]

The current behavior is by design. If you have a file with no tests, it hasn't
validated anything about your program, so it hasn't "passed" anything. It almost
invariably indicates a bug in your test code itself. Silently passing in that
case will mask a very real problem in their code.

[blois, 2013-11-18 21:56:33]

On 2013/11/18 21:44:19, Bob Nystrom wrote:
> The current behavior is by design. If you have a file with no tests, it hasn't
> validated anything about your program, so it hasn't "passed" anything. It
almost
> invariably indicates a bug in your test code itself. Silently passing in that
> case will mask a very real problem in their code.

Emily is correct- we have cases where we skip tests which are not supported,
relying on runtime feature detection to do so (not the status file). This
results in empty test configurations. This should only really happen for
html_individual_config, so we could either change the behavior there or inject a
dummy test (prefer the former).

One of the refactorings that Siggi was looking at making was integrating the
status file into the test file, I assume this could allow skipping individual
tests and could result in the same zero-tests-pass issue.

[Bob Nystrom, 2013-11-18 22:02:53]

On 2013/11/18 21:56:33, blois wrote:
> On 2013/11/18 21:44:19, Bob Nystrom wrote:
> > The current behavior is by design. If you have a file with no tests, it
hasn't
> > validated anything about your program, so it hasn't "passed" anything. It
> almost
> > invariably indicates a bug in your test code itself. Silently passing in
that
> > case will mask a very real problem in their code.
> 
> Emily is correct- we have cases where we skip tests which are not supported,
> relying on runtime feature detection to do so (not the status file). This
> results in empty test configurations. This should only really happen for
> html_individual_config, so we could either change the behavior there or inject
a
> dummy test (prefer the former).
> 
> One of the refactorings that Siggi was looking at making was integrating the
> status file into the test file, I assume this could allow skipping individual
> tests and could result in the same zero-tests-pass issue.

Ah, I suspected something along these lines was in play here.

Maybe you could add a test in those cases that actually tests the feature
detection itself, like:

test("this does nothing on safari", () {
  expect(...);
});

That will ensure that you are disabling the tests for the reason you think you
are, and ensures there is at least one test in the suite.

Another option that we do in pub sometimes is to move the feature detection
inside the test itself. That way you can see that they still ran but they're
essentially no-ops.

I *don't* think we should make "no tests = OK" the default behavior. The use
case here is valid, but allowing this for all tests will mask a lot of scary
bugs.

- bob

[blois, 2013-11-19 00:47:57]

On 2013/11/18 22:02:53, Bob Nystrom wrote:
> On 2013/11/18 21:56:33, blois wrote:
> > On 2013/11/18 21:44:19, Bob Nystrom wrote:
> > > The current behavior is by design. If you have a file with no tests, it
> hasn't
> > > validated anything about your program, so it hasn't "passed" anything. It
> > almost
> > > invariably indicates a bug in your test code itself. Silently passing in
> that
> > > case will mask a very real problem in their code.
> > 
> > Emily is correct- we have cases where we skip tests which are not supported,
> > relying on runtime feature detection to do so (not the status file). This
> > results in empty test configurations. This should only really happen for
> > html_individual_config, so we could either change the behavior there or
inject
> a
> > dummy test (prefer the former).
> > 
> > One of the refactorings that Siggi was looking at making was integrating the
> > status file into the test file, I assume this could allow skipping
individual
> > tests and could result in the same zero-tests-pass issue.
> 
> Ah, I suspected something along these lines was in play here.
> 
> Maybe you could add a test in those cases that actually tests the feature
> detection itself, like:
> 
> test("this does nothing on safari", () {
>   expect(...);
> });
> 
> That will ensure that you are disabling the tests for the reason you think you
> are, and ensures there is at least one test in the suite.
> 
> Another option that we do in pub sometimes is to move the feature detection
> inside the test itself. That way you can see that they still ran but they're
> essentially no-ops.
> 
> I *don't* think we should make "no tests = OK" the default behavior. The use
> case here is valid, but allowing this for all tests will mask a lot of scary
> bugs.
> 
> - bob

Discussed a bit more- for now I'd like to see html_individual_config allow 'no
tests = OK'.

Ideally we can get the status integrated into the tests themselves, then we can
move away from individual config and have all our tests (supported et al) in the
same test run. Assuming that the check becomes passed > 0 || skipped > 0.

[kustermann, 2013-11-19 12:05:37]

My 2c/opinions:

The motivation for this CL was that there are html tests which enqueue 0 tests.
- When running these tests in the browser, you can see everything is green and
it says "All 0 tests passed" (or something like that)
- The "void onDone(bool success)" method on the configuration is call is called
with "success=false".

This is inconsistent!

Furthermore, the testing scripts will only get a "unittest-suite-done" message.
This doesn't allow us to determine whether a given test has failed or not.
Currently this issue is solved by grepping in the DOM for 'FAIL' and 'PASS' to
determine if we succeeded/failed (which is broken -- this is one of the (many)
things I'm trying to fix).

The solution is to print/post "unittest-suite-success"/"unittest-suite-fail".
But then the "0 tests" html tests will start failing!

Which brings us to:

I would expect every sane test framework to work as follows:
You give it a set of tests T. It will run all tests in T. If *any* of the test
runs fail, the outcome will be "Fail". Otherwise the outcome will be "Pass".

If you read that in the context of T being empty, it means the outcome is
"Pass".


> Bob:
> I *don't* think we should make "no tests = OK" the default behavior. 

IMHO there should be no "default behavior". There should be only *one* behavior.
Please pick either "no tests = OK" or "no tests = Fail".


> Bob:
> The use case here is valid, but allowing this for all tests will mask a lot of
scary bugs.

Where would this mask bugs? Do you mean in a test or in the unittest framework?
- The author of the test is responsible for correctly using the unittest
framework -- and I don't think it is that difficult to use (if it is difficult
to use, we should make it simpler).
- Making sure that the unittest framework is doing what it is supposed to do is
the job of "dart/pkg/unittest/test/*_test.dart".

> Pete:
> One of the refactorings that Siggi was looking at making was integrating the
> status file into the test file, I assume this could allow skipping individual
> tests and could result in the same zero-tests-pass issue.
> ...
> Ideally we can get the status integrated into the tests themselves, then we
can
> move away from individual config and have all our tests (supported et al) in
the
> same test run. Assuming that the check becomes passed > 0 || skipped > 0.

IMHO It is not a good idea (actually I think it's a bad idea) to encode status
file entries in the tests:
- You enqueue tests at runtime depending on some conditions: This makes the
tests hard to read, is error prone and it makes it hard to figure out what we're
actually testing.
- We get wrong confidence. If all status files are empty we think that all tests
are passing on all runtimes and we're in a *excellent shape*. But that is wrong
if we decide to skip some tests at runtime depending on browser features etc.

Here's one suggestion, how we could handle this: We could introduce another
"NotSupported" status file marker. It could look like this:

  [ $compiler == dart2js && $runtime == ie9 ]
  LibTest/typed_data/Float32List/*: NotSupported
  canvasrenderingcontext2d_test/*: NotSupported

This would make it crystal clear which tests are failing because they're not
supported.

I hope my criticism was not too harsh. These are just my opinions and you might
disagree or not. I'm happy to discuss this more if you'd like.

[Bob Nystrom, 2013-11-19 16:55:10]

On 2013/11/19 12:05:37, kustermann wrote:
> The motivation for this CL was that there are html tests which enqueue 0
tests.
> - When running these tests in the browser, you can see everything is green and
> it says "All 0 tests passed" (or something like that)
> - The "void onDone(bool success)" method on the configuration is call is
called
> with "success=false".
> 
> This is inconsistent!

Yeah, we should definitely be consistent here. That seems weird.

> I would expect every sane test framework to work as follows:
> You give it a set of tests T. It will run all tests in T. If *any* of the test
> runs fail, the outcome will be "Fail". Otherwise the outcome will be "Pass".
> 
> If you read that in the context of T being empty, it means the outcome is
> "Pass".

Earlier versions of the unittest package, and the previous Expect unit test API
*did* work like that. It masked a frighteningly large number of bugs in tests.

> IMHO there should be no "default behavior". There should be only *one*
behavior.
> Please pick either "no tests = OK" or "no tests = Fail".

The latter.

> Where would this mask bugs? Do you mean in a test or in the unittest
framework?

In tests. Bugs in tests are particularly bad because:

1. They don't have tests! We don't have tests that test our tests, so a bug
there is invisible.
2. Code reviewers don't review tests as thoroughly as they review code.
3. As soon as they see a passing test, people immediately stop worrying about
it. We investigate failing tests, but we aren't skeptical of passing ones.

> - The author of the test is responsible for correctly using the unittest
> framework -- and I don't think it is that difficult to use (if it is difficult
> to use, we should make it simpler).

Punting responsibility onto the user isn't a recipe for a usable system. The
core idea of usability is that humans are fallible. If the system can help us
avoid mistakes, it should.

> Here's one suggestion, how we could handle this: We could introduce another
> "NotSupported" status file marker. It could look like this:
> 
>   [ $compiler == dart2js && $runtime == ie9 ]
>   LibTest/typed_data/Float32List/*: NotSupported
>   canvasrenderingcontext2d_test/*: NotSupported
> 
> This would make it crystal clear which tests are failing because they're not
> supported.

That sounds like an interesting approach to me. Pete, what do you think?

- bob

[kustermann, 2013-11-21 17:31:19]

PTAL.

I removed now the controversial part (i.e. as bob insisted, 0 tests == failure)

@blois/efortuna/...: I can wait until you have updated the html tests or I can
commit this as it is (and make status file updates for the runtimes I can't test
on my linux machine [e.g. safari, ie]).

[Bob Nystrom, 2013-11-21 18:33:15]

LGTM, but please check with someone who knows the HTML configs better than I
too.

+siggy in case he can help.

[Emily Fortuna, 2013-11-21 19:07:03]

We shouldn't commit this without adding in dummy tests or what have you for the
HTML tests, then. This affects a ton of HTML tests, and we need to continue to
monitor their functionality.

https://codereview.chromium.org/75393002/diff/110001/tests/html/html.status
File tests/html/html.status (right):

https://codereview.chromium.org/75393002/diff/110001/tests/html/html.status#n...
tests/html/html.status:423: custom/document_register_type_extensions_test/*:
Pass, RuntimeError
does * work here? I wasn't aware of that.

[kustermann, 2013-11-21 22:17:23]

I'll wait with this CL.

Since the unittest framework says "0 tests == failure" and we have a lot of
these, they should be fixed. I'd be very thankful if this could be done in the
near future.

https://codereview.chromium.org/75393002/diff/110001/tests/html/html.status
File tests/html/html.status (right):

https://codereview.chromium.org/75393002/diff/110001/tests/html/html.status#n...
tests/html/html.status:423: custom/document_register_type_extensions_test/*:
Pass, RuntimeError
On 2013/11/21 19:07:04, Emily Fortuna wrote:
> does * work here? I wasn't aware of that.

Yes it does work. It's necessary in this case, because the unittest
'group("...")' name contains spaces, which is not supported in status files --
so I match them with '*'.

[blois, 2013-11-21 23:59:17]

On 2013/11/21 22:17:23, kustermann wrote:
> I'll wait with this CL.
> 
> Since the unittest framework says "0 tests == failure" and we have a lot of
> these, they should be fixed. I'd be very thankful if this could be done in the
> near future.
> 
> https://codereview.chromium.org/75393002/diff/110001/tests/html/html.status
> File tests/html/html.status (right):
> 
>
https://codereview.chromium.org/75393002/diff/110001/tests/html/html.status#n...
> tests/html/html.status:423: custom/document_register_type_extensions_test/*:
> Pass, RuntimeError
> On 2013/11/21 19:07:04, Emily Fortuna wrote:
> > does * work here? I wasn't aware of that.
> 
> Yes it does work. It's necessary in this case, because the unittest
> 'group("...")' name contains spaces, which is not supported in status files --
> so I match them with '*'.

Alternatively is what I suggested earlier- modify html_individual_config to
allow passing with zero tests.

CL doing that is at: https://codereview.chromium.org/82113002

Siggi and I (and more) will be in Aarhus in 1.5 weeks, should discuss ideas on
test refactoring which may allow us to get away from html_individual_config.

[Jennifer Messerly, 2015-05-11 16:49:03]

jmesserly@google.com changed reviewers:
- jmesserly@google.com