Revert use_ok() change to allow lexical effects?

[Michael G Schwern]

In a series of patches, Father Chrysostomos and I enhanced use_ok() so that it
can apply lexical effects to more closely emulate the real `use`. For example,

use_ok('strict');

Previously this would just load strict and call import, but strictures would
not actually be applied to your scope. Now strictures *will* be applied
making it work more like a regular use.

Testing has shown it's caused just a handful of compatibility problems, and it
hasn't gone out in a stable release yet. Because the new use_ok() is so much
more complex than before I'm having second thoughts about whether it's worth
it and would like opinions.

There are two questions:

1. Should the lexical effect patches to use_ok() be rolled back?
2. Should use_ok() be discouraged in the documentation?

The new use_ok(), while it works remarkably well, is significantly more
complex than previously and it touches magical variables $^H and %^H. There
is a danger of invoking bugs by touching those variables.

It continues to have compatibility issues, for example...

BEGIN { use_ok 'Some::Module' }

this will apply lexical effect while...

BEGIN { use strict }

will not but

BEGIN { require strict; strict->import }

will. Confusing.

use_ok() has low utility to begin with. If you want exports and lexical
effects to be properly applied you have to wrap it in a BEGIN block. And as
has been pointed out before, if use_ok() fails you probably want the program
to halt. So the full invocation should be...

BEGIN {
use_ok("Some::Module") or die;
}

At which point you might as well write...

use Some::Module;

And if you really want the load to be a passing test, you can add...

pass("Loaded Some::Module");

If all you want to do is check that a module can compile, there is the much
simpler require_ok().

Apologies to Father Chrysostomos if his work is reverted, it has been top
notch. If it is reverted, it may find life in another module where a complete
emulation of use is desired as a user function or method.

[Paul Johnson]

Post by Michael G Schwern
In a series of patches, Father Chrysostomos and I enhanced use_ok() so that it
can apply lexical effects to more closely emulate the real `use`.
1. Should the lexical effect patches to use_ok() be rolled back?

I'm slightly in favour of this.

Post by Michael G Schwern
2. Should use_ok() be discouraged in the documentation?

I'm very much in favour of this.

Post by Michael G Schwern
The new use_ok(), while it works remarkably well, is significantly more
complex than previously and it touches magical variables $^H and %^H. There
is a danger of invoking bugs by touching those variables.

I still appreciate Joshua Pritikin's parenthetical comment in the
original Test.pm:

Your test code should be simpler than the code it is testing, yes?

Post by Michael G Schwern
Apologies to Father Chrysostomos if his work is reverted, it has been top
notch. If it is reverted, it may find life in another module where a complete
emulation of use is desired as a user function or method.

It might be nice to take the enhanced use_ok code out completely and put
it into its own module.

Yeah, I know.

[Mike Doherty]

Post by Paul Johnson

Post by Michael G Schwern
2. Should use_ok() be discouraged in the documentation?

I'm very much in favour of this.

I don't see any discouragement in the documentation... and what's wrong
with use_ok to begin with?

-Mike

[Leon Timmermans]

Post by Mike Doherty
I don't see any discouragement in the documentation... and what's wrong
with use_ok to begin with?

If it fails, the module may not be loaded, or partially loaded. In
such circumstances, testing the rest of the code can give very
confusing results. It doesn't make sense to continue testing usually.

Leon

[Andy Lester]

Post by Leon Timmermans
If it fails, the module may not be loaded, or partially loaded. In
such circumstances, testing the rest of the code can give very
confusing results. It doesn't make sense to continue testing usually.

It seems that use_ok() ought to die if it fails. But then we might as well not wrap our use in use_ok().

Is there a case when we DO want to use use_ok()?

xoa

--
Andy Lester => ***@petdance.com => www.petdance.com => AIM:petdance

[Mike Doherty]

Post by Leon Timmermans

Post by Mike Doherty
I don't see any discouragement in the documentation... and what's wrong
with use_ok to begin with?

If it fails, the module may not be loaded, or partially loaded. In
such circumstances, testing the rest of the code can give very
confusing results. It doesn't make sense to continue testing usually.

I typically use_ok(...) or BAIL_OUT. If that's the only way to use
use_ok safely, then maybe it should do that for you automatically.

-Mike

[Aristotle Pagaltzis]

Post by Mike Doherty
I typically use_ok(...) or BAIL_OUT. If that's the only way to use
use_ok safely, then maybe it should do that for you automatically.

I don’t think changing its meaning so drastically is feasible now.

Also, this leaves the issue that if it (or any other replacement of it
with different semantics) becomes a recommended interface then the
behaviour of `use` in lexical scope probably *should* be duplicated.

But that is complexity that ideally should be shed.

I think the ideal way to handle `use` would be a `bail_on_use_failure`
switch that intercepts `use` errors and turns them into BAIL_OUT, so
that plain `use` can be used for everything – assuming this can be done
with less complexity than duplicating `use`.

Then `use_ok` would just completely deprecated.

But it’ll take a while for the entire CPAN to move off of it, so it
can’t be deprecated too aggressively. However if it doesn’t make any
noise, then shedding it will never happen.

