Bug #111143 for Log-Dispatch: 08-screen.t fails with PERL

Wed Jan 13 00:38:40 2016 cjm [...] cpan.org - Ticket created

Subject:

08-screen.t fails with PERL_UNICODE and 5.22.1

With Perl 5.22.1, LANG=en_US.UTF-8, and PERL_UNICODE=SAL, 08-screen.t fails. It succeeds if I set either LANG=C or PERL_UNICODE=0. With Perl 5.20.2, the test passes even with PERL_UNICODE=SAL. I've attached the result of running perl5.22.1 t/00-report-prereqs.t &>/tmp/00-report-prereqs.txt along with the results of 00-report-prereqs.t for both 5.20.2 and 5.22.1.

Subject:

08-screen.txt

# testing ok 1 - STDOUT layers are not changed when Screen utf8 param is true # Subtest: stderr = 0, utf8 = 0 ok 1 - got expected stdout from Screen output ok 2 - got expected stderr from Screen output 1..2 ok 2 - stderr = 0, utf8 = 0 # Subtest: stderr = 1, utf8 = 0 ok 1 - got expected stdout from Screen output ok 2 - got expected stderr from Screen output 1..2 ok 3 - stderr = 1, utf8 = 0 # Subtest: stderr = 0, utf8 = 1 not ok 1 - got expected stdout from Screen output # Failed test 'got expected stdout from Screen output' # at t/08-screen.t line 97. # got: 'test message - á½ ' # expected: 'test message - ὠ' ok 2 - got expected stderr from Screen output 1..2 # Looks like you failed 1 test of 2. not ok 4 - stderr = 0, utf8 = 1 # Failed test 'stderr = 0, utf8 = 1' # at t/08-screen.t line 106. # Subtest: stderr = 1, utf8 = 1 ok 1 - got expected stdout from Screen output not ok 2 - got expected stderr from Screen output # Failed test 'got expected stderr from Screen output' # at t/08-screen.t line 103. # got: 'test message - á½ ' # expected: 'test message - ὠ' 1..2 # Looks like you failed 1 test of 2. not ok 5 - stderr = 1, utf8 = 1 # Failed test 'stderr = 1, utf8 = 1' # at t/08-screen.t line 106. 1..5 # Looks like you failed 2 tests of 5.

Wed Jan 13 00:39:29 2016 cjm [...] cpan.org - Correspondence added

Subject:

00-report-prereqs-5.20.2.txt

1..1 # # Versions for all modules listed in static metadata (including optional ones): # # === Configure Requires === # # Module Want Have # -------------------- ---- ---- # Dist::CheckConflicts 0.02 0.11 # ExtUtils::MakeMaker any 6.98 # # === Test Requires === # # Module Want Have # ------------------- ---- -------- # Data::Dumper any 2.154 # Exporter any 5.71 # ExtUtils::MakeMaker any 6.98 # File::Spec any 3.48_01 # File::Temp any 0.2304 # FindBin any 1.51 # Getopt::Long any 2.42 # IO::File any 1.16 # IPC::Run3 any 0.048 # PerlIO any 1.09 # Test::Fatal any 0.014 # Test::More 0.96 1.001014 # Test::Requires any 0.08 # lib any 0.63 # utf8 any 1.13_01 # # === Test Recommends === # # Module Want Have # ---------- -------- -------- # CPAN::Meta 2.120900 2.143240 # # === Runtime Requires === # # Module Want Have # ------------------------ ---- ------ # Carp any 1.3301 # Devel::GlobalDestruction any 0.13 # Dist::CheckConflicts 0.02 0.11 # Encode any 2.70 # Fcntl any 1.11 # IO::Handle any 1.35 # Module::Runtime any 0.014 # Params::Validate 1.03 1.18 # Scalar::Util any 1.41 # Sys::Syslog 0.28 0.33 # base any 2.22 # strict any 1.08 # warnings any 1.23 # ok 1

Subject:

00-report-prereqs-5.22.1.txt

