Skip to content
This repository

Allow programmer to ask to stop on first failure. #319

Closed
wants to merge 5 commits into from

4 participants

R. Bernstein Michael G. Schwern Karen Etheridge Daniel Perrett
R. Bernstein
rocky commented June 09, 2012

Many times when I run a test suite what I want is to stop at the first failure. For example when installing modules from CPAN, CPAN will by default not install if there is any failure, so you may as well save time and stop after the first failure.

To first approximation it seems pretty easy to add this. I mean like one or two lines of Perl code. (testing and documentation not included in this count..) In the merge request code, set environment variable TEST_MORE_BAIL_EARLY.

The forked repository also has the corresponding code for the Test-Builder-1.5 branch as well. The test code however I've not been able to work out yet.

Also I haven't updated the documentation to mention this. However if the approach and code seems reasonable, I'll add and update the merge request.

added some commits June 09, 2012
R. Bernstein
rocky commented June 10, 2012

Test::Most looks interesting will have to try and use that. Thanks!

Alas there are too many packages out there using Test::More. With the last commit/update I have changed the name of the environment variable to BAIL_ON_FAIL to match Test::Most. And added mention of this in the pod.

This time, for sure! 3f46d9c
Michael G. Schwern
Owner

This idea has come up before. It's a good idea, but unfortunately there is a fundamental problem in the Test::Builder architecture. To see it, simply try BAIL_ON_FAIL with an is() or like() test. You will not see the usual got/expected diagnostics. This is because they come after ok() is done in a separate and unrelated function call. The test will fail and stop without giving you vital information about WHY it failed and stopped.

What would work is to bail at the end of the test if there is any failure. On failure set a flag. When the test is finishing it, bail out if that flag is set. That means if a .t file fails, at least the whole rest of the suite doesn't run.

Move the failure test to the beginning of the next text or _ending po…
…ssibly along the lines suggested by Michael Schwern.
668bb41
R. Bernstein
rocky commented June 10, 2012

Is this along the approach you were thinking?

Karen Etheridge

Alas there are too many packages out there using Test::More

Just run your test like this:

perl -Ilib -MTest::Most=die t/mytest.t

IMO it's not a good practice to actually edit the bail-on-first-failure behaviour directly into the test, as when you ship a dist, you want all tests to run, even if some fail. It's only during development that the bail-on-first-failure behaviour is desirable.

R. Bernstein
rocky commented June 12, 2012

@karenetheride reports:

Just run your test like this:

perl -Ilib -MTest::Most=die t/mytest.t

or

  perl -Ilib -MTest::Most=bail t/mytest.t

or

  BAIL_ON_FAIL=1 perl -Ilib -MTest::Most t/mytest.t

There's more than one way to do it (sm).

But I don't see how this helps with my second scenario. Perhaps I wasn't clear.

When I wrote "installing code from CPAN", I meant using perl -MCPAN -e shell or cpanp. And my CPAN/Config.pm contains:

'build_requires_install_policy' => q[follow/yes],
'prerequisites_policy' => q[follow],

So I don't have to babysit the install process. Many of the existing packages out there use Test::More or Test::Simple. If a change is made to the Test::More that I have installed, then I can control bailout say via an environment variable. I suppose I could install Test::Most in a way that it is used when Test::More is requested, but that seems a bit drastic.

By the way, when I installed my latest change along the lines as I understand Michael suggested, I can't pass the Test::Most tests I think because the Test::More is is bailing in on the Test::Most bailout test. I think this is the same situation as I see in my inability to write a bailout test for the change.

So for my own use, I like the simpler solution even if in some cases I don't get a got/expected output. The times I am likely to use BAIL_ON_FAIL are those times when all I only want to know about is whether things all pass, not why. If I want to know why I can always go back and redo that test.

Karen Etheridge

