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

DirHandle: Improve test coverage and provide descriptions #15497

Closed
p5pRT opened this issue Aug 5, 2016 · 14 comments
Closed

DirHandle: Improve test coverage and provide descriptions #15497

p5pRT opened this issue Aug 5, 2016 · 14 comments

Comments

@p5pRT
Copy link

p5pRT commented Aug 5, 2016

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

Searchable as RT128856$

@p5pRT
Copy link
Author

p5pRT commented Aug 5, 2016

From @jkeenan

The coverage of DirHandle provided by tests in lib/DirHandle.t is poor and none of the individual tests have descriptions.

The patch attached provides full statement, branch, condition and subroutine coverage and provides descriptions for all tests.

Please review, particularly with respect to VMS and other edge cases.

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

@p5pRT
Copy link
Author

p5pRT commented Aug 5, 2016

From @jkeenan

0001-Add-additional-tests-for-DirHandle-to-improve-covera.patch
From af76633d9ddab4b0bb387be7ec3d0058a6db5a52 Mon Sep 17 00:00:00 2001
From: James E Keenan <jkeenan@cpan.org>
Date: Fri, 5 Aug 2016 19:13:15 -0400
Subject: [PATCH] Add additional tests for DirHandle to improve coverage.

Add descriptions to all tests.
---
 lib/DirHandle.t | 95 ++++++++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 87 insertions(+), 8 deletions(-)

diff --git a/lib/DirHandle.t b/lib/DirHandle.t
index f3a9304..6e3eb01 100644
--- a/lib/DirHandle.t
+++ b/lib/DirHandle.t
@@ -11,7 +11,7 @@ BEGIN {
 }
 
 use DirHandle;
-use Test::More tests => 5;
+use Test::More tests => 28;
 
 # Fetching the list of files in two different ways and expecting them 
 # to be the same is a race condition when tests are running in parallel.
@@ -25,22 +25,101 @@ if ($ENV{PERL_CORE} && -d 'uni') {
 
 $dot = DirHandle->new('.');
 
-is(defined $dot, 1);
+ok(defined $dot, "DirHandle->new returns defined value");
+isa_ok($dot, 'DirHandle');
 
 @a = sort <*>;
 do { $first = $dot->read } while defined($first) && $first =~ /^\./;
-ok(+(grep { $_ eq $first } @a));
+ok(+(grep { $_ eq $first } @a),
+    "Scalar context: First non-dot entry returned by 'read' is found in glob");
 
 @b = sort($first, (grep {/^[^.]/} $dot->read));
-ok(+(join("\0", @a) eq join("\0", @b)));
+ok(+(join("\0", @a) eq join("\0", @b)),
+    "List context: Remaining entries returned by 'read' match glob");
 
-$dot->rewind;
+ok($dot->rewind, "'rewind' method returns true value");
 @c = sort grep {/^[^.]/} $dot->read;
-cmp_ok(join("\0", @b), 'eq', join("\0", @c));
+cmp_ok(join("\0", @b), 'eq', join("\0", @c),
+    "After 'rewind', directory re-read as expected");
 
-$dot->close;
+ok($dot->close, "'close' method returns true value");
 $dot->rewind;
-is(defined $dot->read, '');
+ok(! defined $dot->read,
+    "Having closed the directory handle -- and notwithstanding invocation of 'rewind' -- 'read' returns undefined value");
+
+{
+    local $@;
+    eval { $redot = DirHandle->new( '.', '..' ); };
+    like($@, qr/^usage/,
+        "DirHandle constructor with too many arguments fails as expected");
+}
+
+# Now let's test with directory argument provided to 'open' rather than 'new'
+
+$redot = DirHandle->new();
+ok(defined $redot, "DirHandle->new returns defined value even without provided argument");
+isa_ok($redot, 'DirHandle');
+ok($redot->open('.'), "Explicit call of 'open' method returns true value");
+do { $first = $redot->read } while defined($first) && $first =~ /^\./;
+ok(+(grep { $_ eq $first } @a),
+    "Scalar context: First non-dot entry returned by 'read' is found in glob");
+
+@b = sort($first, (grep {/^[^.]/} $redot->read));
+ok(+(join("\0", @a) eq join("\0", @b)),
+    "List context: Remaining entries returned by 'read' match glob");
+
+ok($redot->rewind, "'rewind' method returns true value");
+@c = sort grep {/^[^.]/} $redot->read;
+cmp_ok(join("\0", @b), 'eq', join("\0", @c),
+    "After 'rewind', directory re-read as expected");
+
+ok($redot->close, "'close' method returns true value");
+$redot->rewind;
+ok(! defined $redot->read,
+    "Having closed the directory handle -- and notwithstanding invocation of 'rewind' -- 'read' returns undefined value");
+
+$undot = DirHandle->new('foobar');
+ok(! defined $undot,
+    "Constructor called with non-existent directory returns undefined value");
+
+# Test error conditions for various methods
+
+$aadot = DirHandle->new();
+ok(defined $aadot, "DirHandle->new returns defined value even without provided argument");
+isa_ok($aadot, 'DirHandle');
+{
+    local $@;
+    eval { $aadot->open('.', '..'); };
+    like($@, qr/^usage/,
+        "'open' called with too many arguments fails as expected");
+}
+ok($aadot->open('.'), "Explicit call of 'open' method returns true value");
+{
+    local $@;
+    eval { $aadot->read('foobar'); };
+    like($@, qr/^usage/,
+        "'read' called with argument fails as expected");
+}
+{
+    local $@;
+    eval { $aadot->close('foobar'); };
+    like($@, qr/^usage/,
+        "'close' called with argument fails as expected");
+}
+{
+    local $@;
+    eval { $aadot->rewind('foobar'); };
+    like($@, qr/^usage/,
+        "'rewind' called with argument fails as expected");
+}
+ok($aadot->DESTROY, "'DESTROY' method returns true value");
+
+{
+    local $@;
+    eval { $bbdot = DirHandle::new(); };
+    like($@, qr/^usage/,
+        "DirHandle called as function but with no arguments fails as expected");
+}
 
 if ($chdir) {
   chdir "..";
-- 
2.7.4

@p5pRT
Copy link
Author

p5pRT commented Aug 15, 2016

From @tonycoz

On Fri Aug 05 16​:17​:41 2016, jkeenan wrote​:

The coverage of DirHandle provided by tests in lib/DirHandle.t is poor
and none of the individual tests have descriptions.

The patch attached provides full statement, branch, condition and
subroutine coverage and provides descriptions for all tests.

Please review, particularly with respect to VMS and other edge cases.

+ok($aadot->DESTROY, "'DESTROY' method returns true value");

We don't want to care about the return value of the DESTROY method.

You don't explicitly test $dh->open("unknown"), nor reading from,
rewinding, closing such (which probably needs a no warnings "io" in
each sub.)

Tony

@p5pRT
Copy link
Author

p5pRT commented Aug 15, 2016

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

@p5pRT
Copy link
Author

p5pRT commented Aug 15, 2016

From @jkeenan

On Sun Aug 14 19​:07​:47 2016, tonyc wrote​:

On Fri Aug 05 16​:17​:41 2016, jkeenan wrote​:

The coverage of DirHandle provided by tests in lib/DirHandle.t is poor
and none of the individual tests have descriptions.

The patch attached provides full statement, branch, condition and
subroutine coverage and provides descriptions for all tests.

Please review, particularly with respect to VMS and other edge cases.

+ok($aadot->DESTROY, "'DESTROY' method returns true value");

We don't want to care about the return value of the DESTROY method.

Can you suggest another, better way to exercise the DESTROY method? (I added this test when I saw through coverage analysis that I was not previously exercising it.)

You don't explicitly test $dh->open("unknown"), nor reading from,
rewinding, closing such (which probably needs a no warnings "io" in
each sub.)

Tony

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

@p5pRT
Copy link
Author

p5pRT commented Aug 15, 2016

From @jkeenan

On Sun Aug 14 19​:07​:47 2016, tonyc wrote​:

On Fri Aug 05 16​:17​:41 2016, jkeenan wrote​:

The coverage of DirHandle provided by tests in lib/DirHandle.t is poor
and none of the individual tests have descriptions.

The patch attached provides full statement, branch, condition and
subroutine coverage and provides descriptions for all tests.

Please review, particularly with respect to VMS and other edge cases.

+ok($aadot->DESTROY, "'DESTROY' method returns true value");

We don't want to care about the return value of the DESTROY method.

You don't explicitly test $dh->open("unknown"), nor reading from,
rewinding, closing such (which probably needs a no warnings "io" in
each sub.)

Tony

Attaching second patch with tests for these cases.

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

@p5pRT
Copy link
Author

p5pRT commented Aug 15, 2016

From @jkeenan

0002-Add-tests-starting-with-open-on-nonexistent-director.patch
From 758135d8aa4e98efa0b2a4738dde6849a43d0a32 Mon Sep 17 00:00:00 2001
From: James E Keenan <jkeenan@cpan.org>
Date: Mon, 15 Aug 2016 08:44:30 -0400
Subject: [PATCH] Add tests starting with 'open' on nonexistent directory.

Per suggestion by Tony Cook.

For: RT #128856
---
 lib/DirHandle.t | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/lib/DirHandle.t b/lib/DirHandle.t
index 6e3eb01..03b71b4 100644
--- a/lib/DirHandle.t
+++ b/lib/DirHandle.t
@@ -11,7 +11,7 @@ BEGIN {
 }
 
 use DirHandle;
-use Test::More tests => 28;
+use Test::More tests => 32;
 
 # Fetching the list of files in two different ways and expecting them 
 # to be the same is a race condition when tests are running in parallel.
@@ -121,6 +121,16 @@ ok($aadot->DESTROY, "'DESTROY' method returns true value");
         "DirHandle called as function but with no arguments fails as expected");
 }
 