So maybe announce the deprecation during one perl release cycle and then
add a once-per-testsuite warning in the next?

Regards,

[The Sidhekin]

Post by Mike Doherty
I typically use_ok(...) or BAIL_OUT. If that's the only way to use
use_ok safely, then maybe it should do that for you automatically.

I don’t think changing its meaning so drastically is feasible now.
Also, this leaves the issue that if it (or any other replacement of it
with different semantics) becomes a recommended interface then the
behaviour of `use` in lexical scope probably *should* be duplicated.
But that is complexity that ideally should be shed.
I think the ideal way to handle `use` would be a `bail_on_use_failure`
switch that intercepts `use` errors and turns them into BAIL_OUT, so
that plain `use` can be used for everything – assuming this can be done
with less complexity than duplicating `use`.
Then `use_ok` would just completely deprecated.
But it’ll take a while for the entire CPAN to move off of it, so it
can’t be deprecated too aggressively. However if it doesn’t make any
noise, then shedding it will never happen.
So maybe announce the deprecation during one perl release cycle and then
add a once-per-testsuite warning in the next?

Two questions:

* How would you rewrite a test script such as my own
http://cpansearch.perl.org/src/EBHANSSEN/Test-Trap-v0.2.2/t/00-load.t so
that it does not use use_ok()?
* Why would you? :-\

(Sure, most of those could be just require_ok, but some would need to be
require_ok + import. Why should I care? I just want to know what breaks
when something's so badly broken that loading fails ... so in order to make
maintainence simpler, I'd just test require_ok + import for all of them.
At that point, I'd probably write a function that did both require_ok and
import ... now, what to call it ... oh.)


Eirik

[Ovid]

----- Original Message -----

Post by The Sidhekin
* How would you rewrite a test script such as my own
http://cpansearch.perl.org/src/EBHANSSEN/Test-Trap-v0.2.2/t/00-load.t so
that it does not use use_ok()?
* Why would you? :-\

Just a quick hack:

    use Test::More;                                                                                                                               

    BEGIN {
        my @modules = qw(
          Test::Trap::Builder::TempFile
          Test::Trap::Builder::SystemSafe
          Test::Trap::Builder
          Test::Trap
        );
        push @modules => 'Test::Trap::Builder::PerlIO' if eval "use PerlIO; 1";
        plan tests => scalar @modules;

        for my $module (@modules) {
            eval "use $module";
            BAIL_OUT $@ if $@;
        }
    }


With that, you're using the actual use builtin and not worrying about extra code that might or might not be obscuring problems.

Cheers,
Ovid 
--
Live and work overseas - http://www.overseas-exile.com/
Buy the book - http://www.oreilly.com/catalog/perlhks/
Tech blog - http://blogs.perl.org/users/ovid/
Twitter - http://twitter.com/OvidPerl/

[Ovid]

Or more simply:

    use Test::More tests => 1;

    my $ok;
    END { BAIL_OUT "Could not load all modules" unless $ok }
    use Test::Trap::Builder::TempFile;                                                                                                            
    use Test::Trap::Builder::SystemSafe;
    use Test::Trap::Builder;
    use Test::Trap;
    use if eval "use PerlIO; 1", 'Test::Trap::Builder::PerlIO';
    ok 1, 'All modules loaded successfully';
    $ok = 1;

Cheers,
Ovid
--
Live and work overseas - http://www.overseas-exile.com/
Buy the book - http://www.oreilly.com/catalog/perlhks/
Tech blog - http://blogs.perl.org/users/ovid/
Twitter - http://twitter.com/OvidPerl/


----- Original Message -----

Sent: Wednesday, 11 April 2012, 16:22
Subject: Re: Revert use_ok() change to allow lexical effects?
----- Original Message -----

Post by The Sidhekin
* How would you rewrite a test script such as my own
http://cpansearch.perl.org/src/EBHANSSEN/Test-Trap-v0.2.2/t/00-load.t so
that it does not use use_ok()?
* Why would you? :-\

    use Test::More;                                                            
                                                                  
    BEGIN {
          Test::Trap::Builder::TempFile
          Test::Trap::Builder::SystemSafe
          Test::Trap::Builder
          Test::Trap
        );
            eval "use $module";
        }
    }
With that, you're using the actual use builtin and not worrying about extra
code that might or might not be obscuring problems.
Cheers,
Ovid 
--
Live and work overseas - http://www.overseas-exile.com/
Buy the book - http://www.oreilly.com/catalog/perlhks/
Tech blog - http://blogs.perl.org/users/ovid/
Twitter - http://twitter.com/OvidPerl/

[Aristotle Pagaltzis]

Post by Ovid
    my $ok;
    END { BAIL_OUT "Could not load all modules" unless $ok }

[...]

Post by Ovid
    ok 1, 'All modules loaded successfully';
    $ok = 1;

Ah d’uh! Now I feel stupid.

This could so easily be packaged in a handful of lines:

package Test::AutoBailOut;
use parent 'Test::Builder::Module';
sub _tb { __PACKAGE__->builder }

