Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Multiple testsuite fixup #16149

Closed
p5pRT opened this issue Sep 14, 2017 · 31 comments
Closed

Multiple testsuite fixup #16149

p5pRT opened this issue Sep 14, 2017 · 31 comments

Comments

@p5pRT
Copy link

p5pRT commented Sep 14, 2017

Migrated from rt.perl.org#132092 (status was 'resolved')

Searchable as RT132092$

@p5pRT
Copy link
Author

p5pRT commented Sep 14, 2017

From @atoomic

This is a bug report for perl from atoomic@​cpan.org,
generated with the help of perlbug 1.40 running under perl 5.24.1.


This ticket is about updating a few unit tests for several reasons​:

1 - use set_up_inc helper to set @​INC
2 - remove useless setting in BEGIN
3 - do not use B which is a reserved namespace
4 - do not run test output at compilation time
5 - can run B with the test t/op/stash.t
6 - t/op/symbolcache.t should return true

Each idea comes with its own commit as some might be more difficult to
other to sell :-)

Commits can be read online here​:
https://github.com/atoomic/perl5/commits/devel-tests-26.0



Flags​:
  category=core
  severity=low


Site configuration information for perl 5.24.1​:

Configured by cPanel at Mon Aug 21 10​:13​:10 CDT 2017.

Summary of my perl5 (revision 5 version 24 subversion 1) configuration​:

  Platform​:
  osname=linux, osvers=3.10.0-123.20.1.el7.x86_64,
archname=x86_64-linux-64int
  uname='linux rpmbuild-64-centos-7.dev.cpanel.net
3.10.0-123.20.1.el7.x86_64 #1 smp thu jan 29 18​:05​:33 utc 2015 x86_64
x86_64 x86_64 gnulinux '
  config_args='-des -Dusedevel -Darchname=x86_64-linux-64int
-Dcc=/usr/bin/gcc -Dcpp=/usr/bin/cpp -Dusemymalloc=n -DDEBUGGING=none
-Doptimize=-Os -Accflags=-m64 -Dccflags=-DPERL_DISABLE_PMC -fPIC -DPIC
-I/usr/local/cpanel/3rdparty/perl/524/include
-L/usr/local/cpanel/3rdparty/perl/524/lib64
-I/usr/local/cpanel/3rdparty/include -L/usr/local/cpanel/3rdparty/lib64
-Duseshrplib -Duselargefiles=yes -Duseposix=true -Dhint=recommended
-Duseperlio=yes -Dcppflags=-I/usr/local/cpanel/3rdparty/perl/524/include
-L/usr/local/cpanel/3rdparty/perl/524/lib64
-I/usr/local/cpanel/3rdparty/include -L/usr/local/cpanel/3rdparty/lib64
-Dldflags=-Wl,-rpath -Wl,/usr/local/cpanel/3rdparty/perl/524/lib64
-L/usr/local/cpanel/3rdparty/perl/524/lib64
-L/usr/local/cpanel/3rdparty/lib64
-Dprefix=/usr/local/cpanel/3rdparty/perl/524
-Dsiteprefix=/opt/cpanel/perl5/524 -Dsitebin=/opt/cpanel/perl5/524/bin
-Dsitelib=/opt/cpanel/perl5/524/site_lib -Dusevendorprefix=true
-Dvendorbin=/usr/local/cpanel/3rdparty/perl/524/bin
-Dvendorprefix=/usr/local/cpanel/3rdparty/perl/524/lib64/perl5
-Dvendorlib=/usr/local/cpanel/3rdparty/perl/524/lib64/perl5/cpanel_lib
-Dprivlib=/usr/local/cpanel/3rdparty/perl/524/lib64/perl5/5.24.1
-Dman1dir=none -Dman3dir=none
-Dscriptdir=/usr/local/cpanel/3rdparty/perl/524/bin
-Dscriptdirexp=/usr/local/cpanel/3rdparty/perl/524/bin -Dsiteman1dir=none
-Dsiteman3dir=none -Dinstallman1dir=none -Dversiononly=no
-Dinstallusrbinperl=no -Dcf_by=cPanel -Dmyhostname=localhost
-Dperladmin=root@​localhost -Dcf_email=support@​cpanel.net
-Di_dbm=/usr/local/cpanel/3rdparty/include
-Di_gdbm=/usr/local/cpanel/3rdparty/include
-Di_ndbm=/usr/local/cpanel/3rdparty/include -DDB_File=true -Ud_dosuid
-Uuserelocatableinc -Umad -Uusethreads -Uusemultiplicity -Uusesocks
-Uuselongdouble -Aldflags=-L/usr/local/cpanel/3rdparty/perl/524/lib64
-L/usr/local/cpanel/3rdparty/lib64 -L/usr/lib64 -L/lib64 -lgdbm
-Dlocincpth=/usr/local/cpanel/3rdparty/perl/524/include
/usr/local/cpanel/3rdparty/include /usr/local/include -Duse64bitint
-Uuse64bitall -Dlibpth=/usr/local/cpanel/3rdparty/perl/524/lib64
/usr/local/cpanel/3rdparty/lib64 /usr/local/lib64 /usr/local/lib /lib64
/usr/lib64 '
  hint=recommended, useposix=true, d_sigaction=define
  useithreads=undef, usemultiplicity=undef
  use64bitint=define, use64bitall=undef, uselongdouble=undef
  usemymalloc=n, bincompat5005=undef
  Compiler​:
  cc='/usr/bin/gcc', ccflags ='-DPERL_DISABLE_PMC -fPIC -DPIC
-I/usr/local/cpanel/3rdparty/perl/524/include
-L/usr/local/cpanel/3rdparty/perl/524/lib64
-I/usr/local/cpanel/3rdparty/include -L/usr/local/cpanel/3rdparty/lib64
-m64 -fwrapv -fno-strict-aliasing -pipe -fstack-protector-strong
-I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64
-D_FORTIFY_SOURCE=2',
  optimize='-Os',
  cppflags='-I/usr/local/cpanel/3rdparty/perl/524/include
-L/usr/local/cpanel/3rdparty/perl/524/lib64
-I/usr/local/cpanel/3rdparty/include -L/usr/local/cpanel/3rdparty/lib64
-DPERL_DISABLE_PMC -fPIC -DPIC
-I/usr/local/cpanel/3rdparty/perl/524/include
-L/usr/local/cpanel/3rdparty/perl/524/lib64
-I/usr/local/cpanel/3rdparty/include -L/usr/local/cpanel/3rdparty/lib64
-m64 -fwrapv -fno-strict-aliasing -pipe -fstack-protector-strong
-I/usr/local/include'
  ccversion='', gccversion='4.8.2 20140120 (Red Hat 4.8.2-16)',
gccosandvers=''
  intsize=4, longsize=8, ptrsize=8, doublesize=8, byteorder=12345678,
doublekind=3
  d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=16,
longdblkind=3
  ivtype='long', ivsize=8, nvtype='double', nvsize=8, Off_t='off_t',
lseeksize=8
  alignbytes=8, prototype=define
  Linker and Libraries​:
  ld='/usr/bin/gcc', ldflags ='-Wl,-rpath
-Wl,/usr/local/cpanel/3rdparty/perl/524/lib64
-L/usr/local/cpanel/3rdparty/perl/524/lib64
-L/usr/local/cpanel/3rdparty/lib64
-L/usr/local/cpanel/3rdparty/perl/524/lib64
-L/usr/local/cpanel/3rdparty/lib64 -L/usr/lib64 -L/lib64 -lgdbm
-fstack-protector-strong -L/usr/local/lib'
  libpth=/usr/local/cpanel/3rdparty/perl/524/lib64
/usr/local/cpanel/3rdparty/lib64 /usr/local/lib64 /usr/local/lib /lib64
/usr/lib64 /usr/local/lib /usr/lib /lib/../lib64 /usr/lib/../lib64 /lib
  libs=-lpthread -lnsl -lgdbm -ldb -ldl -lm -lcrypt -lutil -lc
-lgdbm_compat
  perllibs=-lpthread -lnsl -ldl -lm -lcrypt -lutil -lc
  libc=libc-2.17.so, so=so, useshrplib=true, libperl=libperl.so
  gnulibc_version='2.17'
  Dynamic Linking​:
  dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='-Wl,-E
-Wl,-rpath,/usr/local/cpanel/3rdparty/perl/524/lib64/perl5/5.24.1/x86_64-linux-64int/CORE'
  cccdlflags='-fPIC', lddlflags='-shared -Os