+$bbdot = DirHandle->new();
+ok(! $bbdot->open('foobar'),
+    "Calling open method on nonexistent directory returns false value");
+ok(! $bbdot->read(),
+    "Calling read method after failed open method returns faluse value");
+ok(! $bbdot->rewind(),
+    "Calling rewind method after failed open method returns faluse value");
+ok(! $bbdot->close(),
+    "Calling close method after failed open method returns faluse value");
+
 if ($chdir) {
   chdir "..";
 }
-- 
2.7.4

@p5pRT
Copy link
Author

p5pRT commented Aug 22, 2016

From @jkeenan

On Mon Aug 15 05​:47​:57 2016, jkeenan wrote​:

On Sun Aug 14 19​:07​:47 2016, tonyc wrote​:

On Fri Aug 05 16​:17​:41 2016, jkeenan wrote​:

The coverage of DirHandle provided by tests in lib/DirHandle.t is poor
and none of the individual tests have descriptions.

The patch attached provides full statement, branch, condition and
subroutine coverage and provides descriptions for all tests.

Please review, particularly with respect to VMS and other edge cases.

+ok($aadot->DESTROY, "'DESTROY' method returns true value");

We don't want to care about the return value of the DESTROY method.

You don't explicitly test $dh->open("unknown"), nor reading from,
rewinding, closing such (which probably needs a no warnings "io" in
each sub.)

Tony

Attaching second patch with tests for these cases.

Tony C​: Does the second patch address your concerns?

Thank you very much.

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

@p5pRT
Copy link
Author

p5pRT commented Aug 30, 2016

From @jkeenan

On Mon Aug 22 06​:32​:17 2016, jkeenan wrote​:

On Mon Aug 15 05​:47​:57 2016, jkeenan wrote​:

On Sun Aug 14 19​:07​:47 2016, tonyc wrote​:

On Fri Aug 05 16​:17​:41 2016, jkeenan wrote​:

The coverage of DirHandle provided by tests in lib/DirHandle.t is poor
and none of the individual tests have descriptions.