my $reason = 'Tests must succeeded';
sub import {
my %arg = @_;
$reason = $arg{'reason'} if exists $arg{'reason'};
}

my $ok;
sub no_bailout ($) { _tb->ok( $ok = 1, @_ ) }
    END { _tb->BAIL_OUT( $reason ) unless $ok }

@EXPORT = 'no_bailout';

Then you simply do this:

    use Test::More tests => 1;
use Test::AutoBailOut reason => 'Could not load prerequisites';
use Config;

    use Test::Trap::Builder::TempFile;
    use Test::Trap::Builder::SystemSafe;
    use Test::Trap::Builder;
    use Test::Trap;
    use if $Config{'useperlio'}, 'Test::Trap::Builder::PerlIO';

no_bailout 'All prerequisites loaded';

Dead simple. Huge swathes of complexity, *puff* – vanished.

Schwern, care to ship that in Test::More to deprecate `use_ok`? :)

Regards,

[Smylers]

Post by Aristotle Pagaltzis
Ah d’uh! Now I feel stupid.

Please don't! You're still ahead many of us ...
Thanks for doing that.

Post by Aristotle Pagaltzis
my $reason = 'Tests must succeeded';

Grammar correction if anybody is going to publish this in a module:
"succeed", rather than "succeeded".

Cheers

Smylers

[Aristotle Pagaltzis]

Post by Smylers

Post by Aristotle Pagaltzis
my $reason = 'Tests must succeeded';

"succeed", rather than "succeeded".

Woops. Thanks.

Post by Smylers

Post by Aristotle Pagaltzis
Ah d’uh! Now I feel stupid.

Please don't! You're still ahead many of us ...

See above. ;-)

Regards,

[Michael G Schwern]

Post by Ovid
use Test::More tests => 1;
use Test::AutoBailOut reason => 'Could not load prerequisites';
use Config;
use Test::Trap::Builder::TempFile;
use Test::Trap::Builder::SystemSafe;
use Test::Trap::Builder;
use Test::Trap;
use if $Config{'useperlio'}, 'Test::Trap::Builder::PerlIO';
no_bailout 'All prerequisites loaded';
Dead simple. Huge swathes of complexity, *puff* – vanished.
Schwern, care to ship that in Test::More to deprecate `use_ok`? :)

Nope, too much magic for too small a use case. You should make it a module
and talk to Ovid about Test::Most.

Let me reiterate, I have no plans to *deprecate* `use_ok`. Even if I wanted
to there are simply too many users to make deprecation worth while.

It works fine if what you want is a runtime require + import + assert, and
sometimes you want that. The problem is it's been overused and has come to
replace a simple `use` in test scripts. To that end the question is whether
to *discourage* its use in the documentation: to scale it back from "use this
when you load a module" to "use this for some special cases".

[Aristotle Pagaltzis]

Post by Michael G Schwern
Nope, too much magic for too small a use case.

And faithfully duplicating `use` would be less so? :-)

I don’t see how it is any more magic than `done_testing`.

What exactly makes you uneasy? Maybe there is a way to address that
if you can be more specific.

What I don’t like about duplicating `use` is that you need to diddle
internals and keep in mind a long series of edge cases in the interface
that matter to almost no one except those few people who need them. So
it’s very difficult to be sure that the implementation is fully complete
and correct, and hard to find out at short notice when it gets broken by
changes to perl, if any. And all that when there is no reason not to
just use the original instead. It’s ridiculous when you stop to think
about it.

Post by Michael G Schwern
Let me reiterate, I have no plans to *deprecate* `use_ok`. Even if
I wanted to there are simply too many users to make deprecation worth
while.

Yes, as I mentioned, I can see that.

Post by Michael G Schwern
It works fine if what you want is a runtime require + import + assert,
and sometimes you want that. The problem is it's been overused and
has come to replace a simple `use` in test scripts. To that end the
question is whether to *discourage* its use in the documentation: to
scale it back from "use this when you load a module" to "use this for
some special cases".

*Which* special cases? I would rather not recommend it in any case ever.
My suggestion to ship AutoBailOut was so you would be able to suggest it
as a replacement in the docs, as it covers the one case where `use_ok`
is even of interest (though still not the right solution).

But I guess you could do that even if it ships outside of Test::More.

Regards,

[Ovid]

----- Original Message -----

Post by Aristotle Pagaltzis

Post by Michael G Schwern
Nope, too much magic for too small a use case.

And faithfully duplicating `use` would be less so? :-)
I don’t see how it is any more magic than `done_testing`.

Because done_testing is applicable to every test module and solves the far more common issue of hating maintain a plan. I would argue that done_testing is much more necessary than an AutoBailout. That being said, I'd use AutoBailout a lot more often if it was available. Then again, as Schwern pointed out, that's why I wrote Test::Most :)

Post by Aristotle Pagaltzis
What I don’t like about duplicating `use` is that you need to diddle
internals and ...

I think Schwern's not arguing against this. He's just trying to figure out the best way forward.

Cheers,
Ovid

--
Live and work overseas - http://www.overseas-exile.com/
Buy the book - http://www.oreilly.com/catalog/perlhks/
Tech blog - http://blogs.perl.org/users/ovid/
Twitter - http://twitter.com/OvidPerl/

[Aristotle Pagaltzis]

Post by Ovid

Post by Aristotle Pagaltzis
I don’t see how it is any more magic than `done_testing`.

Because done_testing is applicable to every test module and solves the
far more common issue of hating maintain a plan. I would argue that
done_testing is much more necessary than an AutoBailout.

Well sure, which is why I suggested a separate module and not a patch to
Test::More itself. (Also since it is doable separately with such a clean
implementation.)

Post by Ovid
That being said, I'd use AutoBailout a lot more often if it was
Test::Most :)

I guess I should go and put AutoBailout on CPAN. :-)

Post by Ovid

Post by Aristotle Pagaltzis
What I don’t like about duplicating `use` is that you need to diddle
internals and ...

I think Schwern's not arguing against this. He's just trying to figure
out the best way forward.

Yes, that was not an argument, just stating my position on it in line
with my question about his unease.

Regards,

[Aristotle Pagaltzis]

Btw Ovid,

Post by Ovid
done_testing is applicable to every test module and solves the far
more common issue of hating maintain a plan.

^^^^^^

a little Freudian slip there? :-)

[Michael G Schwern]

Post by Aristotle Pagaltzis

Post by Michael G Schwern
Nope, too much magic for too small a use case.

And faithfully duplicating `use` would be less so? :-)

This discussion is about backing away from that. The most magical bits of
use_ok() have not yet gone out in a stable and can be reverted.

Post by Aristotle Pagaltzis
I don’t see how it is any more magic than `done_testing`.

done_testing() has no global side effects, it's just a function.

Unless I'm mistaken, Test::AutoBailOut is doing to need a global $SIG{__DIE__}
handler or override CORE::require or add something to @INC or try to parse
exception messages or something like that. Any of those have global side
effects which can potentially interfere with or be overwritten by the code
being tested.

Post by Aristotle Pagaltzis
What exactly makes you uneasy? Maybe there is a way to address that
if you can be more specific.

Mostly that it's cramming yet more complexity into core test library for a
fairly narrow use case and functionality that lives quite happily in its own
module. It's already difficult enough to keep stable.

To reverse it, why should it be in Test::More? "People don't like to have
extra test dependencies" is not accepted. If you want to bundle it all
together into a sort of universal set of testing extensions, Test::Most does that.

While it could be argued that every distribution should be testing that their
modules load and bailing out if they don't, the bailing out part we can live
without. But it is handy.

Post by Aristotle Pagaltzis

Post by Michael G Schwern
It works fine if what you want is a runtime require + import + assert,
and sometimes you want that. The problem is it's been overused and
has come to replace a simple `use` in test scripts. To that end the
question is whether to *discourage* its use in the documentation: to
scale it back from "use this when you load a module" to "use this for
some special cases".

*Which* special cases? I would rather not recommend it in any case ever.

In my reply to Andy I outlined some cases where you need a runtime require +
import + assert and its better to have a function which does that than write
it all out by hand.

Post by Aristotle Pagaltzis
My suggestion to ship AutoBailOut was so you would be able to suggest it
as a replacement in the docs, as it covers the one case where `use_ok`
is even of interest (though still not the right solution).
But I guess you could do that even if it ships outside of Test::More.

Precisely. For example, it already suggests Test::Differences and Test::Deep.

[Eric Wilhelm]

# from Michael G Schwern

Post by Michael G Schwern

Post by Aristotle Pagaltzis
What exactly makes you uneasy? Maybe there is a way to address that
if you can be more specific.

Mostly that it's cramming yet more complexity into core test library
for a fairly narrow use case and functionality that lives quite
happily in its own module.  It's already difficult enough to keep
stable.
To reverse it, why should it be in Test::More?  "People don't like to
have extra test dependencies" is not accepted.

Looking at this a different way, instead of a library, make a distzilla
extension (or whatever) which generates (and regenerates) a 00-load.t
as per Ovid's earlier example.

The trouble with having it in a library is not just that you have a test
dependency, but also that you have to add more complexity to the
library (and then you have a test dependency on a newer version)
whenever someone wants to do a complex thing.

Many modules in a distribution may not load unless $optional_dependency
is satisfied. But, it is good to precheck all of the modules in a
distribution before starting tests (aside: did someone say prefork?)
and helps to wrap this in some useful diagnostic + bail out.

I think something which embeds code in a 00-load.t, plus helps you
maintain a list of modules by scanning lib/ would be a handy way to do
that. (Perhaps a list + hash to address the optional dependencies and
each optional module has a subref to determine if it is enabled.)

One could argue that the functionality should be included in each test
file to keep things orthogonal. Such a one may find that there may be
a way to require("./t/00-load.t") from each test file, but might also
end up with a library and/or a hybrid with regenerative breaking
(prefork.)

--Eric

[Buddy Burden]

Guys,