-L/usr/local/cpanel/3rdparty/perl/524/lib64
-L/usr/local/cpanel/3rdparty/lib64 -L/usr/lib64 -L/lib64 -L/usr/local/lib
-fstack-protector-strong'

Locally applied patches​:
  RC3
  cPanel patches
  cPanel INC path changes
  Remove . from @​INC


@​INC for perl 5.24.1​:
  /root/.dotfiles/perl-must-have/lib
  /root/perl5/lib/perl5/
  /usr/local/cpanel

/usr/local/cpanel/3rdparty/perl/524/lib64/perl5/cpanel_lib/x86_64-linux-64int
  /usr/local/cpanel/3rdparty/perl/524/lib64/perl5/cpanel_lib

/usr/local/cpanel/3rdparty/perl/524/lib64/perl5/5.24.1/x86_64-linux-64int
  /usr/local/cpanel/3rdparty/perl/524/lib64/perl5/5.24.1
  /opt/cpanel/perl5/524/site_lib/x86_64-linux-64int
  /opt/cpanel/perl5/524/site_lib


Environment for perl 5.24.1​:
  HOME=/root
  LANG=en_US.UTF-8
  LANGUAGE (unset)
  LD_LIBRARY_PATH (unset)
  LOGDIR (unset)

PATH=/usr/local/cpanel/3rdparty/perl/526/bin​:/usr/local/cpanel/3rdparty/perl/524/bin​:/usr/local/cpanel/3rdparty/perl/522/bin​:/usr/local/cpanel/3rdparty/perl/514/bin​:/usr/local/cpanel/3rdparty/bin​:/root/bin/​:/opt/local/bin​:/opt/local/sbin​:/usr/local/sbin​:/usr/local/bin​:/usr/sbin​:/usr/bin​:/opt/cpanel/composer/bin​:/root/.dotfiles/bin​:/root/perl5/bin​:/root/.rvm/bin​:/root/bin
  PERL5DB=use Devel​::NYTProf
  PERL5LIB=/root/.dotfiles/perl-must-have/lib​::/root/perl5/lib/perl5/
  PERL_BADLANG (unset)
  PERL_CPANM_OPT=--quiet
  SHELL=/bin/bash

@p5pRT
Copy link
Author

p5pRT commented Sep 14, 2017

From @atoomic

0005-test-Can-run-B-with-the-test.patch
From 09dbc2b5bb41cbbb14e656fdf16cb1382aa77ac0 Mon Sep 17 00:00:00 2001
From: Nicolas R <atoomic@cpan.org>
Date: Thu, 14 Sep 2017 14:52:59 -0600
Subject: [PATCH 5/6] test - Can run B with the test

This test is using B for testing purpose.
But if we run this test using a B module,
this is going to raise some errors.

Adjust the test to be compliant with B, while
preserving the original logic.
---
 t/op/stash.t | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/t/op/stash.t b/t/op/stash.t