rocky: what I was meaning to say is "when you are developing your own code, you can run tests with Test::Most in 'die' mode to achieve this effect". What I left off, because I thought this was implicit, was "You really shouldn't be altering the behaviour of module installations from the CPAN". I would argue that the minute time savings in aborting earlier do not outweigh the drawbacks of increased complexity in the test system, and losing repeatability in module installations. For example, the cpantesters network installs and tests every (new and old) CPAN release and mails back test results to the authors - which includes full documentation of all tests that fail. Adding in a configuration to bail on the first test would diminish the benefits of such reports.

R. Bernstein
rocky commented June 13, 2012

Ok. Yes, I became aware that Test::Most had a BAIL_ON_FAIL from Ovid.

How cpantesters work and and how individuals who run CPAN or cpanp are different problems. That's why CPAN and cpanp come with configuration files that allow for customization. It allows one for example to specify whether dependencies should be followed or asked for. Smokers almost certainly don't use "ask" which would force someone to type "yes" all the time. However "ask" is the default for most users. This is similar. Some may not be interested in seeing all the failures for all the possible required modules; just whether there is at least one which stops installation. Just as I doubt a smoker would use "ask" to follow dependencies I would doubt that a smoker would ask to bail on the first error.

Daniel Perrett
pdl commented June 14, 2012

unfortunately there is a fundamental problem in the Test::Builder architecture ... got/expected diagnostics ... come after ok() is done in a separate and unrelated function call.

Is there a reason for this?

Is it configurable?

Michael G. Schwern
Owner

@karenetheridge wrt adding complexity and so on, while I generally agree with you... that doesn't mean we're paralyzed. What @rocky is proposing (early abort controlled by an environment variable) is simple to do and entirely optional. AFAIK, bailing out early doesn't break test automation, your test still fails. Maybe you don't get all the stats, but that's the user's choice.

Michael G. Schwern
Owner

@pdl I wrote up the problem, why its a problem, some common solutions and their issues. https://github.com/schwern/test-more/wiki/Stop-On-Fail

Daniel Perrett
pdl commented June 15, 2012

@schwern OK, so the reason we can't die on ok(0) is that

  • TB2 doesn't control when the relevant diagnostics are printed, any more than it knows about print calls.
  • Relevant modules put their diag()s AFTER the ok() by default.

Now, I may be talking pie in the sky here, but let's suppose we changed ok() so that it took another argument, a callback/string to be run on fail - and changed all the Test::* modules that use Test::Builder so that they include fail diags in the ok(). We could then control whether the diags appeared before the ok() or after: that seems to be the ideal solution architecturally.

Then, if we decided this was a) better, and b) worth it (!), we could say "This is the solution", create a flag such that a call to ok() would test, if it fails, run the diags code, report not ok, then abort the test run.

The downside is that even once we've trawled through all the public Test:: modules on CPAN, many other non-test distributions will still put their diags in the .t file after the ok(). You could argue any sort of 'bail on fail' flag is subject to a big caveat of 'don't assume you'll get all the diags'.

Leaving aside the magnitude of the task, is that an accurate summary of the problems for that approach?

Daniel Perrett
pdl commented June 15, 2012

Stop after the next ok.
What if that test fails, too?

I don't follow.

At the end of Test::Builder::ok, set a flag for the result of the ok().

Then at the beginning of Test::Builder::ok, bail if TEST_MORE_BAIL_EARLY is set and the result of the previous ok() indicates that it failed. Doesn't matter if the second ok() would have passed; it never gets evaluated.

R. Bernstein
rocky commented June 15, 2012

@pdl what you describe is almost the logic of my second attempt. However if the failure is the last ok, then there is no subsequent one. So for that I also put a test in Test::Builder->_ending as well. A down side is that you still run some user code after the failure. In particular the code after the ok/is, etc. and before the next one. Still not completely ideal, but catches more than the 90% case of just hooking into just Test::Builder->ending.

Whether my implementation lives up to my intent is a different story.

Michael G. Schwern
Owner