Post by Eric Wilhelm
Looking at this a different way, instead of a library, make a distzilla
extension (or whatever) which generates (and regenerates) a 00-load.t
as per Ovid's earlier example.

This sounds like the best idea to me.


            -- Buddy

[Aristotle Pagaltzis]

Post by Michael G Schwern

Post by Aristotle Pagaltzis
I don’t see how it is any more magic than `done_testing`.

done_testing() has no global side effects, it's just a function.
Unless I'm mistaken, Test::AutoBailOut is doing to need a global
$SIG{__DIE__} handler or override CORE::require or add something to
@INC or try to parse exception messages or something like that. Any
of those have global side effects which can potentially interfere with
or be overwritten by the code being tested.

… what for? Is Ovid’s solution of just using an END block insufficient?
Why?

Post by Michael G Schwern

Post by Aristotle Pagaltzis
My suggestion to ship AutoBailOut was so you would be able to
suggest it as a replacement in the docs, as it covers the one case
where `use_ok` is even of interest (though still not the right
solution).
But I guess you could do that even if it ships outside of
Test::More.

Precisely. For example, it already suggests Test::Differences and
Test::Deep.

OK. Then how about I stick AutoBailout on CPAN with a SYNOPSIS that
covers `t/00-load.t`, and then you change the `use_ok` docs to a)
discourage its use and b) point to AutoBailout for `t/00-load.t` uses.
Sound good?

Regards,

[Andy Lester]

Post by Aristotle Pagaltzis
OK. Then how about I stick AutoBailout on CPAN with a SYNOPSIS that
covers `t/00-load.t`, and then you change the `use_ok` docs to a)
discourage its use and b) point to AutoBailout for `t/00-load.t` uses.
Sound good?

As a module author, I would not require a user to install AutoBailout.pm just to remove boilerplate in my t/00-load.t

But that's just me.

xoa

--
Andy Lester => ***@petdance.com => www.petdance.com => AIM:petdance

[Michael Peters]

Post by Andy Lester
As a module author, I would not require a user to install AutoBailout.pm just to remove boilerplate in my t/00-load.t

With the test_requires stuff they won't have to. It will only be used
for testing and not installed permanently on their system.

[Andy Lester]

Post by Andy Lester
As a module author, I would not require a user to install AutoBailout.pm just to remove boilerplate in my t/00-load.t

With the test_requires stuff they won't have to. It will only be used for testing and not installed permanently on their system.

test_requires is Module::Build only, right? I don't use Module::Build.

Even if I did, I don't think I'd require the user to go through a download and temporary build of AutoBailout.pm just to remove boilerplate.

xoa

--
Andy Lester => ***@petdance.com => www.petdance.com => AIM:petdance

[Michael Peters]

test_requires is Module::Build only, right? I don't use Module::Build.

No, I'm pretty sure it works with Module::Install and
ExtUtils::MakeMaker now too. Although that might just be something that
was worked on at the qa-hackathon.

Even if I did, I don't think I'd require the user to go through a
download and temporary build of AutoBailout.pm just to remove boilerplate.

That's definitely a reasonable position to take.

[Aristotle Pagaltzis]

Post by Michael Peters

test_requires is Module::Build only, right? I don't use
Module::Build.

No, I'm pretty sure it works with Module::Install and
ExtUtils::MakeMaker now too. Although that might just be something
that was worked on at the qa-hackathon.

If the issue is really the extra download (for whatever reason that
isn’t obvious to me), Andy can also just copy it into t/lib. It’s one
dozen-line pure-perl file that isn’t likely to ever change, so…

Might as well have it on CPAN instead of pasting a recipe into the
Test::More docs and then having everyone paste that into their test
scripts in turn.

Post by Michael Peters

Even if I did, I don't think I'd require the user to go through
a download and temporary build of AutoBailout.pm just to remove
boilerplate.

That's definitely a reasonable position to take.

Sure. Same can be said of many Test modules. Test::Exception or just
make do with just an `eval` and some logic and a few extra tests?
Test::Deep or just stick with the worse output of `is_deeply` when it
suffices? Etc. Every author can make each of these choices whichever way
they like, and so can Andy. Likewise with AutoBailout. The stop energy
he is throwing at it has no substantive reason so far, only “I don’t
care for this”.

Regards,

[Andy Lester]

Post by Aristotle Pagaltzis
The stop energy
he is throwing at it has no substantive reason so far, only “I don’t
care for this”.

No, no, no, I'm sorry, I didn't mean to make it sound like "You shouldn't do this module." No stop energy intended. I just wanted to provide a counter viewpoint from a potential customer.

I don't want to have another module prereq. I'd probably just do the cut & paste into my t/00-load.t.

Carry on!

xoa

--
Andy Lester => ***@petdance.com => www.petdance.com => AIM:petdance

[Michael G Schwern]

Post by Andy Lester
test_requires is Module::Build only, right? I don't use Module::Build.

It was just added to ExtUtils::MakeMaker but I don't know when a stable will
be released.

[Aristotle Pagaltzis]

Post by Andy Lester
As a module author, I would not require a user to install
AutoBailout.pm just to remove boilerplate in my t/00-load.t