index c9634a370a..b4fe67fd48 100644
--- a/t/op/stash.t
+++ b/t/op/stash.t
@@ -63,9 +63,9 @@ SKIP: {
 }
 
 SKIP: {
-    eval { require B; 1 } or skip "no B", 29;
+    eval q{ delete $INC{'B.pm'}; require B; 1 } or skip "no B", 29;
 
-    *b = \&B::svref_2object;
+    *b = 'B'->can('svref_2object');
     my $CVf_ANON = B::CVf_ANON();
 
     my $sub = do {
@@ -110,7 +110,7 @@ SKIP: {
     };
     %four:: = ();
 
-    my $gv = B::svref_2object($sub)->GV;
+    my $gv = 'B'->can('svref_2object')->($sub)->GV;
     ok($gv->isa(q/B::GV/), "cleared stash leaves anon CV with valid GV");
 
     my $st = eval { $gv->STASH->NAME };
@@ -122,7 +122,7 @@ SKIP: {
     };
     undef %five::;
 
-    $gv = B::svref_2object($sub)->GV;
+    $gv = 'B'->can('svref_2object')->($sub)->GV;
     ok($gv->isa(q/B::GV/), "undefed stash leaves anon CV with valid GV");
 
     $st = eval { $gv->STASH->NAME };
@@ -137,7 +137,7 @@ SKIP: {
     my $stash_glob = delete $::{"six::"};
     # Now free the GV while the stash still exists (though detached)
     delete $$stash_glob{"six"};
-    $gv = B::svref_2object($sub)->GV;
+    $gv = 'B'->can('svref_2object')->($sub)->GV;
     ok($gv->isa(q/B::GV/),
        'anonymised CV whose stash is detached still has a GV');
     is $gv->STASH->NAME, '__ANON__',
@@ -150,7 +150,7 @@ SKIP: {
 	my $rfoo = \&foo;
 	package main;
 	delete $::{'FOO::'};
-	my $cv = B::svref_2object($rfoo);
+	my $cv = 'B'->can('svref_2object')->($rfoo);
 	# (is there a better way of testing for NULL ?)
 	my $stash = $cv->STASH;
 	like($stash, qr/B::SPECIAL/, "NULL CvSTASH on named sub");
@@ -167,7 +167,7 @@ SKIP: {
 	    *f = sub {};
 	];
 	delete $FOO2::{f};
-	my $cv = B::svref_2object($r);
+	my $cv = 'B'->can('svref_2object')->($r);
 	my $gv = $cv->GV;
 	ok($gv->isa(q/B::GV/), "orphaned CV has valid GV");
 	is($gv->NAME, '__ANON__', "orphaned CV has anon GV");
@@ -185,12 +185,12 @@ SKIP: {
 
 	delete $FOO3::{__ANON__}; # whoops!
 	my ($cv,$gv);
-	$cv = B::svref_2object($named);
+	$cv = 'B'->can('svref_2object')->($named);
 	$gv = $cv->GV;
 	ok($gv->isa(q/B::GV/), "ex-named CV has valid GV");
 	is($gv->NAME, '__ANON__', "ex-named CV has anon GV");
 
-	$cv = B::svref_2object($anon);
+	$cv = 'B'->can('svref_2object')->($anon);
 	$gv = $cv->GV;
 	ok($gv->isa(q/B::GV/), "anon CV has valid GV");
 	is($gv->NAME, '__ANON__', "anon CV has anon GV");
@@ -206,7 +206,7 @@ SKIP: {
 	    }
 	}
 
-	my $br = B::svref_2object($r);
+	my $br = 'B'->can('svref_2object')->($r);
 	is ($br->STASH->NAME, 'bloop',
 	    'stub records the package it was compiled in');
 	# Arguably this shouldn't quite be here, but it's easy to add it
@@ -216,7 +216,7 @@ SKIP: {
 
 	# We need to take this reference "late", after the subroutine is
 	# defined.
-	$br = B::svref_2object(eval 'sub whack {}; \&whack');
+	$br = 'B'->can('svref_2object')->(eval 'sub whack {}; \&whack');
 	die $@ if $@;
 
 	is ($br->STASH->NAME, 'main',
@@ -256,7 +256,7 @@ fresh_perl_is(
 
     # effectively rename a stash
     *slin:: = *rile::; *rile:: = *zor::;
-    
+
     ::is *$globref, "*rile::tat",
      'globs stringify the same way when stashes are moved';
     ::is ref $obj, "rile",
-- 
2.14.1

@p5pRT
Copy link
Author

p5pRT commented Sep 14, 2017

From @atoomic

0006-t-op-symbolcache.t-should-return-true.patch
From 62cc5551e4965ab5965ed9e7d1d08cc384275b71 Mon Sep 17 00:00:00 2001
From: Nicolas R <atoomic@cpan.org>
Date: Thu, 14 Sep 2017 14:53:20 -0600
Subject: [PATCH 6/6] t/op/symbolcache.t should return true

Add missing return 1
---
 t/op/symbolcache.t | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/t/op/symbolcache.t b/t/op/symbolcache.t
index 20b522676f..1daabd8f82 100644
--- a/t/op/symbolcache.t
+++ b/t/op/symbolcache.t
@@ -41,3 +41,5 @@ is( replaced2(), 'meth', 'original function not bound, was replaced' );
 ok( main->replaced2 eq 'meth', 'method is replaced function' );
 BEGIN { undef $main::{replaced2} }
 sub replaced2 { 'meth' }
+
+1;
-- 
2.14.1

@p5pRT
Copy link
Author

p5pRT commented Sep 14, 2017

From @atoomic

0001-tests-Use-set_up_inc-helper-to-set-INC.patch
From a5b6ec1564e563a3428d1e3059a78036bb4c11bb Mon Sep 17 00:00:00 2001
From: Nicolas R <atoomic@cpan.org>
Date: Thu, 14 Sep 2017 14:51:13 -0600
Subject: [PATCH 1/6] tests - Use set_up_inc helper to set @INC

Test plan should not be declared at compile time
---
 t/comp/parser_run.t |  4 ++--
 t/op/threads-dirh.t | 16 ++++++++--------
 t/re/anyof.t        |  6 ++++--
 3 files changed, 14 insertions(+), 12 deletions(-)

diff --git a/t/comp/parser_run.t b/t/comp/parser_run.t
index af35758bee..eba4b9fba8 100644
--- a/t/comp/parser_run.t
+++ b/t/comp/parser_run.t
@@ -5,11 +5,11 @@
 # Note that this should still be runnable under miniperl.
 
 BEGIN {
-    @INC = qw(. ../lib );
     chdir 't' if -d 't';
+    require './test.pl';
+    set_up_inc( qw(. ../lib ) );
 }
 
-require './test.pl';
 plan(4);
 
 # [perl #130814] can reallocate lineptr while looking ahead for
diff --git a/t/op/threads-dirh.t b/t/op/threads-dirh.t
index e1d5c996ad..82d90768ab 100644
--- a/t/op/threads-dirh.t
+++ b/t/op/threads-dirh.t
@@ -4,21 +4,21 @@
 
 BEGIN {
      chdir 't' if -d 't';
-     @INC = '../lib';
      require './test.pl';
+     set_up_inc('../lib');
      $| = 1;
-
      require Config;
-     skip_all_without_config('useithreads');
-     skip_all_if_miniperl("no dynamic loading on miniperl, no threads");
-
-     plan(6);
 }
 
+skip_all_without_config('useithreads');
+skip_all_if_miniperl("no dynamic loading on miniperl, no threads");
+
+plan(6);
+
 use strict;
 use warnings;
-use threads;
-use threads::shared;
+eval q/use threads/;
+eval q/use threads::shared/;
 use File::Path;
 use File::Spec::Functions qw 'updir catdir';
 use Cwd 'getcwd';
diff --git a/t/re/anyof.t b/t/re/anyof.t
index 84f2881e0d..d24e4a71a8 100644
--- a/t/re/anyof.t
+++ b/t/re/anyof.t
@@ -1,3 +1,5 @@
+#!./perl
+
 use utf8;
 
 # This tests that the ANYOF nodes generated by bracketed character classes are
@@ -10,9 +12,9 @@ use utf8;
 
 BEGIN {
     chdir 't' if -d 't';
-    @INC = ('../lib','.','../ext/re');
-    require Config; import Config;
     require './test.pl';
+    set_up_inc('../lib','.','../ext/re');
+    require Config; import Config;
     skip_all('no re module') unless defined &DynaLoader::boot_DynaLoader;
 }
 
-- 
2.14.1

@p5pRT
Copy link
Author

p5pRT commented Sep 14, 2017

From @atoomic

0002-tests-remove-useless-setting-in-BEGIN.patch
From f882d1b083e298bbc3dfabcefc84a656fb8074e8 Mon Sep 17 00:00:00 2001
From: Nicolas R <atoomic@cpan.org>
Date: Thu, 14 Sep 2017 14:51:42 -0600
Subject: [PATCH 2/6] tests - remove useless setting in BEGIN

These two tests are not using test.pl
and do not need to load any special file.

No need for fancy setup in BEGIN, all the most
we cannot use the generic set_up_inc helper.

Simply remove the useless BEGIN block.
---
 t/comp/bproto.t | 6 +-----
 t/opbasic/cmp.t | 5 -----
 2 files changed, 1 insertion(+), 10 deletions(-)

diff --git a/t/comp/bproto.t b/t/comp/bproto.t
index 8d11b915c1..cc91242140 100644
--- a/t/comp/bproto.t
+++ b/t/comp/bproto.t
@@ -3,11 +3,7 @@
 # check if builtins behave as prototyped
 #
 
-BEGIN {
-    chdir 't' if -d 't';
-    @INC = '../lib';
-}
-
+# Ideally tests in t/comp wouldn't use require, as require isn't tested yet
 print "1..16\n";
 
 my $i = 1;
diff --git a/t/opbasic/cmp.t b/t/opbasic/cmp.t
index 241eb491a6..5a88d21d0b 100644
--- a/t/opbasic/cmp.t
+++ b/t/opbasic/cmp.t
@@ -1,10 +1,5 @@
 #!./perl
 
-BEGIN {
-       chdir 't' if -d 't';
-       @INC = '../lib';
-}
-
 # This file has been placed in t/opbasic to indicate that it should not use
 # functions imported from t/test.pl or Test::More, as those programs/libraries
 # use operators which are what is being tested in this file.
-- 
2.14.1

@p5pRT
Copy link
Author

p5pRT commented Sep 14, 2017

From @atoomic

0003-test-Do-not-use-B-which-is-a-reserved-namespace.patch
From 5a3d29cf575b11888c2f1e390f1c5d264ee1e897 Mon Sep 17 00:00:00 2001
From: Nicolas R <atoomic@cpan.org>
Date: Thu, 14 Sep 2017 14:52:19 -0600
Subject: [PATCH 3/6] test - Do not use B which is a reserved namespace

B is already a reserved namespace. This is a bad idea
to use B during unit test, as this increase the complexity
when using one of the B subpackage to run the test.

Simply rename B to BB ( and A to AA ).
---
 t/mro/next_inanon.t | 18 +++++++++---------
 t/mro/next_ineval.t | 16 ++++++++--------
 t/op/method.t       | 52 ++++++++++++++++++++++++++--------------------------
 3 files changed, 43 insertions(+), 43 deletions(-)

diff --git a/t/mro/next_inanon.t b/t/mro/next_inanon.t
index b6f0451611..4c3c007d6e 100644
--- a/t/mro/next_inanon.t
+++ b/t/mro/next_inanon.t
@@ -13,26 +13,26 @@ anonymous subroutine.
 =cut
 
 {
-    package A;
+    package AA;
     use mro 'c3'; 
 
     sub foo {
-      return 'A::foo';
+      return 'AA::foo';
     }
 
     sub bar {
-      return 'A::bar';
+      return 'AA::bar';
     }
 }
 
 {
-    package B;
-    use base 'A';
+    package BB;
+    use base 'AA';
     use mro 'c3'; 
     
     sub foo {
       my $code = sub {
-        return 'B::foo => ' . (shift)->next::method();
+        return 'BB::foo => ' . (shift)->next::method();
       };
       return (shift)->$code;
     }
@@ -40,7 +40,7 @@ anonymous subroutine.
     sub bar {
       my $code1 = sub {
         my $code2 = sub {
-          return 'B::bar => ' . (shift)->next::method();
+          return 'BB::bar => ' . (shift)->next::method();
         };
         return (shift)->$code2;
       };
@@ -48,10 +48,10 @@ anonymous subroutine.
     }
 }
 
-is(B->foo, "B::foo => A::foo",
+is(BB->foo, "BB::foo => AA::foo",
    'method resolved inside anonymous sub');
 
-is(B->bar, "B::bar => A::bar",
+is(BB->bar, "BB::bar => AA::bar",
    'method resolved inside nested anonymous subs');
 
 
diff --git a/t/mro/next_ineval.t b/t/mro/next_ineval.t
index f8c13a6413..14a49b1c6b 100644
--- a/t/mro/next_ineval.t
+++ b/t/mro/next_ineval.t
@@ -12,23 +12,23 @@ This tests the use of an eval{} block to wrap a next::method call.
 =cut
 
 {
-    package A;
+    package AA;
     use mro 'c3'; 
 
     sub foo {
-      die 'A::foo died';
-      return 'A::foo succeeded';
+      die 'AA::foo died';
+      return 'AA::foo succeeded';
     }
 }
 
 {
-    package B;
-    use base 'A';
+    package BB;
+    use base 'AA';
     use mro 'c3'; 
     
     sub foo {
       eval {
-        return 'B::foo => ' . (shift)->next::method();
+        return 'BB::foo => ' . (shift)->next::method();
       };
 
       if ($@) {
@@ -37,8 +37,8 @@ This tests the use of an eval{} block to wrap a next::method call.
     }
 }
 
-like(B->foo, 
-   qr/^A::foo died/, 
+like(BB->foo,
+   qr/^AA::foo died/,
    'method resolved inside eval{}');
 
 
diff --git a/t/op/method.t b/t/op/method.t
index ef181c4ce0..e5cdce03e3 100644
--- a/t/op/method.t
+++ b/t/op/method.t
@@ -15,8 +15,8 @@ no warnings 'once';
 
 plan(tests => 151);
 
-@A::ISA = 'B';
-@B::ISA = 'C';
+@A::ISA = 'BB';
+@BB::ISA = 'C';
 
 sub C::d {"C::d"}
 sub D::d {"D::d"}
@@ -55,7 +55,7 @@ is(method $obj, "method");
 
 is( A->d, "C::d");		# Update hash table;
 
-*B::d = \&D::d;			# Import now.
+*BB::d = \&D::d;			# Import now.
 is(A->d, "D::d");		# Update hash table;
 
 {
@@ -67,42 +67,42 @@ is(A->d, "D::d");		# Update hash table;
 is(A->d, "D::d");
 
 {
-    local *B::d;
-    eval 'sub B::d {"B::d1"}';	# Import now.
-    is(A->d, "B::d1");	# Update hash table;
-    undef &B::d;
+    local *BB::d;
+    eval 'sub BB::d {"BB::d1"}';	# Import now.
+    is(A->d, "BB::d1");	# Update hash table;
+    undef &BB::d;
     is((eval { A->d }, ($@ =~ /Undefined subroutine/)), 1);
 }
 
 is(A->d, "D::d");		# Back to previous state
 
-eval 'no warnings "redefine"; sub B::d {"B::d2"}';	# Import now.
-is(A->d, "B::d2");		# Update hash table;
+eval 'no warnings "redefine"; sub BB::d {"BB::d2"}';	# Import now.
+is(A->d, "BB::d2");		# Update hash table;
 
 # What follows is hardly guarantied to work, since the names in scripts
-# are already linked to "pruned" globs. Say, 'undef &B::d' if it were
-# after 'delete $B::{d}; sub B::d {}' would reach an old subroutine.
+# are already linked to "pruned" globs. Say, 'undef &BB::d' if it were
+# after 'delete $BB::{d}; sub BB::d {}' would reach an old subroutine.
 
-undef &B::d;
-delete $B::{d};
+undef &BB::d;
+delete $BB::{d};
 is(A->d, "C::d");
 
-eval 'sub B::d {"B::d2.5"}';
+eval 'sub BB::d {"BB::d2.5"}';
 A->d;				# Update hash table;
-my $glob = \delete $B::{d};	# non-void context; hang on to the glob
+my $glob = \delete $BB::{d};	# non-void context; hang on to the glob
 is(A->d, "C::d");		# Update hash table;
 
-eval 'sub B::d {"B::d3"}';	# Import now.
-is(A->d, "B::d3");		# Update hash table;
+eval 'sub BB::d {"BB::d3"}';	# Import now.
+is(A->d, "BB::d3");		# Update hash table;
 
-delete $B::{d};
+delete $BB::{d};
 *dummy::dummy = sub {};		# Mark as updated
 is(A->d, "C::d");
 
-eval 'sub B::d {"B::d4"}';	# Import now.
-is(A->d, "B::d4");		# Update hash table;
+eval 'sub BB::d {"BB::d4"}';	# Import now.
+is(A->d, "BB::d4");		# Update hash table;
 
-delete $B::{d};			# Should work without any help too
+delete $BB::{d};			# Should work without any help too
 is(A->d, "C::d");
 
 {
@@ -119,16 +119,16 @@ my $counter;
 
 eval <<'EOF';
 sub C::e;
-BEGIN { *B::e = \&C::e }	# Shouldn't prevent AUTOLOAD in original pkg
+BEGIN { *BB::e = \&C::e }	# Shouldn't prevent AUTOLOAD in original pkg
 sub Y::f;
 $counter = 0;
 
 @X::ISA = 'Y';
-@Y::ISA = 'B';
+@Y::ISA = 'BB';
 
-sub B::AUTOLOAD {
+sub BB::AUTOLOAD {
   my $c = ++$counter;
-  my $method = $B::AUTOLOAD; 
+  my $method = $BB::AUTOLOAD; 
   my $msg = "B: In $method, $c";
   eval "sub $method { \$msg }";
   goto &$method;
@@ -157,7 +157,7 @@ is(Y->f(), "B: In Y::f, 3");	# Which sticks
 
 {
 no warnings 'redefine';
-*B::AUTOLOAD = sub {
+*BB::AUTOLOAD = sub {
   use warnings;
   my $c = ++$counter;
   my $method = $::AUTOLOAD; 
-- 
2.14.1

@p5pRT
Copy link
Author

p5pRT commented Sep 14, 2017

From @atoomic

0004-test-Do-not-run-test-output-at-compilation-time.patch
From f0ae221c72599e6d6c5fa53ae34f7b97f6d28f9a Mon Sep 17 00:00:00 2001
From: Nicolas R <atoomic@cpan.org>
Date: Thu, 14 Sep 2017 14:52:37 -0600
Subject: [PATCH 4/6] test - Do not run test output at compilation time

Even if in most of the cases it seems ok to print
output during compilation time, this is a pretty
bad idea (when compiling the test for example).

Run all the tests at compile time, but only
print the test status at run time.
---
 t/op/array_base.t | 27 ++++++++++++----------
 t/op/caller.t     | 67 ++++++++++++++++++++++++++++++++++++++++---------------
 2 files changed, 64 insertions(+), 30 deletions(-)

diff --git a/t/op/array_base.t b/t/op/array_base.t
index a30236d955..fc15ac830a 100644
--- a/t/op/array_base.t
+++ b/t/op/array_base.t
@@ -1,22 +1,25 @@
 #!perl -w
 use strict;
 
+my %begin_tests;
 BEGIN {
  chdir 't' if -d 't';
  require './test.pl';
-
- plan (tests => my $tests = 11);
-
- # Run these at BEGIN time, before arybase loads
  use v5.15;
- is(eval('$[ = 1; 123'), undef);
- like($@, qr/\AAssigning non-zero to \$\[ is no longer possible/);
+ # Run these at BEGIN time, before arybase loads
+ $begin_tests{123} = eval('$[ = 1; 123');
+ $begin_tests{error} = $@;
+}
+
+plan (tests => my $tests = 11); # plan should not be set at compile time
+ 
+is($begin_tests{123}, undef); 
+like($begin_tests{error}, qr/\AAssigning non-zero to \$\[ is no longer possible/);
 
- if (is_miniperl()) {
+if (is_miniperl()) {
    # skip the rest
    SKIP: { skip ("no arybase.xs on miniperl", $tests-2) }
    exit;
- }
 }
 
 no warnings 'deprecated';
@@ -25,17 +28,17 @@ is(eval('$['), 0);
 is(eval('$[ = 0; 123'), 123);
 is(eval('$[ = 1; 123'), 123);
 $[ = 1;
-ok $INC{'arybase.pm'};
+ok($INC{'arybase.pm'}, "arybase is in INC");
 
 use v5.15;
 is(eval('$[ = 1; 123'), undef);
 like($@, qr/\AAssigning non-zero to \$\[ is no longer possible/);
-is $[, 0, '$[ is 0 under 5.16';
+is($[, 0, '$[ is 0 under 5.16');
 $_ = "hello";
 /l/g;
 my $pos = \pos;
-is $$pos, 3;
+is($$pos, 3);
 $$pos = 1;
-is $$pos, 1;
+is($$pos, 1);
 
 1;
diff --git a/t/op/caller.t b/t/op/caller.t
index 1ffb5b3443..3a38415053 100644
--- a/t/op/caller.t
+++ b/t/op/caller.t
@@ -5,12 +5,20 @@ BEGIN {
     chdir 't' if -d 't';
     require './test.pl';
     set_up_inc('../lib');
-    plan( tests => 100 ); # some tests are run in a BEGIN block
 }
 
-my @c;
+my @tests;
+plan( tests => 100 );
+
+print "# Tests with caller(0)\n";
 
-BEGIN { print "# Tests with caller(0)\n"; }
+foreach my $t ( @tests ) {
+    my $s = \&{'main::'.$t->{type}};
+    $s->( @{$t->{args}}, $t->{txt} );
+}
+print "# end of BEGIN tests\n";
+
+my @c;
 
 @c = caller(0);
 ok( (!@c), "caller(0) in main program" );
@@ -36,8 +44,8 @@ ok( $c[4], "hasargs true with deleted sub" );
 
 BEGIN {
  require strict;
- is +(caller 0)[1], __FILE__,
-  "[perl #68712] filenames after require in a BEGIN block"
+ push @tests, { type => 'is', args => [ +(caller 0)[1], __FILE__ ], 
+    txt => "[perl #68712] filenames after require in a BEGIN block" };
 }
 
 print "# Tests with caller(1)\n";
@@ -97,6 +105,18 @@ sub testwarn {
     check_bits( (caller(0))[9], $w, "warnings match caller ($id)");
 }
 
+sub get_caller_0_9 {
+    return (caller(0))[9];
+}
+
+sub get_caller_0_9 {
+    return (caller(0))[9];
+}
+
+sub get_caller_0_9 {
+    return (caller(0))[9];
+}
+
 {
     no warnings;
     # Build the warnings mask dynamically
@@ -109,20 +129,27 @@ sub testwarn {
 	vec($registered, $warnings::LAST_BIT/2, 2) = 1;
     }
 
-    BEGIN { check_bits( ${^WARNING_BITS}, "\0" x $warnings::BYTES, 'all bits off via "no warnings"' ) }
+    BEGIN { 
+        push @tests, { type => 'check_bits', args => [ ${^WARNING_BITS}, "\0" x $warnings::BYTES ],
+        txt =>  'all bits off via "no warnings"' };
+    }        
     testwarn("\0" x $warnings::BYTES, 'no bits');
 
     use warnings;
-    BEGIN { check_bits( ${^WARNING_BITS}, $default,
-			'default bits on via "use warnings"' ); }
-    BEGIN { testwarn($default, 'all'); }
+    BEGIN { 
+        push @tests, { type => 'check_bits', args => [ ${^WARNING_BITS}, $default ], txt => 'default bits on via "use warnings"' };
+    }
+    BEGIN { 
+        push @tests, { type => 'check_bits', args => [ get_caller_0_9(), $default ], txt => 'warnings match caller' };
+    }
     # run-time :
     # the warning mask has been extended by warnings::register
     testwarn($registered, 'ahead of w::r');
 
     use warnings::register;
-    BEGIN { check_bits( ${^WARNING_BITS}, $registered,
-			'warning bits on via "use warnings::register"' ) }
+    BEGIN { 
+        push @tests, { type => 'check_bits', args => [ ${^WARNING_BITS}, $registered ], txt => 'warning bits on via "use warnings::register"' };
+    }
     testwarn($registered, 'following w::r');
 }
 
@@ -316,7 +343,7 @@ is $line, "3000000000", "check large line numbers are preserved";
 # This was fixed with commit d4d03940c58a0177, which fixed bug #78742
 fresh_perl_is <<'END', "__ANON__::doof\n", {},
 package foo;
-BEGIN {undef %foo::}
+INIT {undef %foo::} # adjust test for B::C
 sub doof { caller(0) }
 print +(doof())[3];
 END
@@ -357,12 +384,16 @@ do './op/caller.pl' or die $@;
     package RT129239;
     BEGIN {
         my ($pkg, $file, $line) = caller;
-        ::is $file, 'virtually/op/caller.t', "BEGIN block sees correct caller filename";
-        ::is $line, 12345,                   "BEGIN block sees correct caller line";
-        TODO: {
-            local $::TODO = "BEGIN blocks have wrong caller package [perl #129239]";
-            ::is $pkg, 'RT129239',               "BEGIN block sees correct caller package";
-        }
+# push @tests, { type => 'is', args => [ +(caller 0)[1], __FILE__ ], 
+#     txt => "[perl #68712] filenames after require in a BEGIN block" };
+
+        push @tests, { type => 'is', args => [ $file, 'virtually/op/caller.t' ], txt => "BEGIN block sees correct caller filename" };
+        push @tests, { type => 'is', args => [ $line, 12345 ], txt => "BEGIN block sees correct caller line" };
+        #TODO: {
+        #    local $::TODO = "BEGIN blocks have wrong caller package [perl #129239]";
+        #    push @tests, { type => is, args => [ $pkg, 'RT129239' ], txt => "BEGIN block sees correct caller package" };
+        #}
+        push @tests, { type => 'ok', txt => 'SKIPPING the BEGIN TODO test above' };
 #line 12345 "virtually/op/caller.t"
     }
 }
-- 
2.14.1

@p5pRT
Copy link
Author

p5pRT commented Sep 15, 2017

From @jkeenan

On Thu, 14 Sep 2017 22​:46​:27 GMT, atoomic@​cpan.org wrote​:

This is a bug report for perl from atoomic@​cpan.org,
generated with the help of perlbug 1.40 running under perl 5.24.1.

-----------------------------------------------------------------

This ticket is about updating a few unit tests for several reasons​:

1 - use set_up_inc helper to set @​INC
2 - remove useless setting in BEGIN
3 - do not use B which is a reserved namespace
4 - do not run test output at compilation time
5 - can run B with the test t/op/stash.t
6 - t/op/symbolcache.t should return true

Each idea comes with its own commit as some might be more difficult to
other to sell :-)

Good thinking!

I scanned each patch. I immediately grokked #1, 2, 3 and 6. 4 and 5 will need a second look. Can we get additional eyeballs please, especially on 4 and 5.

Thank you very much.
Jim Keenan

@p5pRT
Copy link
Author

p5pRT commented Sep 15, 2017

The RT System itself - Status changed from 'new' to 'open'

@p5pRT
Copy link
Author

p5pRT commented Sep 15, 2017

From @cpansprout

On Thu, 14 Sep 2017 17​:55​:57 -0700, jkeenan wrote​:

On Thu, 14 Sep 2017 22​:46​:27 GMT, atoomic@​cpan.org wrote​:

This is a bug report for perl from atoomic@​cpan.org,
generated with the help of perlbug 1.40 running under perl 5.24.1.

-----------------------------------------------------------------

This ticket is about updating a few unit tests for several reasons​:

1 - use set_up_inc helper to set @​INC
2 - remove useless setting in BEGIN
3 - do not use B which is a reserved namespace
4 - do not run test output at compilation time
5 - can run B with the test t/op/stash.t
6 - t/op/symbolcache.t should return true

Each idea comes with its own commit as some might be more difficult
to
other to sell :-)

Good thinking!

I scanned each patch. I immediately grokked #1, 2, 3 and 6. 4 and 5
will need a second look. Can we get additional eyeballs please,
especially on 4 and 5.

4 makes sense to me. If you want to compile the test script, dump it to a file, and then run it, you will be missing the output of the first few tests when you run it.

5 makes changes to stash.t that are wholly non-obvious. Why cannot B​::C run a script that loads B? It sounds like a problem with B​::C.

As for 6, why does a test script need to return any value?

--

Father Chrysostomos

@p5pRT
Copy link
Author

p5pRT commented Sep 15, 2017

From @cpansprout

5 makes changes to stash.t that are wholly non-obvious. Why cannot
B​::C run a script that loads B?

I should have said, ‘that loads B in a roundabout way’. I have no idea why one way of loading it works for you, while the other does not.

--

Father Chrysostomos

@p5pRT
Copy link
Author

p5pRT commented Sep 15, 2017

From @cpansprout

On Thu, 14 Sep 2017 18​:00​:48 -0700, sprout wrote​:

5 makes changes to stash.t that are wholly non-obvious. Why cannot
B​::C run a script that loads B?

I should have said, ‘that loads B in a roundabout way’.

Grrr. s/roundabout/straightforward/;

--

Father Chrysostomos

@p5pRT
Copy link
Author

p5pRT commented Sep 15, 2017

From @atoomic

On Thu, 14 Sep 2017 17​:59​:41 -0700, sprout wrote​:

On Thu, 14 Sep 2017 17​:55​:57 -0700, jkeenan wrote​:

On Thu, 14 Sep 2017 22​:46​:27 GMT, atoomic@​cpan.org wrote​:

This is a bug report for perl from atoomic@​cpan.org,
generated with the help of perlbug 1.40 running under perl 5.24.1.

-----------------------------------------------------------------

This ticket is about updating a few unit tests for several reasons​:

1 - use set_up_inc helper to set @​INC
2 - remove useless setting in BEGIN
3 - do not use B which is a reserved namespace
4 - do not run test output at compilation time
5 - can run B with the test t/op/stash.t
6 - t/op/symbolcache.t should return true

Each idea comes with its own commit as some might be more difficult
to
other to sell :-)

Good thinking!

I scanned each patch. I immediately grokked #1, 2, 3 and 6. 4 and 5
will need a second look. Can we get additional eyeballs please,
especially on 4 and 5.

4 makes sense to me. If you want to compile the test script, dump it
to a file, and then run it, you will be missing the output of the
first few tests when you run it.

5 makes changes to stash.t that are wholly non-obvious. Why cannot
B​::C run a script that loads B? It sounds like a problem with B​::C.

You are right this sounds like a problem with B​::C, and I've just realized that we've already fixed it correctly, so we do not need this patch #5 anymore, we should discard it. [ should not have been part of my merge request ]
The problem was that B​::C was loaded by B, and B​::C by default was not saving B. An improved logic now detects when B is used or not by the script and compiled B or not...

As for 6, why does a test script need to return any value?

yes I agree 6 it is stupid lol we should also discard it

@p5pRT
Copy link
Author

p5pRT commented Sep 15, 2017

From @jkeenan

On Fri, 15 Sep 2017 15​:27​:02 GMT, atoomic wrote​:

On Thu, 14 Sep 2017 17​:59​:41 -0700, sprout wrote​:

On Thu, 14 Sep 2017 17​:55​:57 -0700, jkeenan wrote​:

On Thu, 14 Sep 2017 22​:46​:27 GMT, atoomic@​cpan.org wrote​:

This is a bug report for perl from atoomic@​cpan.org,
generated with the help of perlbug 1.40 running under perl
5.24.1.

-----------------------------------------------------------------

This ticket is about updating a few unit tests for several
reasons​:

1 - use set_up_inc helper to set @​INC
2 - remove useless setting in BEGIN
3 - do not use B which is a reserved namespace
4 - do not run test output at compilation time
5 - can run B with the test t/op/stash.t
6 - t/op/symbolcache.t should return true

Each idea comes with its own commit as some might be more
difficult
to
other to sell :-)

Good thinking!

I scanned each patch. I immediately grokked #1, 2, 3 and 6. 4 and
5
will need a second look. Can we get additional eyeballs please,
especially on 4 and 5.

4 makes sense to me. If you want to compile the test script, dump it
to a file, and then run it, you will be missing the output of the
first few tests when you run it.

5 makes changes to stash.t that are wholly non-obvious. Why cannot
B​::C run a script that loads B? It sounds like a problem with B​::C.

You are right this sounds like a problem with B​::C, and I've just
realized that we've already fixed it correctly, so we do not need this
patch #5 anymore, we should discard it. [ should not have been part of
my merge request ]
The problem was that B​::C was loaded by B, and B​::C by default was not
saving B. An improved logic now detects when B is used or not by the
script and compiled B or not...

As for 6, why does a test script need to return any value?

yes I agree 6 it is stupid lol we should also discard it

Applied 0001 thru 0004 to blead. Rejecting 0005 and 0006. Marking ticket resolved.

Thank you very much.

--
James E Keenan (jkeenan@​cpan.org)

@p5pRT
Copy link
Author

p5pRT commented Sep 15, 2017

@jkeenan - Status changed from 'open' to 'resolved'

@p5pRT
Copy link
Author

p5pRT commented Sep 15, 2017

From @jkeenan

We have to re-open this ticket because the changes to t/op/threads-dirh.t which were part of commit 1ce8be8 earlier today are experiencing failures in threaded builds on Linux. Example​: http​://perl5.test-smoke.org/report/57946

tux++ for all those smoke-testing rigs.

I'm currently testing a revert to just this one file and will push when it PASSes.

Thank you very much.
--
James E Keenan (jkeenan@​cpan.org)

@p5pRT
Copy link
Author

p5pRT commented Sep 15, 2017

@jkeenan - Status changed from 'resolved' to 'open'

@p5pRT
Copy link
Author

p5pRT commented Sep 16, 2017

From @jkeenan

On Fri, 15 Sep 2017 23​:36​:46 GMT, jkeenan wrote​:

We have to re-open this ticket because the changes to t/op/threads-
dirh.t which were part of commit 1ce8be8 earlier today
are experiencing failures in threaded builds on Linux. Example​:
http​://perl5.test-smoke.org/report/57946

tux++ for all those smoke-testing rigs.

I'm currently testing a revert to just this one file and will push
when it PASSes.

Thank you very much.

The reversion has been made in commit 07d51b5.

Before reversion, the test failure output looked like this​:

#####
ok 1 - crash when duping dirh
Can't call method "async" on unblessed reference at op/threads-dirh.t line 80.
# Looks like you planned 6 tests but ran 1.
Dubious, test returned 25 (wstat 6400, 0x1900)
Failed 5/6 subtests

Test Summary Report


op/threads-dirh.t (Wstat​: 6400 Tests​: 1 Failed​: 0)
  Non-zero exit status​: 25
  Parse errors​: Bad plan. You planned 6 tests but ran 1.
Files=1, Tests=1, 0 wallclock secs ( 0.01 usr 0.00 sys + 0.02 cusr 0.00 csys = 0.03 CPU)
Result​: FAIL
#####

On IRC, Tony Cook pointed out this commit from August 2016, which is relevant to the problem​:

https://perl5.git.perl.org/perl.git/commitdiff/0cf18b74ec1005ca929e64dd9158674d5163ff0a

Thank you very much.

--
James E Keenan (jkeenan@​cpan.org)

@p5pRT
Copy link
Author

p5pRT commented Sep 18, 2017

From @atoomic

Thanks for the partial revert via 07d51b5, and catching the error on windows.
I'm fine if we discard that extra patch, and IMO we should close this ticket as is.

thanks
nicolas

On Fri, 15 Sep 2017 17​:00​:46 -0700, jkeenan wrote​:

On Fri, 15 Sep 2017 23​:36​:46 GMT, jkeenan wrote​:

We have to re-open this ticket because the changes to t/op/threads-
dirh.t which were part of commit 1ce8be8 earlier today
are experiencing failures in threaded builds on Linux. Example​:
http​://perl5.test-smoke.org/report/57946

tux++ for all those smoke-testing rigs.

I'm currently testing a revert to just this one file and will push
when it PASSes.

Thank you very much.

The reversion has been made in commit 07d51b5.

Before reversion, the test failure output looked like this​:

#####
ok 1 - crash when duping dirh
Can't call method "async" on unblessed reference at op/threads-dirh.t
line 80.
# Looks like you planned 6 tests but ran 1.
Dubious, test returned 25 (wstat 6400, 0x1900)
Failed 5/6 subtests

Test Summary Report
-------------------
op/threads-dirh.t (Wstat​: 6400 Tests​: 1 Failed​: 0)
Non-zero exit status​: 25
Parse errors​: Bad plan. You planned 6 tests but ran 1.
Files=1, Tests=1, 0 wallclock secs ( 0.01 usr 0.00 sys + 0.02 cusr
0.00 csys = 0.03 CPU)
Result​: FAIL
#####

On IRC, Tony Cook pointed out this commit from August 2016, which is
relevant to the problem​:

https://perl5.git.perl.org/perl.git/commitdiff/0cf18b74ec1005ca929e64dd9158674d5163ff0a

Thank you very much.

@p5pRT
Copy link
Author

p5pRT commented Sep 18, 2017

From @jkeenan

On Mon, 18 Sep 2017 15​:06​:44 GMT, atoomic wrote​:

Thanks for the partial revert via 07d51b5, and catching the error
on windows.
I'm fine if we discard that extra patch, and IMO we should close this
ticket as is.

thanks
nicolas

On Fri, 15 Sep 2017 17​:00​:46 -0700, jkeenan wrote​:

On Fri, 15 Sep 2017 23​:36​:46 GMT, jkeenan wrote​:

We have to re-open this ticket because the changes to t/op/threads-
dirh.t which were part of commit 1ce8be8 earlier today
are experiencing failures in threaded builds on Linux. Example​:
http​://perl5.test-smoke.org/report/57946

tux++ for all those smoke-testing rigs.

I'm currently testing a revert to just this one file and will push
when it PASSes.

Thank you very much.

The reversion has been made in commit 07d51b5.

Before reversion, the test failure output looked like this​:

#####
ok 1 - crash when duping dirh
Can't call method "async" on unblessed reference at op/threads-dirh.t
line 80.
# Looks like you planned 6 tests but ran 1.
Dubious, test returned 25 (wstat 6400, 0x1900)
Failed 5/6 subtests

Test Summary Report
-------------------
op/threads-dirh.t (Wstat​: 6400 Tests​: 1 Failed​: 0)
Non-zero exit status​: 25
Parse errors​: Bad plan. You planned 6 tests but ran 1.
Files=1, Tests=1, 0 wallclock secs ( 0.01 usr 0.00 sys + 0.02 cusr
0.00 csys = 0.03 CPU)
Result​: FAIL
#####

On IRC, Tony Cook pointed out this commit from August 2016, which is
relevant to the problem​:

https://perl5.git.perl.org/perl.git/commitdiff/0cf18b74ec1005ca929e64dd9158674d5163ff0a

Thank you very much.

Thanks. Note to you (as contributor) and (especially) to me (as committer)​:

If one of the files being changed has the string 'thread' in its name, be sure to test on a threaded build!
--
James E Keenan (jkeenan@​cpan.org)

@p5pRT
Copy link
Author

p5pRT commented Sep 18, 2017

@jkeenan - Status changed from 'open' to 'resolved'

@p5pRT
Copy link
Author

p5pRT commented Sep 20, 2017

From @genehack

On 18Sep2017, at 13​:44, James E Keenan via RT <perlbug-followup@​perl.org> wrote​:

Thanks. Note to you (as contributor) and (especially) to me (as committer)​:

If one of the files being changed has the string 'thread' in its name, be sure to test on a threaded build!

A further note​: during the course of releasing 5.27.4, I discovered that one of the commits in this set (d190dde) made changes to t/op/array_base.t that broke it, but only when running under miniperl (i.e., ‘make test’ would pass but ‘make minitest’ was failing).

After discussion with Zefram in #p5p, I reverted the whole commit with commit a8b7548. I will land this shortly, with the other 5.27.4 release-related changes.

Before revision, the test failure looked like​:

  % ../miniperl -I../lib op/array_base.t
  Can't load module arybase, dynamic loading not available in this perl.
  (You may need to build a new perl executable which either supports
  dynamic loading or has the arybase module statically linked into it.)
  at op/array_base.t line 30.
  Compilation failed in require at op/array_base.t line 30.
  BEGIN failed--compilation aborted at op/array_base.t line 30.

Further reminder to contributors and committers​: run ‘make minitest’ in addition to ‘make test’. ;^)

thanks,
john.

@p5pRT
Copy link
Author

p5pRT commented Sep 20, 2017

@jkeenan - Status changed from 'resolved' to 'open'

@p5pRT
Copy link
Author

p5pRT commented Sep 20, 2017

From zefram@fysh.org

John SJ Anderson wrote​:

I discovered that one of the commits in this set
(d190dde) made changes to
t/op/array_base.t that broke it, but only when running under miniperl

Specifically, there had been code in a BEGIN block to exit after the
first couple of tests when running under miniperl. This was to avoid the
compilation of "$[ = 1" later in the file, because compiling that would
attempt to load arybase, and with that being an XS module it can't be
loaded under miniperl. The recent faulty commit moved the is_miniperl()
check out of the BEGIN block, so it would only check miniperlness at
runtime, so the "$[ = 1" would be compiled anyway, causing a compile-time
failure on miniperl.

Both of the test files edited by that commit have good reason to perform
tests and other things at BEGIN time, and there's nothing wrong with
doing so. So quite apart from the introduction of the above bug, this
commit was misconceived, making the test files more convoluted to no
actual benefit. So my advice was to revert the entire commit.

-zefram

@p5pRT
Copy link
Author

p5pRT commented Sep 20, 2017

From @jkeenan

On Wed, 20 Sep 2017 20​:07​:18 GMT, genehack wrote​:

On 18Sep2017, at 13​:44, James E Keenan via RT <perlbug-
followup@​perl.org> wrote​:

Thanks. Note to you (as contributor) and (especially) to me (as
committer)​:

If one of the files being changed has the string 'thread' in its
name, be sure to test on a threaded build!

A further note​: during the course of releasing 5.27.4, I discovered
that one of the commits in this set
(d190dde) made changes to
t/op/array_base.t that broke it, but only when running under miniperl
(i.e., ‘make test’ would pass but ‘make minitest’ was failing).

After discussion with Zefram in #p5p, I reverted the whole commit with
commit a8b7548. I will land this
shortly, with the other 5.27.4 release-related changes.

Before revision, the test failure looked like​:

% ../miniperl -I../lib op/array_base.t
Can't load module arybase, dynamic loading not available in this perl.
(You may need to build a new perl executable which either supports
dynamic loading or has the arybase module statically linked into it.)
at op/array_base.t line 30.
Compilation failed in require at op/array_base.t line 30.
BEGIN failed--compilation aborted at op/array_base.t line 30.

Further reminder to contributors and committers​: run ‘make minitest’
in addition to ‘make test’. ;^)

thanks,
john.

John, as the person who committed d190dde, I send my regrets for the time and effort that commit's problems cost you in the preparation of a monthly release. And I don't disagree with Zefram's critique of the content of that commit, though I should note that before committing that patch I requested additional eyeballs on it and got a thumbs up from one other committer.

However, to expect committers to run 'make minitest' before any given commit requires further discussion. That's not cost-free; here's why.

1. Commits which run into failures during 'make minitest' but not during 'make test' or 'make test_harness' are very infrequent. A search through my perl5-porters newsgroup backlog shows that 'minitest' has not appeared in the Subject line of a message in 14 months. (In fact, I found a message from April 26 2015 in which I myself wrote that I had never heard of 'make minitest' until the previous day.) I infer from that that such failures are rare.

2. pod/perlhack.pod has nothing to say about the 'minitest' target except to define it with respect to 'miniperl' -- and that latter concept is not further discussed in this document.

3. pod/perlgit.pod does have this to say about 'minitest'​:

#####
If you make any changes that affect miniperl or core routines that have
different code paths for miniperl, be sure to run C<make minitest>.
This will catch problems that even the full test suite will not catch
because it runs a subset of tests under miniperl rather than perl.
#####

But this, of course, assumes that the author and committer can identify which changes in source code that "will affect miniperl or core routines that have different code paths for miniperl." Well, I concede that I cannot instantly identify such changes, so I am unlikely to be able to identify those rare circumstances that warrant a run of 'make minitest'. And I doubt I'm the only author or committer in that position. I suspect that there are fewer than a dozen people in the world who could look at a patch for perl 5 core and say, right off the bat, "That needs 'make minitest'."

4. We rely on our smoke-testers to detect problems that don't turn up in a typical author's or committer's run of 'make test'. Indeed, smoke-test reports led to the reversion of one other patch originally submitted to this ticket. Further, if you grep the source code of Test-Smoke, you do find a 'make_minitest' method. But Test-Smoke only calls this method when an overall 'make' has failed but 'make miniperl' has succeeded. So it's not surprising that Test-Smoke did not detect the problem with t/op/array_base.t.

So this is a case where we have to weigh the benefit of advance detection of a relatively rare type of error against the cost of the changes to committers' work flows and (potentially) to Test-Smoke. I'm not going to dig in my heels to oppose such changes. I simply want it recognized that they will not be cost-free.

Thank you very much.

--
James E Keenan (jkeenan@​cpan.org)

@p5pRT
Copy link
Author

p5pRT commented Sep 20, 2017

From @atoomic

Thanks for taking action there, I trend to agree to Zefram comment about the value of the commit itself, which add the end increase its maintenance cost, so reverting it is fine.

I would keep in mind the "make minitest", thanks for your time and effort there,
sorry for the extra noise this brings, would have expect a smoother integration,
will be more careful in the future.

nicolas

On Wed, 20 Sep 2017 14​:40​:41 -0700, jkeenan wrote​:

On Wed, 20 Sep 2017 20​:07​:18 GMT, genehack wrote​:

On 18Sep2017, at 13​:44, James E Keenan via RT <perlbug-
followup@​perl.org> wrote​:

Thanks. Note to you (as contributor) and (especially) to me (as
committer)​:

If one of the files being changed has the string 'thread' in its
name, be sure to test on a threaded build!

A further note​: during the course of releasing 5.27.4, I discovered
that one of the commits in this set
(d190dde) made changes to
t/op/array_base.t that broke it, but only when running under miniperl
(i.e., ‘make test’ would pass but ‘make minitest’ was failing).

After discussion with Zefram in #p5p, I reverted the whole commit
with
commit a8b7548. I will land this
shortly, with the other 5.27.4 release-related changes.

Before revision, the test failure looked like​:

% ../miniperl -I../lib op/array_base.t
Can't load module arybase, dynamic loading not available in this
perl.
(You may need to build a new perl executable which either supports
dynamic loading or has the arybase module statically linked into it.)
at op/array_base.t line 30.
Compilation failed in require at op/array_base.t line 30.
BEGIN failed--compilation aborted at op/array_base.t line 30.

Further reminder to contributors and committers​: run ‘make minitest’
in addition to ‘make test’. ;^)

thanks,
john.

John, as the person who committed d190dde, I send my regrets for the
time and effort that commit's problems cost you in the preparation of
a monthly release. And I don't disagree with Zefram's critique of the
content of that commit, though I should note that before committing
that patch I requested additional eyeballs on it and got a thumbs up
from one other committer.

However, to expect committers to run 'make minitest' before any given
commit requires further discussion. That's not cost-free; here's why.

1. Commits which run into failures during 'make minitest' but not
during 'make test' or 'make test_harness' are very infrequent. A
search through my perl5-porters newsgroup backlog shows that
'minitest' has not appeared in the Subject line of a message in 14
months. (In fact, I found a message from April 26 2015 in which I
myself wrote that I had never heard of 'make minitest' until the
previous day.) I infer from that that such failures are rare.

2. pod/perlhack.pod has nothing to say about the 'minitest' target
except to define it with respect to 'miniperl' -- and that latter
concept is not further discussed in this document.

3. pod/perlgit.pod does have this to say about 'minitest'​:

#####
If you make any changes that affect miniperl or core routines that
have
different code paths for miniperl, be sure to run C<make minitest>.
This will catch problems that even the full test suite will not catch
because it runs a subset of tests under miniperl rather than perl.
#####

But this, of course, assumes that the author and committer can
identify which changes in source code that "will affect miniperl or
core routines that have different code paths for miniperl." Well, I
concede that I cannot instantly identify such changes, so I am
unlikely to be able to identify those rare circumstances that warrant
a run of 'make minitest'. And I doubt I'm the only author or
committer in that position. I suspect that there are fewer than a
dozen people in the world who could look at a patch for perl 5 core
and say, right off the bat, "That needs 'make minitest'."

4. We rely on our smoke-testers to detect problems that don't turn up
in a typical author's or committer's run of 'make test'. Indeed,
smoke-test reports led to the reversion of one other patch originally
submitted to this ticket. Further, if you grep the source code of
Test-Smoke, you do find a 'make_minitest' method. But Test-Smoke only
calls this method when an overall 'make' has failed but 'make
miniperl' has succeeded. So it's not surprising that Test-Smoke did
not detect the problem with t/op/array_base.t.

So this is a case where we have to weigh the benefit of advance
detection of a relatively rare type of error against the cost of the
changes to committers' work flows and (potentially) to Test-Smoke.
I'm not going to dig in my heels to oppose such changes. I simply
want it recognized that they will not be cost-free.

Thank you very much.

@p5pRT
Copy link
Author

p5pRT commented Sep 20, 2017

From zefram@fysh.org

James E Keenan via RT wrote​:

But this, of course, assumes that the author and committer can identify
which changes in source code that "will affect miniperl or core routines
that have different code paths for miniperl."

The file in question contained an explicit call to is_miniperl().
That ought to clue one in that minitest could be affected, especially
when the commit involves moving that call so significantly. The need
will not always be this obvious, of course.

-zefram

@p5pRT
Copy link
Author

p5pRT commented Sep 21, 2017

From @cpansprout

On Wed, 20 Sep 2017 14​:40​:41 -0700, jkeenan wrote​:

So this is a case where we have to weigh the benefit of advance
detection of a relatively rare type of error against the cost of the
changes to committers' work flows and (potentially) to Test-Smoke.
I'm not going to dig in my heels to oppose such changes. I simply
want it recognized that they will not be cost-free.

I, for one, think that occasional minitest bit-rot is perfectly fine. minitest is mainly a tool for people doing deep hacking (stuff that will break things at a low level, for which minitest is helpful in figuring out why the more complicated tests are failing) and for people porting perl to another system (who presumably are knowledgeable enough to handle it).

When doing the former (I have never done the latter), I have fixed up minitest when I was about to use it. I have no problem with that.

Jarkko, on the other hand, would probably disagree with me....

--

Father Chrysostomos

@p5pRT
Copy link
Author

p5pRT commented Sep 23, 2017

From @xsawyerx

[Top-posted]

Jim, this work was done fully in public with all the best intentions and
request for comments. The "damage" it caused was minimal (a few minutes
delay on a release that was already ahead of schedule in the day) and
I'm sure neither Zefram nor John were at all distraught by it, even the
slightest.

It's all good. :)

<3

On 09/20/2017 11​:40 PM, James E Keenan via RT wrote​:

On Wed, 20 Sep 2017 20​:07​:18 GMT, genehack wrote​:

On 18Sep2017, at 13​:44, James E Keenan via RT <perlbug-
followup@​perl.org> wrote​:

Thanks. Note to you (as contributor) and (especially) to me (as
committer)​:

If one of the files being changed has the string 'thread' in its
name, be sure to test on a threaded build!
A further note​: during the course of releasing 5.27.4, I discovered
that one of the commits in this set
(d190dde) made changes to
t/op/array_base.t that broke it, but only when running under miniperl
(i.e., ‘make test’ would pass but ‘make minitest’ was failing).

After discussion with Zefram in #p5p, I reverted the whole commit with
commit a8b7548. I will land this
shortly, with the other 5.27.4 release-related changes.

Before revision, the test failure looked like​:

% ../miniperl -I../lib op/array_base.t
Can't load module arybase, dynamic loading not available in this perl.
(You may need to build a new perl executable which either supports
dynamic loading or has the arybase module statically linked into it.)
at op/array_base.t line 30.
Compilation failed in require at op/array_base.t line 30.
BEGIN failed--compilation aborted at op/array_base.t line 30.

Further reminder to contributors and committers​: run ‘make minitest’
in addition to ‘make test’. ;^)

thanks,
john.
John, as the person who committed d190dde, I send my regrets for the time and effort that commit's problems cost you in the preparation of a monthly release. And I don't disagree with Zefram's critique of the content of that commit, though I should note that before committing that patch I requested additional eyeballs on it and got a thumbs up from one other committer.

However, to expect committers to run 'make minitest' before any given commit requires further discussion. That's not cost-free; here's why.

1. Commits which run into failures during 'make minitest' but not during 'make test' or 'make test_harness' are very infrequent. A search through my perl5-porters newsgroup backlog shows that 'minitest' has not appeared in the Subject line of a message in 14 months. (In fact, I found a message from April 26 2015 in which I myself wrote that I had never heard of 'make minitest' until the previous day.) I infer from that that such failures are rare.

2. pod/perlhack.pod has nothing to say about the 'minitest' target except to define it with respect to 'miniperl' -- and that latter concept is not further discussed in this document.

3. pod/perlgit.pod does have this to say about 'minitest'​:

#####
If you make any changes that affect miniperl or core routines that have
different code paths for miniperl, be sure to run C<make minitest>.
This will catch problems that even the full test suite will not catch
because it runs a subset of tests under miniperl rather than perl.
#####

But this, of course, assumes that the author and committer can identify which changes in source code that "will affect miniperl or core routines that have different code paths for miniperl." Well, I concede that I cannot instantly identify such changes, so I am unlikely to be able to identify those rare circumstances that warrant a run of 'make minitest'. And I doubt I'm the only author or committer in that position. I suspect that there are fewer than a dozen people in the world who could look at a patch for perl 5 core and say, right off the bat, "That needs 'make minitest'."

4. We rely on our smoke-testers to detect problems that don't turn up in a typical author's or committer's run of 'make test'. Indeed, smoke-test reports led to the reversion of one other patch originally submitted to this ticket. Further, if you grep the source code of Test-Smoke, you do find a 'make_minitest' method. But Test-Smoke only calls this method when an overall 'make' has failed but 'make miniperl' has succeeded. So it's not surprising that Test-Smoke did not detect the problem with t/op/array_base.t.

So this is a case where we have to weigh the benefit of advance detection of a relatively rare type of error against the cost of the changes to committers' work flows and (potentially) to Test-Smoke. I'm not going to dig in my heels to oppose such changes. I simply want it recognized that they will not be cost-free.

Thank you very much.

@p5pRT
Copy link
Author

p5pRT commented Sep 24, 2017

From @jkeenan

Since the tests added in this RT that have not been reverted appear to be stable, I am once again marking this ticket Resolved.

Thank you very much.
--
James E Keenan (jkeenan@​cpan.org)

@p5pRT p5pRT closed this as completed Sep 24, 2017
@p5pRT
Copy link
Author

p5pRT commented Sep 24, 2017

@jkeenan - Status changed from 'open' to 'resolved'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant