This queue is for tickets about the Text-CSV_XS CPAN distribution.

Report information
The Basics
Id:
107074
Status:
resolved
Priority:
Low/Low
Queue:

People
Owner:
Nobody in particular
Requestors:
jkeen [...] verizon.net
Cc:
AdminCc:

BugTracker
Severity:
Unimportant
Broken in:
(no value)
Fixed in:
(no value)



Subject: t/12_acc.t: Order of unit tests affects outcome of tests
Date: Sun, 13 Sep 2015 19:34:53 -0400
To: bug-Text-CSV_XS@rt.cpan.org
From: James E Keenan <jkeen@verizon.net>
The results of individual tests in t/12_acc.t depend on the results of earlier tests in a way that it is not immediately apparent from the layout of tests in the file. Inserting an additional test at a given point may change the outcome of a test farther down in the file in an unexpected way. Today I cloned Text-CSV_XS from github at commit a143fda784ab54baf765ade9872395b55736ba98. Being a coverage fanatic, one of the first things I did was to run the test suite through Devel::Cover. I was very pleased to see very high test coverage: ##### $ cover -summary Reading database from /home/jkeenan/gitwork/Text-CSV_XS/cover_db ------------------------- ------ ------ ------ ------ ------ ------ File stmt bran cond sub time total ------------------------- ------ ------ ------ ------ ------ ------ blib/lib/Text/CSV_XS.pm 99.6 92.5 80.8 100.0 100.0 93.7 Total 99.6 92.5 80.8 100.0 100.0 93.7 ------------------------- ------ ------ ------ ------ ------ ------ ##### But, being a coverage fanatic, I could not resist the temptation to start writing tests for uncovered branches. What I first saw was an uncovered branch in sub quote(): ##### 301 sub quote 302 { 303 14 14 10 my $self = shift; 304 14 100 20 if (@_) { 305 6 4 my $quote = shift; 306 *** 6 50 11 defined $quote or $quote = ""; ##### Or, when looked at from the "Branches" section of the coverage report: ##### 306 *** 50 0 6 unless defined $quote ##### I acked through the test suite to see where sub quote() was exercised, then I studied the source code. I figured that with the following addition to t/12_acc.t I could cover the uncovered branch at line 306 of CSV_XS.pm. ##### diff --git a/t/12_acc.t b/t/12_acc.t index 90ea17c..347edf1 100644 --- a/t/12_acc.t +++ b/t/12_acc.t @@ -3,7 +3,7 @@ use strict; use warnings; -use Test::More tests => 183; +use Test::More qw(no_plan); # tests => 183; BEGIN { use_ok "Text::CSV_XS"; @@ -50,6 +50,7 @@ is ($csv->sep (";"), ';', "sep (;)"); is ($csv->sep_char (), ';', "sep_char ()"); is ($csv->quote_char ("="), '=', "quote_char (=)"); is ($csv->quote ("="), '=', "quote (=)"); +is ($csv->quote (undef), '', "quote (undef)"); is ($csv->eol (undef), "", "eol (undef)"); is ($csv->eol (""), "", "eol ('')"); is ($csv->eol ("\r"), "\r", "eol (\\r)"); ##### But when I ran this test file, I got a test failure -- but only several tests after the test I added! ##### $> prove -vb t/12_acc.t ... ok 36 - quote (undef) # <- added test ok 37 - eol (undef) ok 38 - eol ('') ok 39 - eol (\r) ok 40 - keep_meta_info (1) ok 41 - always_quote (undef) ok 42 - always_quote (1) ok 43 - allow_loose_quotes (1) ok 44 - allow_loose_escapes (1) ok 45 - allow_unquoted_escape (1) ok 46 - allow_whitespace (1) ok 47 - blank_is_undef (1) ok 48 - empty_is_undef (1) ok 49 - auto_diag (1) ok 50 - auto_diag (2) ok 51 - auto_diag (9) ok 52 - auto_diag ("true") ok 53 - auto_diag ("false") ok 54 - auto_diag (undef) ok 55 - auto_diag ("") ok 56 - diag_verbose (1) ok 57 - diag_verbose (2) ok 58 - diag_verbose (9) ok 59 - diag_verbose ("true") ok 60 - diag_verbose ("false") is ($csv->quote ("="), '=', "quote (=)"); +is ($csv->quote (undef), '', "quote (undef)"); ok 61 - diag_verbose (undef) ok 62 - diag_verbose ("") ok 63 - verbatim (1) ok 64 - quote_space (1) ok 65 - quote_empty (1) ok 66 - escape_null (1) ok 67 - quote_null (1) ok 68 - quote_binary (1) ok 69 - escape_char (\) ok 70 - combine not ok 71 - string ... # Failed test 'string' # at t/12_acc.t line 88. ' got: 'txt =, "Hi!";Yes;;2;;1.09; '; expected: '=txt \=, "Hi!"=;=Yes=;==;=2=;;=1.09=;= # Looks like you failed 1 test of 184. Dubious, test returned 1 (wstat 256, 0x100) Failed 1/184 subtests Test Summary Report ------------------- t/12_acc.t (Wstat: 256 Tests: 184 Failed: 1) Failed test: 71 Non-zero exit status: 1 Files=1, Tests=184, 0 wallclock secs ( 0.03 usr 0.01 sys + 0.03 cusr 0.00 csys = 0.07 CPU) Result: FAIL ##### It took several minutes to figure out how adding a new test after the 35th test caused a failure in test 71. The new test 36 set the quote character to an empty string -- as intended -- but I didn't realize that would have an effect on the 'combine' in test 70 and hence on the 'string' in test 71. Granted, there's no problem with the source code here -- only with the layout of the test file. Since it is only with the 'is ($csv->string, ...' tests that we actually ask what comma-separated record has been created, perhaps the test file should be broken up into several blocks, in each of which a new Text::CSV_XS object is created and which culminates in a '$csv->string' test. Final thought: When I placed the 'quote (undef)' test *ahead of* the 'quote (=)' test, all tests PASSed and when I ran it through Devel::Cover I found I got the expected increase in branch coverage. Thank you very much. Jim Keenan
Subject: Re: [rt.cpan.org #107074] t/12_acc.t: Order of unit tests affects outcome of tests
Date: Mon, 14 Sep 2015 08:16:09 +0200
To: bug-Text-CSV_XS@rt.cpan.org
From: "H.Merijn Brand" <h.m.brand@xs4all.nl>
On Sun, 13 Sep 2015 19:35:29 -0400, "jkeen@verizon.net via RT" <bug-Text-CSV_XS@rt.cpan.org> wrote:
Show quoted text
> The results of individual tests in t/12_acc.t depend on the results of > earlier tests in a way that it is not immediately apparent from the > layout of tests in the file. Inserting an additional test at a given > point may change the outcome of a test farther down in the file in an > unexpected way.
All but "unexpected" are intentional in 12_acc.t I keep this test small and simple. That you found this uncovered code is `unfortunate' as I should have added that when I made $csv->quote an alias to $csv->quote_char.
Show quoted text
> Today I cloned Text-CSV_XS from github at commit > a143fda784ab54baf765ade9872395b55736ba98. Being a coverage fanatic, one > of the first things I did was to run the test suite through > Devel::Cover. I was very pleased to see very high test coverage:
:)
Show quoted text
> ##### > $ cover -summary > Reading database from /home/jkeenan/gitwork/Text-CSV_XS/cover_db > > > ------------------------- ------ ------ ------ ------ ------ ------ > File stmt bran cond sub time total > ------------------------- ------ ------ ------ ------ ------ ------ > blib/lib/Text/CSV_XS.pm 99.6 92.5 80.8 100.0 100.0 93.7 > Total 99.6 92.5 80.8 100.0 100.0 93.7 > ------------------------- ------ ------ ------ ------ ------ ------ > ##### > > But, being a coverage fanatic, I could not resist the temptation to > start writing tests for uncovered branches.
I admit I stopped at a certain point. There were two reasons: • It was impossible to get to 100% due to D::C being incapable (at that moment) to merge cover reports from different versions of perl. As the code has branches for perl-version differences, e.g. for utf8, I could not reach 100% • I tend to write defensive code. Some parts are over-protective and the uncovered parts are simply not reachable, as that would probably mean that the problem is in the perl core or a memory shortage that would cause breakage elsewhere (too).
Show quoted text
> What I first saw was an > uncovered branch in sub quote (): > > ##### > 301 sub quote > 302 { > 303 14 14 10 my $self = shift; > 304 14 100 20 if (@_) { > 305 6 4 my $quote = shift; > 306 *** 6 50 11 defined $quote or $quote > = ""; > ##### > > Or, when looked at from the "Branches" section of the coverage report: > > ##### > 306 *** 50 0 6 unless defined $quote > ##### > > I acked through the test suite to see where sub quote() was exercised, > then I studied the source code. I figured that with the following > addition to t/12_acc.t I could cover the uncovered branch at line 306 of > CSV_XS.pm. > > ##### > diff --git a/t/12_acc.t b/t/12_acc.t > index 90ea17c..347edf1 100644 > --- a/t/12_acc.t > +++ b/t/12_acc.t > @@ -3,7 +3,7 @@ > use strict; > use warnings; > > -use Test::More tests => 183; > +use Test::More qw(no_plan); # tests => 183; > > BEGIN { > use_ok "Text::CSV_XS"; > @@ -50,6 +50,7 @@ is ($csv->sep (";"), ';', > "sep (;)"); > is ($csv->sep_char (), ';', "sep_char ()"); > is ($csv->quote_char ("="), '=', "quote_char (=)"); > is ($csv->quote ("="), '=', "quote (=)"); > +is ($csv->quote (undef), '', "quote (undef)"); > is ($csv->eol (undef), "", "eol (undef)"); > is ($csv->eol (""), "", "eol ('')"); > is ($csv->eol ("\r"), "\r", "eol (\\r)"); > ##### > > But when I ran this test file, I got a test failure -- but only several > tests after the test I added! > > ##### > $> prove -vb t/12_acc.t > ... > ok 36 - quote (undef) # <- added test > ok 37 - eol (undef) > ok 38 - eol ('') > ok 39 - eol (\r) > ok 40 - keep_meta_info (1) > ok 41 - always_quote (undef) > ok 42 - always_quote (1) > ok 43 - allow_loose_quotes (1) > ok 44 - allow_loose_escapes (1) > ok 45 - allow_unquoted_escape (1) > ok 46 - allow_whitespace (1) > ok 47 - blank_is_undef (1) > ok 48 - empty_is_undef (1) > ok 49 - auto_diag (1) > ok 50 - auto_diag (2) > ok 51 - auto_diag (9) > ok 52 - auto_diag ("true") > ok 53 - auto_diag ("false") > ok 54 - auto_diag (undef) > ok 55 - auto_diag ("") > ok 56 - diag_verbose (1) > ok 57 - diag_verbose (2) > ok 58 - diag_verbose (9) > ok 59 - diag_verbose ("true") > ok 60 - diag_verbose ("false") is ($csv->quote ("="), '=', "quote (=)"); > +is ($csv->quote (undef), '', "quote (undef)"); > ok 61 - diag_verbose (undef) > ok 62 - diag_verbose ("") > ok 63 - verbatim (1) > ok 64 - quote_space (1) > ok 65 - quote_empty (1) > ok 66 - escape_null (1) > ok 67 - quote_null (1) > ok 68 - quote_binary (1) > ok 69 - escape_char (\) > ok 70 - combine > not ok 71 - string > ... > # Failed test 'string' > # at t/12_acc.t line 88. > ' got: 'txt =, "Hi!";Yes;;2;;1.09; > '; expected: '=txt \=, "Hi!"=;=Yes=;==;=2=;;=1.09=;= > # Looks like you failed 1 test of 184. > Dubious, test returned 1 (wstat 256, 0x100) > Failed 1/184 subtests > > Test Summary Report > ------------------- > t/12_acc.t (Wstat: 256 Tests: 184 Failed: 1) > Failed test: 71 > Non-zero exit status: 1 > Files=1, Tests=184, 0 wallclock secs ( 0.03 usr 0.01 sys + 0.03 cusr > 0.00 csys = 0.07 CPU) > Result: FAIL > ##### > > It took several minutes to figure out how adding a new test after the > 35th test caused a failure in test 71. The new test 36 set the quote > character to an empty string -- as intended -- but I didn't realize that > would have an effect on the 'combine' in test 70 and hence on the > 'string' in test 71. > > Granted, there's no problem with the source code here -- only with the > layout of the test file. Since it is only with the 'is ($csv->string, > ...' tests that we actually ask what comma-separated record has been > created, perhaps the test file should be broken up into several blocks, > in each of which a new Text::CSV_XS object is created and which > culminates in a '$csv->string' test.
If you look at the test files 20 and up, you'll see that it is exactly what I did.
Show quoted text
> Final thought: When I placed the 'quote (undef)' test *ahead of* the > 'quote (=)' test, all tests PASSed and when I ran it through > Devel::Cover I found I got the expected increase in branch coverage.
The correct test should have been (if all tests were "block"ed is ($csv->quote_char, '"', "check default"); is ($csv->quote, '"', "check default"); is ($csv->quote_char ("="), "=", "check setting"); is ($csv->quote_char (undef), "", "check clearing"); is ($csv->quote ("="), "=", "check setting"); is ($csv->quote (undef), "", "check clearing"); test has been added and pushed -- H.Merijn Brand http://tux.nl Perl Monger http://amsterdam.pm.org/ using perl5.00307 .. 5.21 porting perl5 on HP-UX, AIX, and openSUSE http://mirrors.develooper.com/hpux/ http://www.test-smoke.org/ http://qa.perl.org http://www.goldmark.org/jeff/stupid-disclaimers/

Message body not shown because it is not plain text.



This service runs on Request Tracker, is sponsored by The Perl Foundation, and maintained by Best Practical Solutions.

Please report any issues with rt.cpan.org to rt-cpan-admin@bestpractical.com.