The patch attached provides full statement, branch, condition and
subroutine coverage and provides descriptions for all tests.

Please review, particularly with respect to VMS and other edge cases.

+ok($aadot->DESTROY, "'DESTROY' method returns true value");

We don't want to care about the return value of the DESTROY method.

You don't explicitly test $dh->open("unknown"), nor reading from,
rewinding, closing such (which probably needs a no warnings "io" in
each sub.)

Tony

Attaching second patch with tests for these cases.

Tony C​: Does the second patch address your concerns?

Thank you very much.

Tony C​: Ping re second patch.

Thank you very much.
Jim Keenan

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

@p5pRT
Copy link
Author

p5pRT commented Sep 5, 2016

From @tonycoz

On Tue Aug 30 09​:22​:45 2016, jkeenan wrote​:

Tony C​: Ping re second patch.

There's some spelling errors in the test comments​:

+ "Calling read method after failed open method returns faluse value");
  *****

repeated in 2 other places.

As to DESTROY, all you can really do to test it is to check for resource
leaks, which is more in the domain of tools like valgrind.

Testing the return value of DESTROY doesn't really check that any resources
have been released.

Tony

@p5pRT
Copy link
Author

p5pRT commented Sep 5, 2016

From @jkeenan

On Mon Sep 05 05​:07​:59 2016, tonyc wrote​:

On Tue Aug 30 09​:22​:45 2016, jkeenan wrote​:

Tony C​: Ping re second patch.

There's some spelling errors in the test comments​:

+ "Calling read method after failed open method returns faluse value");
*****

repeated in 2 other places.

As to DESTROY, all you can really do to test it is to check for resource
leaks, which is more in the domain of tools like valgrind.

Testing the return value of DESTROY doesn't really check that any resources
have been released.

Tony

Please review replacement patch attached.

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

@p5pRT
Copy link
Author

p5pRT commented Sep 5, 2016

From @jkeenan

128856-0001-Add-additional-tests-for-DirHandle-to-improve-covera.patch
From 37e2ceb8bb60f11112e271d07b5bd31c9b1670d6 Mon Sep 17 00:00:00 2001
From: James E Keenan <jkeenan@cpan.org>
Date: Fri, 5 Aug 2016 19:13:15 -0400
Subject: [PATCH] Add additional tests for DirHandle to improve coverage.

Add descriptions to all tests. Revise per suggestions by Tony Cook.

For: RT #128856
---
 lib/DirHandle.t | 104 +++++++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 96 insertions(+), 8 deletions(-)

diff --git a/lib/DirHandle.t b/lib/DirHandle.t
index f3a9304..2a131e6 100644
--- a/lib/DirHandle.t
+++ b/lib/DirHandle.t
@@ -11,7 +11,7 @@ BEGIN {
 }
 
 use DirHandle;
-use Test::More tests => 5;
+use Test::More tests => 31;
 
 # Fetching the list of files in two different ways and expecting them 
 # to be the same is a race condition when tests are running in parallel.
