ext/File-Find/t/*.t: Continued intermittent failures during parallel testing #16814
Comments
From @jkeenanWe are still having intermittent problems with parallel testing in http://perl5.test-smoke.org/report/77754 In this smoke-test configuration 'make test_harness' runs 36 times. In ##### (See also this (out-of-date) matrix report: When I examine the log for this instance, I see: ##### [2019-01-07 07:15:35+0000] TSTENV = stdio error while running harness target 'test_harness': 1 at I tried reproducing this failure on the same machine with this ##### I was not surprised that this did not reproduce the failure. The file which is actually failing in this report is ##### It's failing in the finddepth() call at line 117, i.e., it PASSes all 3 But the "from" directory in the error message is referring to testing ##### By implication, the failure in find.t is occurring because testing In the smoke-me/jkeenan/test-file-find-serially branch I placed Thank you very much. |
From @jkeenan0001-Do-not-test-ext-File-Find-t-.t-files-in-parallel.patchFrom 3cfb5c799c5af768855bd06d71cc73029fe0b2a8 Mon Sep 17 00:00:00 2001
From: James E Keenan <jkeenan@cpan.org>
Date: Wed, 9 Jan 2019 12:51:38 -0500
Subject: [PATCH] Do not test ext/File-Find/t/*.t files in parallel.
Smoke test reports suggest that files and directories which are being
created temporarily for testing purposes in t/taint.t are being seen by
finddepth() while the harness is running t/find.t.
---
t/harness | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/t/harness b/t/harness
index caa2a318b8..3b9f7488df 100644
--- a/t/harness
+++ b/t/harness
@@ -189,7 +189,11 @@ if (@ARGV) {
# directory containing such files should be tested in serial order.
#
# Add exceptions to the above rule
- for (qw(ext/Pod-Html/t cpan/IO-Zlib/t)) {
+ for (qw(
+ ext/Pod-Html/t
+ cpan/IO-Zlib/t
+ ext/File-Find/t
+ )) {
$serials{$_} = 1;
}
--
2.17.1
|
From @iabynOn Tue, Jan 15, 2019 at 01:05:23PM -0800, James E Keenan (via RT) wrote:
Surely it would be far better to make find.t parallel-run-safe rather than From a brief visual inspection of find.t, it seems that it generally -- |
|
The RT System itself - Status changed from 'new' to 'open' |
From @jkeenanOn Wed, 16 Jan 2019 11:23:21 GMT, davem wrote:
Please review the patch attached, which is available for smoke testing in the following branch: smoke-me/jkeenan/133771-repair-file-find-tests Thank you very much. |
From @jkeenan0001-find.t-Use-temporary-testing-directory-for-all-block.patchFrom d754d1a80a3b7fd9e5b315e97226f82cfcde2107 Mon Sep 17 00:00:00 2001
From: James E Keenan <jkeenan@cpan.org>
Date: Wed, 16 Jan 2019 10:56:44 -0500
Subject: [PATCH] find.t: Use temporary testing directory for all blocks of
tests.
During the execution of ext/File-Find/t/taint.t certain files and directories
are created. As inferred from smoke-test reports, when testing in parallel
these entities ran the risk of being detected by certain tests in
ext/File-Find/t/find.t, thereby causing intermittent failures in the smoke
tests.
Problem is being addressed by moving the block of tests created in August 2014
(RT #122547) to prevent processing of misspelled options to a place in the
file where we have moved from the current working directory one level down to
a temporary testing directory (hence the change from File::Spec->curdir to
File::Spec->updir).
For: RT # 133771
---
ext/File-Find/t/find.t | 84 ++++++++++++++++++++++--------------------
1 file changed, 44 insertions(+), 40 deletions(-)
diff --git a/ext/File-Find/t/find.t b/ext/File-Find/t/find.t
index b532752a5a..297d0580e0 100644
--- a/ext/File-Find/t/find.t
+++ b/ext/File-Find/t/find.t
@@ -90,46 +90,6 @@ finddepth({wanted => sub { ++$::count_taint if $_ eq 'taint.t'; } },
File::Spec->curdir);
is($::count_taint, 1, "'finddepth' found exactly 1 file named 'taint.t'");
-##### RT #122547 #####
-# Do find() and finddepth() correctly warn on invalid options?
-{
- my $bad_option = 'foobar';
- my $second_bad_option = 'really_foobar';
-
- $::count_taint = 0;
- local $SIG{__WARN__} = sub { $warn_msg = $_[0]; };
- {
- find(
- {
- wanted => sub { ++$::count_taint if $_ eq 'taint.t'; },
- $bad_option => undef,
- },
- File::Spec->curdir
- );
- };
- like($warn_msg, qr/Invalid option/s, "Got warning for invalid option");
- like($warn_msg, qr/$bad_option/s, "Got warning for $bad_option");
- is($::count_taint, 1, "count_taint incremented");
- undef $warn_msg;
-
- $::count_taint = 0;
- {
- finddepth(
- {
- wanted => sub { ++$::count_taint if $_ eq 'taint.t'; },
- $bad_option => undef,
- $second_bad_option => undef,
- },
- File::Spec->curdir
- );
- };
- like($warn_msg, qr/Invalid option/s, "Got warning for invalid option");
- like($warn_msg, qr/$bad_option/s, "Got warning for $bad_option");
- like($warn_msg, qr/$second_bad_option/s, "Got warning for $second_bad_option");
- is($::count_taint, 1, "count_taint incremented");
- undef $warn_msg;
-}
-
my $FastFileTests_OK = 0;
sub cleanup {
@@ -300,6 +260,50 @@ create_file_ok( file_path('fa', 'fab', 'fab_ord') );
mkdir_ok( dir_path('fa', 'fab', 'faba'), 0770 );
create_file_ok( file_path('fa', 'fab', 'faba', 'faba_ord') );
+##### RT #122547 #####
+# Do find() and finddepth() correctly warn on invalid options?
+##### RT #133771 #####
+# When running tests in parallel, avoid clash with tests in
+# ext/File-Find/t/taint by moving into the temporary testing directory
+# before testing for warnings on invalid options.
+{
+ my $bad_option = 'foobar';
+ my $second_bad_option = 'really_foobar';
+
+ $::count_taint = 0;
+ local $SIG{__WARN__} = sub { $warn_msg = $_[0]; };
+ {
+ find(
+ {
+ wanted => sub { ++$::count_taint if $_ eq 'taint.t'; },
+ $bad_option => undef,
+ },
+ File::Spec->updir
+ );
+ };
+ like($warn_msg, qr/Invalid option/s, "Got warning for invalid option");
+ like($warn_msg, qr/$bad_option/s, "Got warning for $bad_option");
+ is($::count_taint, 1, "count_taint incremented");
+ undef $warn_msg;
+
+ $::count_taint = 0;
+ {
+ finddepth(
+ {
+ wanted => sub { ++$::count_taint if $_ eq 'taint.t'; },
+ $bad_option => undef,
+ $second_bad_option => undef,
+ },
+ File::Spec->updir
+ );
+ };
+ like($warn_msg, qr/Invalid option/s, "Got warning for invalid option");
+ like($warn_msg, qr/$bad_option/s, "Got warning for $bad_option");
+ like($warn_msg, qr/$second_bad_option/s, "Got warning for $second_bad_option");
+ is($::count_taint, 1, "count_taint incremented");
+ undef $warn_msg;
+}
+
##### Basic tests for find() #####
# Set up list of files we expect to find.
# Run find(), removing a file from the list once we have found it.
--
2.17.1
|
From @tonycozOn Wed, 16 Jan 2019 07:59:39 -0800, jkeenan wrote:
I don't think the patch is correct. The problem is caused by a race between Find::Find called by find.t trying to search the t/fa_taint/... directory tree while taint.t is removing it. Since with your change find.t will still try to search everything under t, including fa_taint, you're still going to get races. The fix is to move the search as Dave suggested, but to search within the t/for_find tree that find.t controls instead of everything under t/. The test code will need to be updated to search for something other than taint.t. Tony |
From @jkeenanOn Wed, 16 Jan 2019 23:04:01 GMT, tonyc wrote:
Let me pose this question: Would we be better off using File::Spec->tmpdir() to create the top-level of the testing directories, then creating for_find/ underneath that directory (in most cases, /tmp)? We're already using File::Spec in the find.t, so we would not be adding a dependency. (I want to avoid File::Temp::tempdir() for that reason.) Thank you very much. -- |
From @tonycozOn Thu, Jan 17, 2019 at 05:02:29AM -0800, James E Keenan via RT wrote:
I don't think it will make a difference over just searching within t/for_find. Tony |
From @jkeenanOn Fri, 18 Jan 2019 02:55:24 GMT, tonyc wrote:
Please review patch 0001-find.t-Use-temporary-testing-directory-for-all-block.201901211500.patch, attached. Smoke-testing in this branch: Thank you very much. -- |
From @jkeenan0001-find.t-Use-temporary-testing-directory-for-all-block.201901211500.patchFrom a89df5e81b3b0903bcde25fad6a193fdf754d726 Mon Sep 17 00:00:00 2001
From: James E Keenan <jkeenan@cpan.org>
Date: Wed, 16 Jan 2019 10:56:44 -0500
Subject: [PATCH] find.t: Use temporary testing directory for all blocks of
tests.
During the execution of ext/File-Find/t/taint.t certain files and directories
are created. As inferred from smoke-test reports, when testing in parallel
these entities ran the risk of being detected by certain tests in
ext/File-Find/t/find.t, thereby causing intermittent failures in the smoke
tests.
Problem is being addressed by (a) moving the block of tests created in August 2014
(RT #122547) to prevent processing of misspelled options to a place in the
file where we have moved from the current working directory one level down to
a temporary testing directory; and (b) providing a list of basenames of files
we expect to find therewithin.
---
ext/File-Find/t/find.t | 102 ++++++++++++++++++++++-------------------
1 file changed, 56 insertions(+), 46 deletions(-)
diff --git a/ext/File-Find/t/find.t b/ext/File-Find/t/find.t
index b532752a5a..b0e30eb478 100644
--- a/ext/File-Find/t/find.t
+++ b/ext/File-Find/t/find.t
@@ -90,46 +90,6 @@ finddepth({wanted => sub { ++$::count_taint if $_ eq 'taint.t'; } },
File::Spec->curdir);
is($::count_taint, 1, "'finddepth' found exactly 1 file named 'taint.t'");
-##### RT #122547 #####
-# Do find() and finddepth() correctly warn on invalid options?
-{
- my $bad_option = 'foobar';
- my $second_bad_option = 'really_foobar';
-
- $::count_taint = 0;
- local $SIG{__WARN__} = sub { $warn_msg = $_[0]; };
- {
- find(
- {
- wanted => sub { ++$::count_taint if $_ eq 'taint.t'; },
- $bad_option => undef,
- },
- File::Spec->curdir
- );
- };
- like($warn_msg, qr/Invalid option/s, "Got warning for invalid option");
- like($warn_msg, qr/$bad_option/s, "Got warning for $bad_option");
- is($::count_taint, 1, "count_taint incremented");
- undef $warn_msg;
-
- $::count_taint = 0;
- {
- finddepth(
- {
- wanted => sub { ++$::count_taint if $_ eq 'taint.t'; },
- $bad_option => undef,
- $second_bad_option => undef,
- },
- File::Spec->curdir
- );
- };
- like($warn_msg, qr/Invalid option/s, "Got warning for invalid option");
- like($warn_msg, qr/$bad_option/s, "Got warning for $bad_option");
- like($warn_msg, qr/$second_bad_option/s, "Got warning for $second_bad_option");
- is($::count_taint, 1, "count_taint incremented");
- undef $warn_msg;
-}
-
my $FastFileTests_OK = 0;
sub cleanup {
@@ -283,22 +243,72 @@ sub my_postprocess {
mkdir_ok( dir_path('for_find'), 0770 );
ok( chdir( dir_path('for_find')), "Able to chdir to 'for_find'")
or die("Unable to chdir to 'for_find'");
+
+my @testing_basenames = ( qw| fb_ord fba_ord fa_ord faa_ord fab_ord faba_ord | );
+
mkdir_ok( dir_path('fa'), 0770 );
mkdir_ok( dir_path('fb'), 0770 );
-create_file_ok( file_path('fb', 'fb_ord') );
+create_file_ok( file_path('fb', $testing_basenames[0]) );
mkdir_ok( dir_path('fb', 'fba'), 0770 );
-create_file_ok( file_path('fb', 'fba', 'fba_ord') );
+create_file_ok( file_path('fb', 'fba', $testing_basenames[1]) );
if ($symlink_exists) {
symlink_ok('../fb','fa/fsl');
}
-create_file_ok( file_path('fa', 'fa_ord') );
+create_file_ok( file_path('fa', $testing_basenames[2]) );
mkdir_ok( dir_path('fa', 'faa'), 0770 );
-create_file_ok( file_path('fa', 'faa', 'faa_ord') );
+create_file_ok( file_path('fa', 'faa', $testing_basenames[3]) );
mkdir_ok( dir_path('fa', 'fab'), 0770 );
-create_file_ok( file_path('fa', 'fab', 'fab_ord') );
+create_file_ok( file_path('fa', 'fab', $testing_basenames[4]) );
mkdir_ok( dir_path('fa', 'fab', 'faba'), 0770 );
-create_file_ok( file_path('fa', 'fab', 'faba', 'faba_ord') );
+create_file_ok( file_path('fa', 'fab', 'faba', $testing_basenames[5]) );
+
+##### RT #122547 #####
+# Do find() and finddepth() correctly warn on invalid options?
+##### RT #133771 #####
+# When running tests in parallel, avoid clash with tests in
+# ext/File-Find/t/taint by moving into the temporary testing directory
+# before testing for warnings on invalid options.
+
+my %tb = map { $_ => 1 } @testing_basenames;
+
+{
+ my $bad_option = 'foobar';
+ my $second_bad_option = 'really_foobar';
+
+ $::count_tb = 0;
+ local $SIG{__WARN__} = sub { $warn_msg = $_[0]; };
+ {
+ find(
+ {
+ wanted => sub { ++$::count_tb if $tb{$_}; },
+ $bad_option => undef,
+ },
+ File::Spec->curdir
+ );
+ };
+ like($warn_msg, qr/Invalid option/s, "Got warning for invalid option");
+ like($warn_msg, qr/$bad_option/s, "Got warning for $bad_option");
+ is($::count_tb, scalar(@testing_basenames), "count_tb incremented");
+ undef $warn_msg;
+
+ $::count_tb = 0;
+ {
+ finddepth(
+ {
+ wanted => sub { ++$::count_tb if $tb{$_}; },
+ $bad_option => undef,
+ $second_bad_option => undef,
+ },
+ File::Spec->curdir
+ );
+ };
+ like($warn_msg, qr/Invalid option/s, "Got warning for invalid option");
+ like($warn_msg, qr/$bad_option/s, "Got warning for $bad_option");
+ like($warn_msg, qr/$second_bad_option/s, "Got warning for $second_bad_option");
+ is($::count_tb, scalar(@testing_basenames), "count_tb incremented");
+ undef $warn_msg;
+}
##### Basic tests for find() #####
# Set up list of files we expect to find.
--
2.17.1
|
From @tonycozOn Mon, 21 Jan 2019 12:09:17 -0800, jkeenan wrote:
That looks better. Tony |
From @jkeenanOn Mon, 21 Jan 2019 22:52:57 GMT, tonyc wrote:
Merged to blead in commit 0abd162. Will monitor ticket for several days before closing. Thank you very much. |
From @jkeenanOn Wed, 23 Jan 2019 20:24:27 GMT, jkeenan wrote:
Unfortunately, we don't have the problem licked. See this smoke-test report: http://perl5.test-smoke.org/report/79324 ##### Here it is taint.t that is experiencing failures; the patch applied dealt with find.t. I doubt that the particular ./Configure options had anything to do with the failure. The smokecurrent.log didn't have any more information than is visible at the URL cited above. Needs further investigation. Thank you very much. -- |
From @tonycozOn Mon, 28 Jan 2019 14:52:18 -0800, jkeenan wrote:
Has this happened again since then? It might be due to something unrelated. Tony |
From @jkeenanOn Wed, 13 Feb 2019 23:38:55 GMT, tonyc wrote:
See http://perl5.test-smoke.org/report/81935 -- |
From @jkeenanOn Tue, 12 Mar 2019 16:19:57 GMT, jkeenan wrote:
Also: http://perl5.test-smoke.org/report/82348 -- |
From @jkeenanOn Wed, 13 Feb 2019 23:38:55 GMT, tonyc wrote:
See: http://perl5.test-smoke.org/submatrix?test=../ext/File-Find/t/taint.t The failures from FreeBSD-11 are on the host where I smoke-test blead. In my most frequently used configuration on that host, perl is configured, built and tested 36 times. I get 1 or 2 instances of the failure in taint.t every 2 or 3 smoke-test runs. I have stared at the relevant code off and on for months, but since I have no reliable way of generating the race condition, I can't test any insights that I might have. I think we would be better off (where "better off" means getting us to the point where we can close this ticket) simply restoring ext/File-Find/t to the list of directories which are tested in series (currently at line 192 of t/harness). Please review the patch attached. Thank you very much. -- |
From @jkeenan0001-Run-tests-in-ext-File-Find-t-in-series.patchFrom 48e29007b910dd740fd967de95acadd7d522e71a Mon Sep 17 00:00:00 2001
From: James E Keenan <jkeenan@cpan.org>
Date: Wed, 7 Aug 2019 09:39:56 -0400
Subject: [PATCH] Run tests in ext/File-Find/t in series
For: RT # 133771
---
t/harness | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/harness b/t/harness
index caa2a318b8..b9857fa022 100644
--- a/t/harness
+++ b/t/harness
@@ -189,7 +189,7 @@ if (@ARGV) {
# directory containing such files should be tested in serial order.
#
# Add exceptions to the above rule
- for (qw(ext/Pod-Html/t cpan/IO-Zlib/t)) {
+ for (qw(ext/Pod-Html/t cpan/IO-Zlib/t ext/File-Find/t)) {
$serials{$_} = 1;
}
--
2.17.1
|
From @jkeenanOn Wed, 07 Aug 2019 13:41:57 GMT, jkeenan wrote:
Hearing no objection, pushed to blead in commit 665ac6a. Thank you very much. |
|
@jkeenan - Status changed from 'open' to 'pending release' |
Migrated from rt.perl.org#133771 (status was 'pending release')
Searchable as RT133771$
The text was updated successfully, but these errors were encountered: