Perl / perl5

šŸŖ The Perl programming language
https://dev.perl.org/perl5/
Other
1.92k stars 549 forks source link

t/op/not.t: Misspecified tests; add descriptions to tests lacking them #12676

Closed p5pRT closed 11 years ago

p5pRT commented 11 years ago

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

Searchable as RT116242$

p5pRT commented 11 years ago

From @jkeenan

Since the St Louis Perl Hackathon in November\, I and others have been writing descriptions for tests in the Perl 5 core distribution's test suite which currently lack them. I have encountered a case where adding descriptions to two tests (a) in one case changes the outcome of the test from PASS to FAIL; and (b) where in both cases the descriptions fail to print -- as if they had never been written in the first place!

Consider tests 4 and 5 in t/op/not.t​:

##### # test not(..) and ! is(! 1\, not 1); is(! 0\, not 0); #####

Currently\, none of the tests in t/op/not.t have descriptions. Hence\, the output of running these two tests is as opaque as that of any other test in that file​:

##### $ ./perl t/op/not.t 1..16 ok 1 ... ok 4 ok 5 ... ok 16 #####

Now\, suppose we add descriptions to all tests in this file. Here I display only the first five​:

##### pass("logical negation of empty list") if not(); is(not()\, 1\, "logical negation of empty list in numeric comparison"); is(not()\, not(0)\,   "logical negation of empty list compared with logical negation of false value");

is(! 1\, not 1\, q{description 1}); is(! 0\, not 0\, q{description 0}); #####

The output I get is​:

##### $ ./perl t/op/xnot.t 1..5 ok 1 - logical negation of empty list ok 2 - logical negation of empty list in numeric comparison ok 3 - logical negation of empty list compared with logical negation of false value ok 4 not ok 5 # Failed test 5 - at t/op/xnot.t line 19 # got "1" # expected "" #####

Where did the descriptions for tests 4 and 5 go? And why is test 5 now failing?

I posed this question on the St Louis and Toronto Perlmongers mailing lists. Thanks to Nathan Nutter\, Scott Smith\, Liam R E Quin\, Olaf Alders\, Uri Guttman\, Tom Legrady\, Fulko Hew for participating in the discussion.

Adapting a diagnostic suggested by Fulko Hew\, let's set aside the "testing" aspect of the problem and define a function which prints out its arguments​:

##### sub examine {print "arg​: \<$_>\n" for @​_;} #####

Let's first pass to this function the 3 arguments provided to is() above\, but without the 'not ' in the second argument​:

##### examine(! 1\, 1\, q{description 1}); examine(! 0\, 0\, q{description 0}); #####

The output​:

##### arg​: \<> arg​: \<1> arg​: \<description 1> arg​: \<1> arg​: \<0> arg​: \<description 0> #####

The high-precedence '!' operator binds to the number following it and (apparently) immediately forces that argument to be evaluated. '! 1' evaluates to Perl-false -- hence the empty string between the angle-brackets in the first line of output. '! 0' evaluates to Perl-true -- hence the '1' in the fourth line of output. As we would intuitively expect\, examine() reports 3 arguments in each call. And why do we intuitively expect that? Because the commas inside the function call are functioning as item separators within a list.

Now let's add the string 'not ' to the second argument in each call​:

##### print "Prepending 'not ' to second argument\n"; examine(! 1\, not 1\, q{description 1}); examine(! 0\, not 0\, q{description 0}); #####

This time\, our overall output is​: ##### arg​: \<> arg​: \<1> arg​: \<description 1> arg​: \<1> arg​: \<0> arg​: \<description 0> Prepending 'not ' to second argument arg​: \<> arg​: \<> arg​: \<1> arg​: \<> #####

Here we're seeing the apparently anomalous results we saw when we tried to add a description to tests 4 and 5 in t/op/not.t. We note​:

1. The high-precedence '!' operator is continuing to bind to the number following it in each call\, forcing immediate evaluation. The first and third lines following "Prepending" correspond to the first and fourth lines above "Prepending".

2. Each call\, however\, is only reporting two "arguments" -- and each "argument" is being evaluated to an empty string (second and fourth lines following "Prepending").

In the discussion on the Perlmonger mailing lists\, Uri Guttman observed that 'not' isn't just a lower precedence version of !. It is also a named function with a prototype.

##### ./perl -le 'print prototype "CORE​::not"' $; #####

Since 'not' as a function is prototyped to expect a scalar argument\, the comma to the right of each of 'not 1' and 'not 0' is no longer functioning as an item separator in list context. Rather (to quote 'perlop')\, "[i]n scalar context it evaluates its left argument\, throws that value away\, then evaluates its right argument and returns that value." The "right argument"\, in these cases\, are q{description 1} and q{description 0}. When evaluated by 'not' -- now serving as a unary negation operator -- the expressions evaluate to Perl-false. Hence the empty strings between the angle brackets in the second and fourth lines after "Prepending".

If you adapt the diagnostic suggested by Fulko to use either Test​::More​::is() or the is() from 't/test.pl'\, you should get similar results.

This suggests that tests in 4 and 5 in t/op/not.t are not conducting a precise test of what you would at first think they are testing. As written\, you would think they're testing that '! 1' and 'not 1' "do the same thing" and that '! 0' and 'not 0' "do the same thing" as well. But the fact that adding a description to the test affects the outcome of the test when run in a testing context means that the tests are not spelled sufficiently precisely.

Hence\, I propose we apply the two patches attached. The first merely puts parentheses around the second argument in each of tests 4 and 5. The second adds descriptions to those two tests and to every other test in the file as well.

Please review.

Thank you very much. Jim Keenan

p5pRT commented 11 years ago

From @jkeenan

0001-Add-parens-around-second-argument-to-tests-4-and-5.patch ```diff From 22adfd31e44fba1a05f3c8a238c1e6a94539996a Mon Sep 17 00:00:00 2001 From: James E Keenan Date: Sat, 29 Dec 2012 11:09:34 -0500 Subject: [PATCH 1/2] Add parens around second argument to tests 4 and 5. If a description were to be added to these tests, in the absence of parentheses the scalar prototype of CORE::not would enforce a scalar context onto the balance of the statement, leading to apparently anomalous behavior, viz., the descriptions would not be printed and test 5 would be reported to FAIL. --- t/op/not.t | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/t/op/not.t b/t/op/not.t index 3d07797..aa3e192 100644 --- a/t/op/not.t +++ b/t/op/not.t @@ -14,8 +14,8 @@ is(not(), 1); is(not(), not(0)); # test not(..) and ! -is(! 1, not 1); -is(! 0, not 0); +is(! 1, (not 1)); +is(! 0, (not 0)); is(! (0, 0), not(0, 0)); # test the return of ! -- 1.6.3.2 ```
p5pRT commented 11 years ago

From @jkeenan