If it’s only a test_requires the user won’t have to. (It would be pretty
silly to keep discarding it though if it gets widespread use, esp seeing
as it’s only a dozen lines of pure Perl.)

Post by Andy Lester
But that's just me.

Yup. And it’s a free country. :-)

Regards,

[Michael G Schwern]

Post by Aristotle Pagaltzis

Post by Michael G Schwern
Unless I'm mistaken, Test::AutoBailOut is doing to need a global
$SIG{__DIE__} handler or override CORE::require or add something to
@INC or try to parse exception messages or something like that. Any
of those have global side effects which can potentially interfere with
or be overwritten by the code being tested.

… what for? Is Ovid’s solution of just using an END block insufficient?
Why?

You're referring to this?
http://www.nntp.perl.org/group/perl.qa/2012/04/msg13128.html

If all you want is to bail out if the test fails to complete, that's easy(er).
I thought it was going to do something specifically when a `use` fails, thus
all the complexity.

A general "bail out if this test file fails" function might be handy. An even
more general "do X if the test finishes in Y state" would be even more handy.

# There is no test_passed()
END { BAIL_OUT() unless test_passed(); }

I don't want to add a huge bunch of functions to Test::More to cover all the
possible test states, in 1.5 you can get them from the TB2::History object.

# This works in TB 1.5
END { BAIL_OUT() unless Test::More->builder->history->test_was_successful }

Which is a bit of a mouthful. Providing a function to get at the history
object would make a bunch of test state introspection easier.

# That looks pretty good.
END { BAIL_OUT() unless test_history->test_was_successful }

[Aristotle Pagaltzis]

Hi Michael,

Post by Michael G Schwern

Post by Aristotle Pagaltzis

Post by Michael G Schwern
Unless I'm mistaken, Test::AutoBailOut is doing to need a global
$SIG{__DIE__} handler or override CORE::require or add something to
@INC or try to parse exception messages or something like that. Any
of those have global side effects which can potentially interfere with
or be overwritten by the code being tested.

… what for? Is Ovid’s solution of just using an END block insufficient?
Why?

You're referring to this?
http://www.nntp.perl.org/group/perl.qa/2012/04/msg13128.html

yes.

Post by Michael G Schwern
If all you want is to bail out if the test fails to complete, that's easy(er).

That’s all AutoBailout does, it is just Ovid’s code packaged up as
a tiny module. That is why I suggested shipping it with Test::More.
I cannot imagine it ever causing a burden (though I’m fine with shipping
it separately too). Just because it does so little, bordering on nothing.

Post by Michael G Schwern
I thought it was going to do something specifically when a `use`
fails, thus all the complexity.

Well, does it need to? Test reports will contain STDERR anyway. All you
lose is that the error message is a bit more raw.

Post by Michael G Schwern
A general "bail out if this test file fails" function might be handy.
An even more general "do X if the test finishes in Y state" would be
even more handy.
# There is no test_passed()
END { BAIL_OUT() unless test_passed(); }
I don't want to add a huge bunch of functions to Test::More to cover
all the possible test states, in 1.5 you can get them from the
TB2::History object.
# This works in TB 1.5
END { BAIL_OUT() unless Test::More->builder->history->test_was_successful }
Which is a bit of a mouthful. Providing a function to get at the
history object would make a bunch of test state introspection easier.
# That looks pretty good.
END { BAIL_OUT() unless test_history->test_was_successful }

I like that.

Regards,

[Michael G Schwern]

Post by Aristotle Pagaltzis

Post by Michael G Schwern
Which is a bit of a mouthful. Providing a function to get at the
history object would make a bunch of test state introspection easier.
# That looks pretty good.
END { BAIL_OUT() unless test_history->test_was_successful }

I like that.

Go at it.
https://github.com/schwern/test-more/issues/286

[James E Keenan]

Post by Michael G Schwern
Let me reiterate, I have no plans to *deprecate* `use_ok`. Even if I wanted
to there are simply too many users to make deprecation worth while.
It works fine if what you want is a runtime require + import + assert, and
sometimes you want that. The problem is it's been overused and has come to
replace a simple `use` in test scripts. To that end the question is whether
to *discourage* its use in the documentation: to scale it back from "use this
when you load a module" to "use this for some special cases".

+1 to discouraging use_ok

[Ruud H.G. van Tol]

Post by Ovid
eval "use $module";

Alternatively:

eval "use $module; 1"
or BAIL_OUT ( $@ // 'zombie error' );

[Leon Timmermans]

Post by Ovid
          Test::Trap::Builder::TempFile
          Test::Trap::Builder::SystemSafe
          Test::Trap::Builder
          Test::Trap
        );

I don't think that «if eval "use PerlIO; 1"» logic makes much sense.
It should return true on any perl 5.8+, even without PerlIO support
(not that you want to support that). If you must, use
$Config{useperlio}.

Leon

[Michael G Schwern]

Post by The Sidhekin
* How would you rewrite a test script such as my own
http://cpansearch.perl.org/src/EBHANSSEN/Test-Trap-v0.2.2/t/00-load.t so
that it does not use use_ok()?