@pdl There's two separate issues here, and it's best to keep them separated. 1) Something that works with the existing way tests are written with Test::Builder. 2) Fixing the design. I'd like to focus on 1 here, and 2 elsewhere. 2 will take a very long time to percolate through CPAN, and there's additional issues to take into account when fixing it which simply bolting a flag onto ok() doesn't cover.

The larger issue is that its very useful to know when a test function starts and when it ends. Related is eliminating the need for $Level. Have a look at lib/Test/Builder2.pm to see the attempt on hold to solve the problem. It doesn't have to be solved that way, but it does outline some of the issues. I stopped work on Test::Builder2 to focus on Test::Builder1.5, but I'd love it if you want to pick it up. Crack open a new issue to continue the discussion.

@rocky brings up the excellent point that bailing on the next test means some user code gets run after the failure. I find this to be disquieting, but I can't really come up with a concrete problem this would cause. Maybe it just seems embarrassing that tests have to coast to a stop. :-) Plow ahead with what you're doing please.

Daniel Perrett
pdl commented June 16, 2012

@schwern " I'd like to focus on 1 here, and 2 elsewhere. "
Makes sense!

Karen Etheridge

@schwern Regarding https://github.com/schwern/test-more/wiki/Stop-On-Fail I have another potential solution to propose:

Change the semantics of $tb->ok so that accompanying diagnostic messages are all part of the same call.

Therefore, change this:

# Do the actual test, prints ok #, the line and file and so on.
my $ok = $tb->ok( $have eq $want, $name );

# Prints out the failure diagnostics.
$tb->diag(<<DIAGNOSTIC);

to this:

# Do the actual test, prints ok #, the line and file and also a more
# detailed message on failure.
my $ok = $tb2->ok( $have eq $want, $name, <<DIAGNOSTIC );

That way, we can still stop on the first failure, and be sure to print all
the necessary diagnostics, all in one go.

I think this is pretty doable - just change the prototype of ok() to ($$;$$).
Existing test libraries that use ok() don't have to immediately change, but
they can pass a fourth parameter to have that diagnostic string printed even
in die-on-first-failure mode.

Michael G. Schwern
Owner

@karenetheridge That is a possibility long considered to fix it at the design level and eventually have it trickle out. And it might be what Test::Builder has to do because of how its written.

But there's trouble. Not insurmountable.

The foremost is if you do it for ok() what about is()? And like()? And is_deeply()? And what about other modules like Test::Exception and Test::Deep and ... you get the idea. Everybody has to change their interface to accept more diagnostic information.

The additional problem is you have to pass in your diagnostic information at the point you run the test. If your diagnostics involve expensive calculations you have to do them pessimistically (or employ hacks).

And maybe these are surmountable.

Other ideas worth considering: if the $name is a hash ref it would be interpreted as diagnostics (the name is then passed in with the diagnostics). This presumes that nothing is doing anything with the name except Test::Builder, which is probably safe. That has the benefit of not having to update every interface. The down side is now we're guessing at the user's intent, which is a big no-no.

The other approach is what Test::Builder2 (the class) tries to do. It keeps track of the stack of test functions which have been called (for example it will know that ok() was called by is()). This was both to get rid of the need or $Test::Builder::Level, Test::Builder2 knows where the stack of assert calls started, but it also means the result does not get posted (and thus output) until the stack has completely returned. What does this mean? It means the result is not output when $tb->ok is called, but when is() (or whatever) finally returns. This allows...

sub is {
    my($have, $want, $name) = @_;

    my $result = $tb->ok($have eq $want, $name);
    $result->diagnostics({ have => $have, want => $want });

    # Only now is $result turned into TAP
    return $result;
}

It's very flexible and it eliminates the need for $Test::Builder::Level, but it also turned out to have a lot of corner cases and was de-prioritized. It also requires people to use Test::Builder2 instead of Test::Builder which would take a long time to convert.

What do you think?

