Bug 14113 - Silence warnings t/Date.t
Summary: Silence warnings t/Date.t
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Test Suite (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor (vote)
Assignee: Mark Tompsett
QA Contact: Galen Charlton
URL:
Keywords:
Depends on:
Blocks: 8879
  Show dependency treegraph
 
Reported: 2015-05-01 20:33 UTC by Mark Tompsett
Modified: 2016-12-05 21:22 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 14113: Silence t/Dates.t warnings and cleanup output. (5.16 KB, patch)
2015-05-01 20:37 UTC, Mark Tompsett
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 14113: Silence t/Dates.t warnings and cleanup output. (5.29 KB, patch)
2015-05-06 20:01 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 14113: Silence t/Dates.t warnings and cleanup output. (5.86 KB, patch)
2015-05-22 17:13 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 14113: Silence t/Dates.t warnings and cleanup output. (6.02 KB, patch)
2015-05-27 02:57 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 14113: Silence t/Dates.t warnings and cleanup output. (6.13 KB, patch)
2015-05-27 19:02 UTC, Indranil Das Gupta
Details | Diff | Splinter Review
[PASSED QA] Bug 14113: Silence t/Dates.t warnings and cleanup output. (6.20 KB, patch)
2015-05-29 15:40 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Tompsett 2015-05-01 20:33:16 UTC
And clean up the vertical spacing too on the output which is forced on the user.
Comment 1 Mark Tompsett 2015-05-01 20:37:48 UTC Comment hidden (obsolete)
Comment 2 Bernardo Gonzalez Kriegel 2015-05-06 20:01:25 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2015-05-22 08:23:00 UTC
Comment on attachment 38904 [details] [review]
[SIGNED-OFF] Bug 14113: Silence t/Dates.t warnings and cleanup output.

Review of attachment 38904 [details] [review]:
-----------------------------------------------------------------

::: t/Dates.t
@@ +59,3 @@
>  foreach ( @{ $thash{'iso'} } ) {
> +    if ($_ =~ /1952/) {
> +        warning_like { $val = format_date($_); } qr/Illegal date/,

This is confusing I think.
The date is not illegal because it contains 1952, but because the day or month is 0.
Maybe should we test all cases (list all the illegal dates), or be explicit somewhere.
Comment 4 Mark Tompsett 2015-05-22 11:58:30 UTC
(In reply to Jonathan Druart from comment #3)
> Comment on attachment 38904 [details] [review] [review]
> [SIGNED-OFF] Bug 14113: Silence t/Dates.t warnings and cleanup output.
> 
> Review of attachment 38904 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: t/Dates.t
> @@ +59,3 @@
> >  foreach ( @{ $thash{'iso'} } ) {
> > +    if ($_ =~ /1952/) {
> > +        warning_like { $val = format_date($_); } qr/Illegal date/,
> 
> This is confusing I think.
> The date is not illegal because it contains 1952, but because the day or
> month is 0.

Yes, but of the dates in the hash, the only illegal dates have 1952 as the year.

> Maybe should we test all cases (list all the illegal dates), or be explicit
> somewhere.

I didn't want to restructure code. I suppose if we broke the dates into legal and illegal dates and did the same loop twice that would be okay?
Comment 5 Jonathan Druart 2015-05-22 12:23:20 UTC
(In reply to M. Tompsett from comment #4)
> (In reply to Jonathan Druart from comment #3)
> > Comment on attachment 38904 [details] [review] [review] [review]
> > [SIGNED-OFF] Bug 14113: Silence t/Dates.t warnings and cleanup output.
> > 
> > Review of attachment 38904 [details] [review] [review] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: t/Dates.t
> > @@ +59,3 @@
> > >  foreach ( @{ $thash{'iso'} } ) {
> > > +    if ($_ =~ /1952/) {
> > > +        warning_like { $val = format_date($_); } qr/Illegal date/,
> > 
> > This is confusing I think.
> > The date is not illegal because it contains 1952, but because the day or
> > month is 0.
> 
> Yes, but of the dates in the hash, the only illegal dates have 1952 as the
> year.

Yes I understood, but it's confusing :)

> > Maybe should we test all cases (list all the illegal dates), or be explicit
> > somewhere.
> 
> I didn't want to restructure code. I suppose if we broke the dates into
> legal and illegal dates and did the same loop twice that would be okay?

What about checking /^00?-/, /-0?0$/ and /0{4}$/ ?
Comment 6 Mark Tompsett 2015-05-22 17:13:36 UTC Comment hidden (obsolete)
Comment 7 Aleisha Amohia 2015-05-26 22:13:25 UTC
I still see the Illegal Date messages. Did you want to get rid of these or reword them entirely? If this is the expected outcome then I'm happy to sign off on this.
Comment 8 Mark Tompsett 2015-05-27 02:57:54 UTC Comment hidden (obsolete)
Comment 9 Indranil Das Gupta 2015-05-27 19:02:59 UTC Comment hidden (obsolete)
Comment 10 Kyle M Hall 2015-05-29 15:40:28 UTC
Created attachment 39695 [details] [review]
[PASSED QA] Bug 14113: Silence t/Dates.t warnings and cleanup output.

The expectation of a user not freaking out over 'Illegal date'
messages is not sensible. Catch the messages, give a nicer
message.

Also, compress the veritical spacing on the output.

TEST PLAN
---------
1) $ prove t/Dates.t
   -- notice Illegal date messages, and extra lines between
      some sectional messages.
2) Apply patch
3) $ prove t/Dates.t
   -- Notice how nice and clean it is?
      Try it with -v to see all the output, including the
      expected warning type messages.
4) run koha qa test tools.

Test Remarks : complies with test plan.
Signed-off-by: Indranil Das Gupta (L2C2 Technologies) <indradg@gmail.com>a

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 11 Tomás Cohen Arazi 2015-06-01 17:19:53 UTC
Patch pushed to master.

Thanks Mark!