use Test::More tests => 1;

use Test::Trap::Builder::TempFile;
use Test::Trap::Builder::SystemSafe;
SKIP: {
skip 'Lacking PerlIO', 1 unless eval "use PerlIO; 1";
require Test::Trap::Builder::PerlIO;
Test::Trap::Builder::PerlIO->import;
}
use Test::Trap::Builder;
require Test::Trap
or BAIL_OUT( "Nothing to test without the Test::Trap class" );

diag( "Testing Test::Trap $Test::Trap::VERSION, Perl $], $^X" );
pass("Test::Trap loaded successfully");

Or a slimmed down version, recognizing that Test::Trap loads all the above
already and will tell you just which module failed to load. It also makes the
test black box.

eval {
require Test::Trap;
Test::Trap->import;
1;
} or BAIL_OUT("Test::Trap failed to load");

diag( "Testing Test::Trap $Test::Trap::VERSION, Perl $], $^X" );
pass("Test::Trap loaded successfully");

This can be boxed up in a module. There's already Test::Requires. It skips
instead of bails, it would be a good target for test_must_have() or something
that bails.

Post by The Sidhekin
* Why would you? :-\

Because it reads like normal Perl and doesn't rely on more code to do
something rather simple: load a module.

TAP (the ok 1 stuff) has us very much convinced that we need to output
something for every assert. The rest of the testing universe relies on
exceptions to tell them something's failed and seems to get along just fine
with that.

But quite honestly, if use_ok() works for you, if all you need is require +
import + test, continue to use it. I don't plan on eliminating it.

[Michael G Schwern]

* I won't get to know if any of the other modules loaded correctly, or how
they fail. Less of the interesting output.
* And there will be no BAIL_OUT, so the rest of the tests will run, burying
the interesting output. More uninteresting output.

Like I said, if use_ok() is working for you keep using it. Your example where
you want to load a bunch of modules just to make sure they compile is one of
those cases where use_ok() is probably the right thing.

Post by The Sidhekin
* Why would you? :-\

Because it reads like normal Perl and doesn't rely on more code to do
something rather simple: load a module.
But it fails to DWIW: report clearly on failures. Perhaps what it is doing
is not so simple, after all?

Personally I'm a fan of "scroll up and read the first failure". It always works!

[Ovid]

----- Original Message -----

  But it fails to DWIW: report clearly on failures.  Perhaps what it is

doing

is not so simple, after all?

Personally I'm a fan of "scroll up and read the first failure". 
It always works!

Try it on a Test::Class test suite running thousands of test in a single process, whizzing past on your terminal :) 

Cheers,
Ovid
--
Live and work overseas - http://www.overseas-exile.com/
Buy the book - http://www.oreilly.com/catalog/perlhks/
Tech blog - http://blogs.perl.org/users/ovid/
Twitter - http://twitter.com/OvidPerl/

[Aristotle Pagaltzis]

Post by Ovid

Personally I'm a fan of "scroll up and read the first failure".  It
always works!

Try it on a Test::Class test suite running thousands of test in
a single process, whizzing past on your terminal :) 

That is not relevant to t/00-load.t though, nor applicable when BAIL_OUT
is involved, and here we are considering both of these.

Regards,

[Eirik Berg Hanssen]

Post by Ovid

Post by The Sidhekin
* How would you rewrite a test script such as my own
http://cpansearch.perl.org/src/EBHANSSEN/Test-Trap-v0.2.2/t/00-load.t so
that it does not use use_ok()?

use Test::More tests => 1;
use Test::Trap::Builder::TempFile;

*cut*

If this fails, the test script will terminate immediately:

* I won't get to know if any of the other modules loaded correctly, or how
they fail. Less of the interesting output.
* And there will be no BAIL_OUT, so the rest of the tests will run, burying
the interesting output. More uninteresting output.

Which are two very annoying changes in your rewrite. And of course,
there's more of the same in the part of the rewrite I cut.

Post by Ovid
* Why would you? :-\
Because it reads like normal Perl and doesn't rely on more code to do
something rather simple: load a module.

But it fails to DWIW: report clearly on failures. Perhaps what it is
doing is not so simple, after all?

Post by Ovid
But quite honestly, if use_ok() works for you, if all you need is require +
import + test, continue to use it. I don't plan on eliminating it.

Thank you.


Eirik

[Andy Lester]

So is there ANY legit use for use_ok()?

xoa

--
Andy Lester => ***@petdance.com => www.petdance.com => AIM:petdance

[Michael G Schwern]

Post by Andy Lester
So is there ANY legit use for use_ok()?

Yes. Sometimes you want to conditionally test if a module can be loaded and
its import does not blow up.

It's a convenience function so it can be more easily understood what's going
on and we don't each write it a million different ways. require_ok() solves a
big chunk of the problem.

if( something something ) {
use_ok 'Foo';

# Here's one long hand way...
# require_ok 'Foo';
# ok eval { Foo->import; 1; } or diag $@;

# or maybe this, hope you remember that 1;
# eval 'use Foo; 1;' or diag $@;
}