1..1 # # Versions for all modules listed in static metadata (including optional ones): # # === Configure Requires === # # Module Want Have # -------------------- ---- ------- # Dist::CheckConflicts 0.02 0.11 # ExtUtils::MakeMaker any 7.04_01 # # === Test Requires === # # Module Want Have # ------------------- ---- -------- # Data::Dumper any 2.158 # Exporter any 5.72 # ExtUtils::MakeMaker any 7.04_01 # File::Spec any 3.56 # File::Temp any 0.2304 # FindBin any 1.51 # Getopt::Long any 2.45 # IO::File any 1.16 # IPC::Run3 any 0.048 # PerlIO any 1.09 # Test::Fatal any 0.014 # Test::More 0.96 1.001014 # Test::Requires any 0.10 # lib any 0.63 # utf8 any 1.17 # # === Test Recommends === # # Module Want Have # ---------- -------- -------- # CPAN::Meta 2.120900 2.150001 # # === Runtime Requires === # # Module Want Have # ------------------------ ---- ----- # Carp any 1.36 # Devel::GlobalDestruction any 0.13 # Dist::CheckConflicts 0.02 0.11 # Encode any 2.72 # Fcntl any 1.13 # IO::Handle any 1.35 # Module::Runtime any 0.014 # Params::Validate 1.03 1.21 # Scalar::Util any 1.41 # Sys::Syslog 0.28 0.33 # base any 2.22 # strict any 1.09 # warnings any 1.34 # ok 1

Wed Jan 13 01:00:55 2016 ether [...] cpan.org - Correspondence added

relevant: http://www.dagolden.com/index.php/1771/why-perl_unicode-makes-me-sad/

Wed Jan 13 01:00:56 2016 The RT System itself - Status changed from 'new' to 'open'

Wed Jan 13 01:09:39 2016 cjm [...] cpan.org - Correspondence added

On Wed, Jan 13, 2016 12:00:55 AM, ETHER wrote: Show quoted text

> relevant: http://www.dagolden.com/index.php/1771/why-perl_unicode-makes-me-sad/

Possibly, although I don't use D, just S(tandard IO handles) and A(rgv). (L means that SA applies only if the locale is UTF-8, which is why LANG=C makes the test pass.)

Wed Jan 13 01:16:27 2016 DROLSKY [...] cpan.org - Correspondence added

On Wed Jan 13 01:09:39 2016, CJM wrote: Show quoted text

> On Wed, Jan 13, 2016 12:00:55 AM, ETHER wrote:

> > relevant: http://www.dagolden.com/index.php/1771/why-perl_unicode- > > makes-me-sad/

> > Possibly, although I don't use D, just S(tandard IO handles) and > A(rgv). (L means that SA applies only if the locale is UTF-8, which > is why LANG=C makes the test pass.)

It'd probably the S causing the problem. That said, I don't think running arbitrary tests with env vars that change the Perl interpreter's behavior should be expected to work.

Wed Jan 13 01:26:59 2016 cjm [...] cpan.org - Correspondence added

As xdg mentions in that article, it's useful to test modules the way they'll be used. I'm of the opinion that a test should clean up its environment if it's doing something tricky that breaks if certain variables are set. In this case, the attached patch will work.

Subject:

08-screen.patch