0002-t-op-not.t-Add-descriptions-to-all-tests.patch ```diff From 68b3dfb6c35650d77142c62c9aad62acea4c7b4c Mon Sep 17 00:00:00 2001 From: James E Keenan Date: Sat, 29 Dec 2012 11:23:56 -0500 Subject: [PATCH 2/2] t/op/not.t: Add descriptions to all tests. Also, add note() before tests 4 and 5 explaining rationale for addition of parentheses to second arguments. --- t/op/not.t | 47 +++++++++++++++++++++++++++++++---------------- 1 files changed, 31 insertions(+), 16 deletions(-) diff --git a/t/op/not.t b/t/op/not.t index aa3e192..4aae02c 100644 --- a/t/op/not.t +++ b/t/op/not.t @@ -9,14 +9,19 @@ BEGIN { plan tests => 16; # not() tests -pass() if not(); -is(not(), 1); -is(not(), not(0)); +pass("logical negation of empty list") if not(); +is(not(), 1, "logical negation of empty list in numeric comparison"); +is(not(), not(0), + "logical negation of empty list compared with logical negation of false value"); # test not(..) and ! -is(! 1, (not 1)); -is(! 0, (not 0)); -is(! (0, 0), not(0, 0)); +note("parens needed around second argument in next two tests\nto preserve list context inside function call"); +is(! 1, (not 1), + "high- and low-precedence logical negation of true value"); +is(! 0, (not 0), + "high- and low-precedence logical negation of false value"); +is(! (0, 0), not(0, 0), + "high- and low-precedence logical negation of lists"); # test the return of ! { @@ -24,13 +29,18 @@ is(! (0, 0), not(0, 0)); my $not1 = ! 1; no warnings; - ok($not1 == undef); - ok($not1 == ()); + ok($not1 == undef, + "logical negation (high-precedence) of true value is numerically equal to undefined value"); + ok($not1 == (), + "logical negation (high-precedence) of true value is numerically equal to empty list"); use warnings; - ok($not1 eq ''); - ok($not1 == 0); - ok($not0 == 1); + ok($not1 eq '', + "logical negation (high-precedence) of true value in string context is equal to empty string"); + ok($not1 == 0, + "logical negation (high-precedence) of true value is false in numeric context"); + ok($not0 == 1, + "logical negation (high-precedence) of false value is true in numeric context"); } # test the return of not @@ -39,11 +49,16 @@ is(! (0, 0), not(0, 0)); my $not1 = not 1; no warnings; - ok($not1 == undef); - ok($not1 == ()); + ok($not1 == undef, + "logical negation (low-precedence) of true value is numerically equal to undefined value"); + ok($not1 == (), + "logical negation (low-precedence) of true value is numerically equal to empty list"); use warnings; - ok($not1 eq ''); - ok($not1 == 0); - ok($not0 == 1); + ok($not1 eq '', + "logical negation (low-precedence) of true value in string context is equal to empty string"); + ok($not1 == 0, + "logical negation (low-precedence) of true value is false in numeric context"); + ok($not0 == 1, + "logical negation (low-precedence) of false value is true in numeric context"); } -- 1.6.3.2 ```
p5pRT commented 11 years ago

From @tonycoz

On Sat Dec 29 08​:29​:43 2012\, jkeen@​verizon.net wrote​:

Hence\, I propose we apply the two patches attached. The first merely puts parentheses around the second argument in each of tests 4 and 5. The second adds descriptions to those two tests and to every other test in the file as well.

Please review.

That all makes sense to me.

Tony

p5pRT commented 11 years ago

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

p5pRT commented 11 years ago

From @jkeenan

On Wed Jan 02 19​:37​:18 2013\, tonyc wrote​:

On Sat Dec 29 08​:29​:43 2012\, jkeen@​verizon.net wrote​:

Hence\, I propose we apply the two patches attached. The first merely puts parentheses around the second argument in each of tests 4 and 5. The second adds descriptions to those two tests and to every other test in the file as well.

Please review.

That all makes sense to me.

Tony

Thanks for your feedback.

Applied to blead in these commits​:

commit 129da470b34ba9fe0fc51cba042f0581496b6ce0   Add parens around second argument to tests 4 and 5.

commit 877c9ac819c561aa1bf9560121722ac74ceee0d7   t/op/not.t​: Add descriptions to all tests.

Resolving ticket.

Thank you very much. Jim Keenan

p5pRT commented 11 years ago

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

p5pRT commented 11 years ago

From @ap

* James E Keenan \perlbug\-followup@&#8203;perl\.org [2012-12-29 17​:35]​:

In the discussion on the Perlmonger mailing lists\, Uri Guttman observed that 'not' isn't just a lower precedence version of !. It is also a named function with a prototype.

##### ./perl -le 'print prototype "CORE​::not"' $; #####

Ugh. Whyever is the prototype `$;` rather than just `$`?

The latter would do exactly what one would expect here​:

  $ perl -E 'say not 0\, "foo"'

  $ perl -E 'sub non ($) { !$_[0] } say non 0\, "foo"'   1foo

There must be hysterical raisins for this nonsense\, Iā€™m sureā€¦ it is not just a stupid mistake\, hopefully\, is it? Anyone know why?

Regards\, -- Aristotle Pagaltzis // \<http​://plasmasturm.org/>