Similar to how like() is basically a convenience for...

my $regex = qr/bar/;
ok $foo =~ $regex or diag "$foo does not match $regex";

If it didn't already exist, it could probably be argued that it's not needed.
But it's there and can be left alone.

[Andy Lester]

Post by Michael G Schwern
It's a convenience function so it can be more easily understood what's going
on and we don't each write it a million different ways. require_ok() solves a
big chunk of the problem.
if( something something ) {
use_ok 'Foo';

So in these cases, we're using it basically as an eval block, because a simple "use Foo" would be bad.

What it sounds to me like is: "If all you're testing is that the module loads, and it must always, then simply do a use and do without the use_ok()".

In this example:

BEGIN {
use_ok( 'App::Ack' );
use_ok( 'App::Ack::Repository' );
use_ok( 'App::Ack::Resource' );
use_ok( 'File::Next' );
}
diag( "Testing App::Ack $App::Ack::VERSION, File::Next $File::Next::VERSION, Perl $], $^X" );

it sounds like we're saying that the use_ok() doesn't help at all, and I might as well write

use App::Ack;
use App::Ack::Repository;
use App::Ack::Resource;
use File::Next;
diag( "Testing App::Ack $App::Ack::VERSION, File::Next $File::Next::VERSION, Perl $], $^X" );

Agreed?

xoa

--
Andy Lester => ***@petdance.com => www.petdance.com => AIM:petdance

[chromatic]

Post by Michael G Schwern
BEGIN {
use_ok( 'App::Ack' );
use_ok( 'App::Ack::Repository' );
use_ok( 'App::Ack::Resource' );
use_ok( 'File::Next' );
}
diag( "Testing App::Ack $App::Ack::VERSION, File::Next $File::Next::VERSION, Perl $], $^X" );
it sounds like we're saying that the use_ok() doesn't help at all, and I might as well write
use App::Ack;
use App::Ack::Repository;
use App::Ack::Resource;
use File::Next;
diag( "Testing App::Ack $App::Ack::VERSION, File::Next $File::Next::VERSION, Perl $], $^X" );
Agreed?

Exactly. If any of those modules fail to load or compile or import, Perl will
give you the error at that point and stop the compilation (modulo any die
handler in effect). You can debug it from there.

-- c

[Michael G Schwern]

Post by Michael G Schwern
BEGIN {
use_ok( 'App::Ack' );
use_ok( 'App::Ack::Repository' );
use_ok( 'App::Ack::Resource' );
use_ok( 'File::Next' );
}
diag( "Testing App::Ack $App::Ack::VERSION, File::Next $File::Next::VERSION, Perl $], $^X" );
it sounds like we're saying that the use_ok() doesn't help at all, and I might as well write
use App::Ack;
use App::Ack::Repository;
use App::Ack::Resource;
use File::Next;
diag( "Testing App::Ack $App::Ack::VERSION, File::Next $File::Next::VERSION, Perl $], $^X" );
Agreed?

Yes, exactly.

[Michael G Schwern]

Post by Mike Doherty

Post by Paul Johnson

Post by Michael G Schwern
2. Should use_ok() be discouraged in the documentation?

I'm very much in favour of this.

I don't see any discouragement in the documentation...

We're discussing adding that discouragement.

Post by Mike Doherty
and what's wrong with use_ok to begin with?

That was the second half of my post. It's a lot of extra work for a version
of this that doesn't work quite right...

use Foo::Bar;
pass "Loaded Foo::Bar";

And the pass() is just informational. If Foo::Bar fails to load the test dies
which is a failure.

Post by Mike Doherty
I typically use_ok(...) or BAIL_OUT. If that's the only way to use
use_ok safely, then maybe it should do that for you automatically.

It cannot be a bail out, because there's nothing which says the module being
loaded is THE CRITICAL MODULE on which the whole test suite hangs.

It could die, but that introduces even more compatibility issues than the
lexical change. And it still leaves the problem of having to remember to wrap
it in a BEGIN block for it to truly emulate use properly.

Post by Mike Doherty
I think the ideal way to handle `use` would be a `bail_on_use_failure`
switch that intercepts `use` errors and turns them into BAIL_OUT,

Implementing this involves putting in a $SIG{__DIE__} and all the fun that
entails. Test::Builder got rid of its $SIG{__DIE__} to avoid interfering with
other code.

And it cannot be a bail out for the reasons above.

Post by Mike Doherty
Then `use_ok` would just completely deprecated.

I don't think it needs to be deprecated in the "issue a warning" sense. It's
not actively harmful to use, if its working for you go ahead and use it, it's
just kinda useless and not worth the effort of maintaining. I don't think
it's worth the effort to change all the hojillions of test suites that use it.
Documentation and a decision not to add more features should be enough.

[Andy Lester]

Post by Michael G Schwern
--
ROCKS FALL! EVERYONE DIES!
http://www.somethingpositive.net/sp05032002.shtml

I think we've found our correct non-loading module behavior right there.

xoa

--
Andy Lester => ***@petdance.com => www.petdance.com => AIM:petdance