Skip to content
Browse files

Disable default-global-warnings behaviour.

  • Loading branch information...
1 parent 6935811 commit 18794a8565633ae08f048b9cf6c25b03be6e1020 @AndyA AndyA committed Apr 25, 2012
Showing with 16 additions and 1 deletion.
  1. +2 −0 MANIFEST
  2. +2 −0 MANIFEST.CUMMULATIVE
  3. +1 −0 MANIFEST.SKIP
  4. +1 −1 lib/Test/Harness.pm
  5. +10 −0 t/nowarn.t
View
2 MANIFEST
@@ -76,6 +76,7 @@ t/aggregator.t
t/bailout.t
t/base.t
t/callbacks.t
+t/compat/env.opts.t
t/compat/env.t
t/compat/failure.t
t/compat/inc-propagation.t
@@ -125,6 +126,7 @@ t/multiplexer.t
t/nested.t
t/nofork-mux.t
t/nofork.t
+t/nowarn.t
t/object.t
t/parse.t
t/parser-config.t
View
2 MANIFEST.CUMMULATIVE
@@ -156,6 +156,7 @@ t/compat/040-test-harness-compat.t
t/compat/060-version.t
t/compat/base.t
t/compat/callback.t
+t/compat/env.opts.t
t/compat/env.t
t/compat/failure.t
t/compat/from_line.t
@@ -220,6 +221,7 @@ t/multiplexer.t
t/nested.t
t/nofork-mux.t
t/nofork.t
+t/nowarn.t
t/object.t
t/parse.t
t/parser-config.t
View
1 MANIFEST.SKIP
@@ -40,3 +40,4 @@
^Session\.vim$
^MYMETA.yml$
^packaging/
+^MYMETA\.json$
View
2 lib/Test/Harness.pm
@@ -73,7 +73,7 @@ END {
$Verbose = $ENV{HARNESS_VERBOSE} || 0;
$Debug = $ENV{HARNESS_DEBUG} || 0;
-$Switches = '-w';
+$Switches = '';
$Columns = $ENV{HARNESS_COLUMNS} || $ENV{COLUMNS} || 80;
$Columns--; # Some shells have trouble with a full line of text.
$Timer = $ENV{HARNESS_TIMER} || 0;
View
10 t/nowarn.t
@@ -0,0 +1,10 @@
+#!perl
+
+use Test::More tests => 1;
+
+# Make sure that warnings are only enabled if we enable them
+# specifically.
+ok !$^W, 'warnings disabled';
+
+# vim:ts=2:sw=2:et:ft=perl
+

19 comments on commit 18794a8

@schwern
Perl Toolchain Gang member

This is a huge backwards compatibility change, done without discussion and right in the face of https://rt.cpan.org/Ticket/Display.html?id=58229. I'm going to revert it, then we can discuss it.

@karenetheridge
Perl Toolchain Gang member

Okay, where's the discussion? This was a very much needed change, and should stay.

@shadowcat-mst
Perl Toolchain Gang member

It's been discussed on irc.perl.org repeatedly; general consensus among the Catalyst, DBIx::Class, Moose and #toolchain community has been that it's a dinosaur thing and really needs to go away.

I've been trying to get people to turn that consensus into public discussion for a while, but kept running into "if we make the change schwern will just unilaterally revert it", which I considered to be more about schwern's reputation than any reasonable expectation of unilateral action in this case.

Apparently they were right and I was wrong about that.

Schwern, if you want a discussion of this, please start one and I'll try and convince people they should turn up to it. Otherwise, please can we have our feature back?

@miyagawa
Perl Toolchain Gang member

I very much support this change.

In RT ticket a prove option was suggested, but prove doesn't enable the global warnings by default anyway, unless you set -w. This default annoys me when running the test with make test.

@Leont
Perl Toolchain Gang member
Leont commented on 18794a8 Jun 16, 2013

I agree with the rest, -w is no longer a sensible default unless you're targetting perl releases older than 5.6.

@ewilhelm
Perl Toolchain Gang member
@ribasushi
Perl Toolchain Gang member

I seem to be the minority - but without further data I tend to side with Schwern. I've read the little available material there is, and would like to argue this point further.

For starters - what is the actual problem we are trying to solve with this change, apart from "OH NOES! IT'S NOISY!!!"?

@miyagawa
Perl Toolchain Gang member

@ewilhelm like this?

diff --git a/lib/ExtUtils/Command/MM.pm b/lib/ExtUtils/Command/MM.pm
index dcedd2c..4704ad9 100644
--- a/lib/ExtUtils/Command/MM.pm
+++ b/lib/ExtUtils/Command/MM.pm
@@ -51,6 +51,7 @@ sub test_harness {
     require File::Spec;

     $Test::Harness::verbose = shift;
+    $Test::Harness::switches = '';

     # Because Windows doesn't do this for us and listing all the *.t files
     # out on the command line can blow over its exec limit.

It seems to fix the issue specifically on MakeMaker, but i'm not sure what the others intention was, since that might include other use cases as well.

@miyagawa
Perl Toolchain Gang member

For starters - what is the actual problem we are trying to solve with this change,

It enables warnings globally, to cause warnings where it isn't supposed to when running on non-test environment, and that forces authors to add no warnings when it's not necessary.

Nobody would disagree that adding warnings everywhere (and opting out with no warnings in a narrow scope) is a good practice, but enabling it globally in the tests to change the behavior of the code in the tests is a completely separate story.

@shadowcat-mst
Perl Toolchain Gang member
@ewilhelm
Perl Toolchain Gang member
@shadowcat-mst
Perl Toolchain Gang member
@ribasushi
Perl Toolchain Gang member
It alters global interpreter behaviour for a purpose that ceased to exist
when lexical warnings were introduced.

Additionally, it can mean that a warning in a dependency that didn't declare
'use warnings;' because they explicitly didn't want to can disrupt SIGWARN
based testing and cause false failures.

Right... Still - where is the downside? Extra warnings won't actually hurt anything. Yes it will be noisy. Yes there may be failures of imroperly written tests (globally hooking SIGWARN and expecting a continuously warn-free depchain is nothing short of insane).

On the upside, with this altered interpreter state warnings are visible, regardless of the sentiments of a particular dependency author. As you aptly noted - one can lexically counter the effects of -w in scopes where it really matters.

To me -w has always been a tool to unearth malice-due-to-laziness which is ripe on CPAN these days. It would be a shame to lose this tool to "winds of change" which are not rooted in anything resembling technical reality.

@Leont
Perl Toolchain Gang member
Leont commented on 18794a8 Jun 17, 2013

IIRC, Module::Build has --use-tap-harness or something, which should probably just be the default these days.

That's currently ignoring HARNESS_OPTIONS though. That logic should probably be split out of Test::Harness and be made available for other TAP::Harness users.

@shadowcat-mst
Perl Toolchain Gang member
@ribasushi
Perl Toolchain Gang member
@shadowcat-mst
Perl Toolchain Gang member
@Leont
Perl Toolchain Gang member
Leont commented on 18794a8 Jul 4, 2013

It alters global interpreter behaviour for a purpose that ceased to exist when lexical warnings were introduced.

The purpose ceased to exist when Test::Harness learned to parse the arguments to perl on the hashbang line. Can't be bothered to do the archeology into the early 90's right now, but that may mean it never had a purpose.

@Leont
Perl Toolchain Gang member
Leont commented on 18794a8 Jul 17, 2013

This patch seems to cause warnings in Module::Build. IMO, that's M::B being stupid (it sets $switches and $Switches to undef if they are an empty string, them being aliased is causing a Use of uninitialized value in length warning). This may also be relevant if we want to add a use warnings to Test::Harness, which I'd like to do shortly.

Please sign in to comment.
Something went wrong with that request. Please try again.