--- t/08-screen.t 2015-09-19 12:00:02.000000000 -0500 +++ t/08-screen.t 2016-01-13 00:18:10.053658763 -0600 @@ -16,6 +16,8 @@ use Log::Dispatch; use PerlIO; +delete $ENV{PERL_UNICODE}; + { my @orig_layers = PerlIO::get_layers(STDOUT);

Wed Jan 13 10:08:48 2016 dagolden [...] cpan.org - Correspondence added

On Wed Jan 13 01:16:27 2016, DROLSKY wrote: Show quoted text

> That said, I don't think running arbitrary tests with env vars that > change the Perl interpreter's behavior should be expected to work.

People put all sorts of stuff in their environment, so tests ought to reset any that cause problems. This is no different than PERL5LIB, PERL5OPT, etc. Or setting up a temporary HOME directly to avoid automatically picking up config files. PERL_UNICODE is just a particularly uncommon one. :-/

Wed Jan 13 10:11:05 2016 DROLSKY [...] cpan.org - Correspondence added

On Wed Jan 13 10:08:48 2016, DAGOLDEN wrote: Show quoted text

> On Wed Jan 13 01:16:27 2016, DROLSKY wrote:

> > That said, I don't think running arbitrary tests with env vars that > > change the Perl interpreter's behavior should be expected to work.

> > People put all sorts of stuff in their environment, so tests ought to > reset any that cause problems. This is no different than PERL5LIB, > PERL5OPT, etc. Or setting up a temporary HOME directly to avoid > automatically picking up config files. > > PERL_UNICODE is just a particularly uncommon one. :-/

This is something that should be done in Test::More or something like that, then. Do we really expect every CPAN module to add boilerplate to every test to reset every possible env var that someone might set? I'm of course fine with patching this test now that we know the issue and the simple fix, but as a general rule I think that if you run tests with random perl env vars set, you should expect random failures.

Wed Jan 13 12:50:36 2016 ether [...] cpan.org - Correspondence added

On 2016-01-13 07:11:05, DROLSKY wrote: Show quoted text

> This is something that should be done in Test::More or something like > that, then. Do we really expect every CPAN module to add boilerplate > to every test to reset every possible env var that someone might set? > > I'm of course fine with patching this test now that we know the issue > and the simple fix, but as a general rule I think that if you run > tests with random perl env vars set, you should expect random > failures.

Having Test::More reset the variables entirely defeats the point of being able to test a CPAN module working with particular variables set -- which is something an application may wish to do (providing, of course, that all the CPAN modules it uses actually *does* work with those variables, which is what this patch is about). This isn't a random variable -- it's one that affects the behaviour of filehandle operations -- and so it's not unreasonable to try to make this module work when the variable is set to various values. I see it as similar to making a module work in various locales, or with different filesystems. It may be an implementation variant that the author hadn't thought of, or he doesn't have configured by default on his development system, but that's why there are smokers, as well as a large contingent of helpful users that send patches when edge cases are discovered :D (I'm also now pondering which of my modules do interesting things with filehandles that might need testing with PERL_UNICODE...)

Wed Jan 13 16:16:21 2016 DROLSKY [...] cpan.org - Correspondence added

On Wed Jan 13 12:50:36 2016, ETHER wrote: Show quoted text

> > Having Test::More reset the variables entirely defeats the point of > being able to test a CPAN module working with particular variables set > -- > which is something an application may wish to do (providing, of > course, > that all the CPAN modules it uses actually *does* work with those > variables, > which is what this patch is about).

Good point, except that this patch explicitly breaks your ability to test this module with the env var in question! In fact, it will mislead you into thinking that this will work when it fact it will not. Show quoted text

> This isn't a random variable -- it's one that affects the behaviour of > filehandle operations -- and so it's not unreasonable to try to make > this > module work when the variable is set to various values.

There's a limit to how many different environments a module can be expected to work in. An environment that globally messes with every single filehandle is likely to break a lot of CPAN modules. This seems like one of those "use at your own risk or with lots of testing things". That said, if someone wants to patch the module (not the test) to make it actually do the right thing here, I would happily apply such a patch. I'm not sure exactly what the right thing is or why it breaks though. I'm guessing there some sort of double encoding issue going on. Also, this is testing a "utf8" option that you would want to _not_ pass if you set this global env var in all your code. So maybe just a doc patch?

Sat Feb 27 11:52:21 2016 MAUKE [...] cpan.org - Correspondence added

On Wed Jan 13 16:16:21 2016, DROLSKY wrote: Show quoted text

> On Wed Jan 13 12:50:36 2016, ETHER wrote:

> > > > Having Test::More reset the variables entirely defeats the point of > > being able to test a CPAN module working with particular variables > > set > > -- > > which is something an application may wish to do (providing, of > > course, > > that all the CPAN modules it uses actually *does* work with those > > variables, > > which is what this patch is about).

> > Good point, except that this patch explicitly breaks your ability to > test this module with the env var in question! In fact, it will > mislead you into thinking that this will work when it fact it will > not. >

> > This isn't a random variable -- it's one that affects the behaviour > > of > > filehandle operations -- and so it's not unreasonable to try to make > > this > > module work when the variable is set to various values.

> > There's a limit to how many different environments a module can be > expected to work in. An environment that globally messes with every > single filehandle is likely to break a lot of CPAN modules. This seems > like one of those "use at your own risk or with lots of testing > things". > > That said, if someone wants to patch the module (not the test) to make > it actually do the right thing here, I would happily apply such a > patch. I'm not sure exactly what the right thing is or why it breaks > though. I'm guessing there some sort of double encoding issue going > on. > > Also, this is testing a "utf8" option that you would want to _not_ > pass if you set this global env var in all your code. So maybe just a > doc patch?

I don't know about this problen but there's a kind of similar bug in DBI: https://rt.cpan.org/Ticket/Display.html?id=71341 That one is caused by sending binary data over STDIN/STDOUT without calling binmode() on them. Which breaks with PERL_UNICODE=S because that makes the STD* handles start out as ":utf8".

Sun Nov 13 22:35:46 2016 DROLSKY [...] cpan.org - Correspondence added

Ticket migrated to GitHub as https://github.com/houseabsolute/Log-Dispatch/issues/32

Sun Nov 13 22:35:53 2016 DROLSKY [...] cpan.org - Status changed from 'open' to 'resolved'

Bug #111143 for Log-Dispatch: 08-screen.t fails with PERL_UNICODE and 5.22.1

Preferred bug tracker