@@ -25,22 +25,110 @@ if ($ENV{PERL_CORE} && -d 'uni') {
 
 $dot = DirHandle->new('.');
 
-is(defined $dot, 1);
+ok(defined $dot, "DirHandle->new returns defined value");
+isa_ok($dot, 'DirHandle');
 
 @a = sort <*>;
 do { $first = $dot->read } while defined($first) && $first =~ /^\./;
-ok(+(grep { $_ eq $first } @a));
+ok(+(grep { $_ eq $first } @a),
+    "Scalar context: First non-dot entry returned by 'read' is found in glob");
 
 @b = sort($first, (grep {/^[^.]/} $dot->read));
-ok(+(join("\0", @a) eq join("\0", @b)));
+ok(+(join("\0", @a) eq join("\0", @b)),
+    "List context: Remaining entries returned by 'read' match glob");
 
-$dot->rewind;
+ok($dot->rewind, "'rewind' method returns true value");
 @c = sort grep {/^[^.]/} $dot->read;
-cmp_ok(join("\0", @b), 'eq', join("\0", @c));
+cmp_ok(join("\0", @b), 'eq', join("\0", @c),
+    "After 'rewind', directory re-read as expected");
 
-$dot->close;
+ok($dot->close, "'close' method returns true value");
 $dot->rewind;
-is(defined $dot->read, '');
+ok(! defined $dot->read,
+    "Having closed the directory handle -- and notwithstanding invocation of 'rewind' -- 'read' returns undefined value");
+
+{
+    local $@;
+    eval { $redot = DirHandle->new( '.', '..' ); };
+    like($@, qr/^usage/,
+        "DirHandle constructor with too many arguments fails as expected");
+}
+
+# Now let's test with directory argument provided to 'open' rather than 'new'
+
+$redot = DirHandle->new();
+ok(defined $redot, "DirHandle->new returns defined value even without provided argument");
+isa_ok($redot, 'DirHandle');
+ok($redot->open('.'), "Explicit call of 'open' method returns true value");
+do { $first = $redot->read } while defined($first) && $first =~ /^\./;
+ok(+(grep { $_ eq $first } @a),
+    "Scalar context: First non-dot entry returned by 'read' is found in glob");
+
+@b = sort($first, (grep {/^[^.]/} $redot->read));
+ok(+(join("\0", @a) eq join("\0", @b)),
+    "List context: Remaining entries returned by 'read' match glob");
+
+ok($redot->rewind, "'rewind' method returns true value");
+@c = sort grep {/^[^.]/} $redot->read;
+cmp_ok(join("\0", @b), 'eq', join("\0", @c),
+    "After 'rewind', directory re-read as expected");
+
+ok($redot->close, "'close' method returns true value");
+$redot->rewind;
+ok(! defined $redot->read,
+    "Having closed the directory handle -- and notwithstanding invocation of 'rewind' -- 'read' returns undefined value");
+
+$undot = DirHandle->new('foobar');
+ok(! defined $undot,
+    "Constructor called with non-existent directory returns undefined value");
+
+# Test error conditions for various methods
+
+$aadot = DirHandle->new();
+ok(defined $aadot, "DirHandle->new returns defined value even without provided argument");
+isa_ok($aadot, 'DirHandle');
+{
+    local $@;
+    eval { $aadot->open('.', '..'); };
+    like($@, qr/^usage/,
+        "'open' called with too many arguments fails as expected");
+}
+ok($aadot->open('.'), "Explicit call of 'open' method returns true value");
+{
+    local $@;
+    eval { $aadot->read('foobar'); };
+    like($@, qr/^usage/,
+        "'read' called with argument fails as expected");
+}
+{
+    local $@;
+    eval { $aadot->close('foobar'); };
+    like($@, qr/^usage/,
+        "'close' called with argument fails as expected");
+}
+{
+    local $@;
+    eval { $aadot->rewind('foobar'); };
+    like($@, qr/^usage/,
+        "'rewind' called with argument fails as expected");
+}
+
+{
+    local $@;
+    eval { $bbdot = DirHandle::new(); };
+    like($@, qr/^usage/,
+        "DirHandle called as function but with no arguments fails as expected");
+}
+
+$bbdot = DirHandle->new();
+ok(! $bbdot->open('foobar'),
+    "Calling open method on nonexistent directory returns false value");
+ok(! $bbdot->read(),
+    "Calling read method after failed open method returns false value");
+ok(! $bbdot->rewind(),
+    "Calling rewind method after failed open method returns false value");
+ok(! $bbdot->close(),
+    "Calling close method after failed open method returns false value");
 
 if ($chdir) {
   chdir "..";
-- 
2.7.4

@p5pRT
Copy link
Author

p5pRT commented Sep 8, 2016

From @jkeenan

Hearing no objection to the latest revision of the patch, I applied it to blead in commit a89c828.

Marking ticket resolved.

Thank you very much.

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

@p5pRT
Copy link
Author

p5pRT commented Sep 8, 2016

@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
Projects
None yet
Development

No branches or pull requests

1 participant