Bug #48555 for Catalyst-Runtime: Don't localize %ENV

Sun Aug 09 08:22:12 2009 ron [...] rblasch.org - Ticket created

Subject:

Don't localize %ENV

t/unit_core_setup.t and t/unit_core_setup_stats.t localize %ENV. I don't know about other platforms, but Windows depends quite heavily on some environment variables, like 'Path' for loading libraries. Besides, few will work with a completely empty environment, so it's not really a good test if Catalyst works on the current system. With Catalyst-Runtime-5.80007, I get error messages like: "...site/lib/auto/HTML/Parser/Parser.dll' for module HTML::Parser: load_file:The referenced assembly is not installed on your system" Removing the two localizations makes the tests pass for me again. Test Summary Report ------------------- t/aggregate/live_engine_request_escaped_path.t (Wstat: 0 Tests: 6 Failed: 0) TODO passed: 4 Files=112, Tests=2387, 225 wallclock secs ( 0.77 usr + 0.41 sys = 1.17 CPU) Result: PASS

Sun Aug 09 08:38:15 2009 bobtfish [...] bobtfish.net - Taken

Sun Aug 09 08:40:41 2009 bobtfish [...] bobtfish.net - Correspondence added

Hiya. Thanks for the bug report. The issue with your proposed solution is that if someone has CATALYST_STATS set in their environment, then this will break the tests, ergo why I localized the environment in the first place ;) I have, however, made an alternate fix which should solve this issue for you. This was committed as r11068, I'd be very grateful if you could test trunk for me to verify it's fixed for you: http://dev.catalyst.perl.org/repos/Catalyst/Catalyst-Runtime/5.80/trunk Thanks is advance t0m

Sun Aug 09 08:40:43 2009 The RT System itself - Status changed from 'new' to 'open'

Sun Aug 09 08:41:12 2009 bobtfish [...] bobtfish.net - Status changed from 'open' to 'stalled'

Sun Aug 09 08:41:12 2009 bobtfish [...] bobtfish.net - Broken in 5.80007 added

Sun Aug 09 09:55:49 2009 ron [...] rblasch.org - Correspondence added

On Sun Aug 09 08:40:41 2009, BOBTFISH wrote: Show quoted text

> Hiya. > > Thanks for the bug report. > > The issue with your proposed solution is that if someone has > CATALYST_STATS set in their environment, then this will break the tests, > ergo why I localized the environment in the first place ;) > > I have, however, made an alternate fix which should solve this issue for > you. > > This was committed as r11068, I'd be very grateful if you could test > trunk for me to verify it's fixed for you: > > http://dev.catalyst.perl.org/repos/Catalyst/Catalyst-Runtime/5.80/trunk

I've tested r11069 and t/unit_core_setup_stats.t still fails with ".../site/lib/auto/Time/HiRes/HiRes.dll' for module Time::HiRes: load_file:The referenced assembly is not installed on your system" It's loaded by Catalyst::Stats. The tests output is: 1..5 Setting up mock application: UnusedApp Setting up mock application: TestNoStats The solution feels somewhat brittle. Would it be better to localize %ENV and change only as necessary (see attached patch)? If you'd prefer your way, I guess we'd need to pull in Catalyst::Stats near "mock_app('UnusedApp');". Ron

Index: t/unit_core_setup_stats.t =================================================================== --- t/unit_core_setup_stats.t (revision 11069) +++ t/unit_core_setup_stats.t (working copy) @@ -6,6 +6,25 @@ use Catalyst (); +sub my_env { + my (%add_env) = @_; + + # Clone environment + my %env = %ENV; + + # Remove all CATALYST env variables + for my $name (grep { /^CATALYST/ } keys %ENV) { + delete $env{$name}; + } + + # Add custom env variables + while (my ($name, $value) = each %add_env) { + $env{$name} = $value; + } + + return %env; +} + my %log_messages; # TODO - Test log messages as expected. my $mock_log = Class::MOP::Class->create_anon_class( methods => { @@ -29,12 +48,8 @@ return $meta->name; } -mock_app('UnusedApp'); # Mock an app before localizing %ENV - # to ensure that anything which is dynamically - # loaded from the enviornment is loaded +local %ENV = my_env(); # Clean -local %ENV; # Ensure blank or someone, somewhere will fail.. - { my $app = mock_app('TestNoStats'); $app->setup_stats(); @@ -52,13 +67,13 @@ ok $app->use_stats, 'debug on turns stats on'; } { - local %ENV = ( CATALYST_STATS => 1 ); + local %ENV = my_env( CATALYST_STATS => 1 ); my $app = mock_app('TestStatsAppStatsEnvSet'); $app->setup_stats(); ok $app->use_stats, 'ENV turns stats on'; } { - local %ENV = ( CATALYST_STATS => 0 ); + local %ENV = my_env( CATALYST_STATS => 0 ); my $app = mock_app('TestStatsAppStatsEnvUnset'); $app->meta->add_method('debug' => sub { 1 }); $app->setup_stats(1);

Sun Aug 09 09:55:51 2009 The RT System itself - Status changed from 'stalled' to 'open'

Sun Aug 09 10:23:12 2009 bobtfish [...] bobtfish.net - Correspondence added

Thanks. I haven't quite applied you patch as-is, but I went with that basic idea. Can you retest and confirm this is now fixed? Many thanks! t0m

Sun Aug 09 10:49:18 2009 ron [...] rblasch.org - Correspondence added

On Sun Aug 09 10:23:12 2009, BOBTFISH wrote: Show quoted text

> Thanks. I haven't quite applied you patch as-is, but I went with that > basic idea. > > Can you retest and confirm this is now fixed?

Tested r11070 on Windows XP SP3, Perl 5.10. Looks good. Test Summary Report ------------------- t/aggregate/live_engine_request_escaped_path.t (Wstat: 0 Tests: 6 Failed: 0) TODO passed: 4 Files=114, Tests=2506, 325 wallclock secs ( 0.73 usr + 0.59 sys = 1.33 CPU) Result: PASS Many thanks for fixing this! Ron

Sun Aug 09 11:17:21 2009 bobtfish [...] bobtfish.net - Correspondence added

On Sun Aug 09 10:49:18 2009, ron@rblasch.org wrote: Show quoted text

> Tested r11070 on Windows XP SP3, Perl 5.10. Looks good.

Great! Show quoted text

> Many thanks for fixing this!

Many thanks for reporting this :) Cheers t0m

Sun Aug 09 11:17:22 2009 bobtfish [...] bobtfish.net - Status changed from 'open' to 'resolved'