Topic/demolish warnings #167
Conversation
t/bugs/DEMOLISH_warnings.t
Outdated
| use lib 't/lib'; | ||
| use Test::More tests => 1; | ||
|
|
||
| my @warnings; |
There was a problem hiding this comment.
We're using Test::Warnings in other tests. Might as well use it here too. That means you need to make it optional with Test::Requires but all the optional tests get run by travis so that's ok.
There was a problem hiding this comment.
I'm not entirely clear on how to write this test using Test::Warnings - some of this might be lack of familiarity on my end about when exactly Perl decides to display the warnings: when I replace use, with require, I don't see the DEMOLISH warning. And I don't see how to wrap warnings {} around a use statement - any suggestions?
There was a problem hiding this comment.
There are a few ways, but the easiest is probably just to load Test::Warnings, load the module in question, and then use had_no_warnings() to do the actual test.
| @@ -78,7 +78,10 @@ sub DEMOLISHALL { | |||
|
|
|||
| foreach my $class (@isa) { | |||
| no strict 'refs'; | |||
| my $demolish = *{"${class}::DEMOLISH"}{CODE}; | |||
| my $demolish = do { | |||
| no warnings 'once'; | |||
There was a problem hiding this comment.
A comment explaining why this is necessary would be good.
Per pull request moose#167, add comments better explaining why these changes are necessary, and use Test::Warnings
|
Alright, made those tweaks. Sorry for the delay - as noted in our other thread I was spending some time away from computers in California and Yosemite :) |
t/bugs/DEMOLISH_warnings.t
Outdated
| use warnings; | ||
|
|
||
| use lib 't/lib'; | ||
| use Test::More tests => 2; # Test::Warnings re-tests had_no_warnings implicitly |
There was a problem hiding this comment.
There's no need to put in a test count.
|
|
||
| use lib 't/lib'; | ||
| use Test::More tests => 2; # Test::Warnings re-tests had_no_warnings implicitly | ||
| use Test::Requires qw(Test::Warnings); |
There was a problem hiding this comment.
I think if you add Test::Warnings->import(':no_end_test') here then it will only run one test.
| # This variable is only in scope during the initial `use` | ||
| # As it leaves scope, Perl will call DESTROY on it | ||
| # (and Moose::Object will then go through its DEMOLISHALL method) | ||
| my $d = Demolition::Demolisher->new; |
There was a problem hiding this comment.
Could we move this into a block in the .t file? I think that'd make it a bit clearer.
jrubinator
force-pushed
the
topic/demolish_warnings
branch
from
1036e30
to
d17ebd8
Compare
Per pull request moose#167, add comments better explaining why these changes are necessary, and use Test::Warnings
jrubinator
force-pushed
the
topic/demolish_warnings
branch
from
d17ebd8
to
a0361cb
Compare
|
FYI I accidentally rewrote the commit history (I amended the first post-review commit), but realized that was silly and did some git surgery to put it back. The results should be what we want now. |
karenetheridge
force-pushed
the
master
branch
from
e79a7d8
to
d45b381
Compare
Fix for https://rt.cpan.org/Public/Bug/Display.html?id=124194
Single calls to demolish had the potential to emit warnings. This silences 'em.