Daniel Perrett
pdl commented August 18, 2012

The additional problem is you have to pass in your diagnostic information at the point you run the test. If your diagnostics involve expensive calculations you have to do them pessimistically (or employ hacks).

(Appreciate this is a tangent but...) this could (I think) be solved if the onfail argument was a coderef?

Michael G. Schwern
Owner

@pdl Clever! Now all we have to do is add an onfail argument to everything. But I guess by "onfail" you mean the diagnostic argument? Structured diagnostics (not the strings we currently pass into diag()) would be desirable on passing tests as well. They just might not be displayed, but they'd be available for other purposes. For example, you could add timing information.

Karen Etheridge

Just to be clear, when we say "user" here we mean "author of a test library", not "author of tests". So it is Test::Builder::ok (and friends) that would change, not the users of Test::More::ok etc.

Since it already looks like every test library will need to be upgraded anyway to use Test::Tester rather than Test::Builder::Tester, this can be thrown in as just one more API change (and one that is backwards compatible too).

I also like the idea of onfail being a coderef.

R. Bernstein

And just to be clear. What I was looking for was a change to Test::More/Test::Simple that would stop on the first failure without having to change any of the vast existing code that uses these.

As a writer of tests, if I want to allow folks to be able to stop at the first failure, I'd use Test::Most or something already available that allows this.

I am coming at this from the standpoint of people who are installing the Perl modules of others and only want to ask the question: does everything check out? If not, please tell me as fast as possible with the least amount of other extraneous information. A secondary concern as the writer of the tests is that I may want to stop at the first failure for the same reason and because my brain wants to focus on one problem at a time.

Daniel Perrett
pdl commented August 19, 2012

I am coming at this from the standpoint of people who are installing the Perl modules of others and only want to ask the question: does everything check out? If not, please tell me as fast as possible with the least amount of other extraneous information

@rocky I think, in summary, while this is obviously possible (I'd also like to be able to do this!), the question is how do we keep the most amount of relevant information, i.e. diagnostics calculated after the fail. The options seem to be:

  1. Fail, and stop immediately, losing all post-test diagnostics; encourage authors to diagnose first, ok() later.
  2. Fail and stop at a future point, as soon as we can reasonably determine the failing test has ended, and hope that the test author has put test diags before they run the next test, not somewhere at the end as a summary.
  3. Modify Test::Builder::ok() et al. so that test authors can explicitly link diags with tests, then wait for the best-practice to percolate through the CPAN.

If we can do 3 in the long run, it would be better for your use case. The solution for 3 might affect which of 1 and 2 is more suitable as an interim solution.

Michael G. Schwern
Owner
Michael G. Schwern
Owner
Michael G. Schwern
Owner

I've proposed the "bail on fail" option to Test::Harness. Perl-Toolchain-Gang/Test-Harness#4

Michael G. Schwern
Owner

This pull request has a lot of interesting discussion about a lot of things having to do with stopping the test after failure. I'd like to close it up, but capture something concrete. I've opened #319 for the idea of providing a means to bail out at the end of a test file.

Michael G. Schwern schwern closed this April 23, 2013
Michael G. Schwern
Owner

Whoops, I meant #373

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Showing 5 unique commits by 1 author.

Jun 09, 2012
Allow programmer to ask to bail after first failure. It saves a lot o…
…f time!

Now in master branch.
1971c54
Untabify change. 91dd9a4
Jun 10, 2012
Change bail on fail environment to BAIL_ON_FAIL to match Test::Most. …
…Note BAIL_ON_FAIL in pod.
1ddcd6c
This time, for sure! 3f46d9c
Jun 11, 2012
Move the failure test to the beginning of the next text or _ending po…
…ssibly along the lines suggested by Michael Schwern.
668bb41
This page is out of date. Refresh to see the latest.
11  lib/Test/Builder.pm
@@ -459,6 +459,7 @@ sub reset {    ## no critic (Subroutines::ProhibitBuiltinHomonyms)
459 459
     $self->{Todo_Stack} = [];
460 460
     $self->{Start_Todo} = 0;
461 461
     $self->{Opened_Testhandles} = 0;
  462
+    $self->{No_Failures} = 1;
462 463
 
463 464
     $self->_share_keys;
464 465
     $self->_dup_stdhandles;
@@ -803,6 +804,10 @@ like Test::Simple's C<ok()>.
803 804
 sub ok {
804 805
     my( $self, $test, $name ) = @_;
805 806
 
  807
+    if ($ENV{'BAIL_ON_FAIL'} && !$self->{No_Failures}) {
  808
+        $self->BAIL_OUT("Early exit requested.")
  809
+    }
  810
+
806 811
     if ( $self->{Child_Name} and not $self->{In_Destroy} ) {
807 812
         $name = 'unnamed test' unless defined $name;
808 813
         $self->is_passing(0);
@@ -888,6 +893,8 @@ ERR
888 893
     # Check that we haven't violated the plan
889 894
     $self->_check_is_passing_plan();
890 895
 
  896
+    $self->{No_Failures} &&= $test;
  897
+
891 898
     return $test ? 1 : 0;
892 899
 }
893 900
 
@@ -2452,6 +2459,10 @@ sub _ending {
2452 2459
         return;
2453 2460
     }
2454 2461
 
  2462
+    if ($ENV{'BAIL_ON_FAIL'} && !$self->{No_Failures}) {
  2463
+        $self->BAIL_OUT("Early exit requested.")
  2464
+    }
  2465
+
2455 2466
     # Ran tests but never declared a plan or hit done_testing
2456 2467
     if( !$self->{Have_Plan} and $self->{Curr_Test} ) {
2457 2468
         $self->is_passing(0);
6  lib/Test/More.pm
@@ -1399,6 +1399,12 @@ available such as a database connection failing.
1399 1399
 
1400 1400
 The test will exit with 255.
1401 1401
 
  1402
+Sometimes in developing code or installing dependant package from
  1403
+CPAN, you may want to bail out after the first failure. To do that set
  1404
+environment variable C<FAIL_ON_BAIL>. For example:
  1405
+    
  1406
+   BAIL_ON_FAIL=1 prove t/
  1407
+
1402 1408
 For even better control look at L<Test::Most>.
1403 1409
 
1404 1410
 =cut
43  t/bail_early.t.try
... ...
@@ -0,0 +1,43 @@
  1
+#!/usr/bin/perl -w
  2
+
  3
+BEGIN {
  4
+    if( $ENV{PERL_CORE} ) {
  5
+        chdir 't';
  6
+        @INC = ('../lib', 'lib');
  7
+    }
  8
+    else {
  9
+        unshift @INC, 't/lib';
  10
+    }
  11
+}
  12
+
  13
+my $Exit_Code;
  14
+BEGIN {
  15
+    *CORE::GLOBAL::exit = sub { $Exit_Code = shift; };
  16
+}
  17
+
  18
+
  19
+use Test::Builder;
  20
+use Test::More;
  21
+
  22
+my $output;
  23
+my $TB = Test::More->builder;
  24
+$TB->output(\$output);
  25
+
  26
+my $Test = Test::Builder->create;
  27
+$Test->level(0);
  28
+
  29
+$Test->plan(tests => 3);
  30
+
  31
+plan tests => 4;
  32
+
  33
+$ENV{BAIL_ON_FAIL}=1;
  34
+ok(0, "Let's see what happens when we fail");
  35
+
  36
+$Test->is_eq( $output, <<'OUT' );
  37
+1..4
  38
+not ok 1 - Let's see what happens when we fail
  39
+OUT
  40
+
  41
+$Test->is_eq( $Exit_Code, 255 );
  42
+
  43
+$Test->ok( $Test->can("BAILOUT"), "Backwards compat" );
Commit_comment_tip

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.