Bug 29403 - dt_from_string should fail if passed more data than expected for the format
Summary: dt_from_string should fail if passed more data than expected for the format
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Martin Renvoize
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 29718 24850 29884
Blocks: 29711
  Show dependency treegraph
 
Reported: 2021-11-03 12:14 UTC by Martin Renvoize
Modified: 2023-06-08 22:26 UTC (History)
6 users (show)

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


Attachments
Bug 29403: Unit tests (1.33 KB, patch)
2021-11-03 12:16 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29403: Match the whole string (916 bytes, patch)
2021-11-03 12:16 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29403: Fix api/v1/patrons.t (1.52 KB, patch)
2021-11-03 13:44 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29403: Unit tests (1.38 KB, patch)
2021-11-29 14:16 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29403: Match the whole string (972 bytes, patch)
2021-11-29 14:17 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29403: Fix api/v1/patrons.t (1.57 KB, patch)
2021-11-29 14:17 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29403: Unit tests (1.38 KB, patch)
2021-12-06 11:11 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29403: Match the whole string (972 bytes, patch)
2021-12-06 11:11 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29403: Fix api/v1/patrons.t (1.57 KB, patch)
2021-12-06 11:11 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29403: Unit tests (1.46 KB, patch)
2021-12-13 11:58 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29403: Match the whole string (1.02 KB, patch)
2021-12-13 11:58 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29403: Fix api/v1/patrons.t (1.65 KB, patch)
2021-12-13 11:58 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29403: Fix unit test for bug 29718 (1.16 KB, patch)
2021-12-21 11:46 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29403: Unit tests (1.45 KB, patch)
2021-12-21 11:48 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29403: Match the whole string (1.02 KB, patch)
2021-12-21 11:48 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29403: Fix api/v1/patrons.t (1.64 KB, patch)
2021-12-21 11:48 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29403: Fix unit test for bug 29718 (1.16 KB, patch)
2021-12-21 11:48 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29403: Unit tests (1.44 KB, patch)
2022-01-13 06:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29403: Match the whole string (1.02 KB, patch)
2022-01-13 06:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29403: Fix api/v1/patrons.t (1.65 KB, patch)
2022-01-13 06:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29403: Fix unit test for bug 29718 (1.16 KB, patch)
2022-01-13 06:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29403: Unit tests (1.43 KB, patch)
2022-01-13 09:30 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29403: Match the whole string (1.02 KB, patch)
2022-01-13 09:30 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29403: Fix api/v1/patrons.t (1.64 KB, patch)
2022-01-13 09:30 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29403: Fix unit test for bug 29718 (1.23 KB, patch)
2022-01-13 09:30 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29403: (QA follow-up) Add comments why test should fail (1.36 KB, patch)
2022-01-14 10:35 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29403: Fix api/v1/patrons.t (1.71 KB, patch)
2022-01-14 14:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29403: Fix unit test for bug 29718 (1.23 KB, patch)
2022-01-14 14:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29403: (QA follow-up) Add comments why test should fail (1.33 KB, patch)
2022-01-14 14:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29403: Unit tests (1.53 KB, patch)
2022-01-18 12:18 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29403: Match the whole string (1.12 KB, patch)
2022-01-18 12:18 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29403: Fix api/v1/patrons.t (1.81 KB, patch)
2022-01-18 12:18 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29403: Fix unit test for bug 29718 (1.40 KB, patch)
2022-01-18 12:18 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29403: (QA follow-up) Add comments why test should fail (1.36 KB, patch)
2022-01-18 12:18 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize 2021-11-03 12:14:32 UTC
Our dt_from_string regex for time is optional.. as such we allow for non-matches.. but we don't check that the string as a whole matches a valid date/datetime string.. thus we don't fail when we should.
Comment 1 Martin Renvoize 2021-11-03 12:16:26 UTC
Created attachment 127246 [details] [review]
Bug 29403: Unit tests

Add a unit test for proper iso date handling
Comment 2 Martin Renvoize 2021-11-03 12:16:29 UTC
Created attachment 127247 [details] [review]
Bug 29403: Match the whole string

This patch wraps the constructed regex such that we expect the entire
regex to match and don't allow leading or trailing garbage.
Comment 3 Martin Renvoize 2021-11-03 12:17:45 UTC
This should be tested in conjunction with the existing API tests.. I'm betting by making this more strict we could be exposing a few issues ;)
Comment 4 Martin Renvoize 2021-11-03 13:44:07 UTC
Created attachment 127254 [details] [review]
Bug 29403: Fix api/v1/patrons.t

t::lib::Dates::compare expects either correct DateTime objects, or a
system formatted datetime strings to be passed.. but as we're testing
the API, we are consistently using RFC3339 strings for date-times and
ISO8601 strings for dates.  We need to therefore pre-process the strings
into DateTime objects for the comparison.
Comment 5 Tomás Cohen Arazi 2021-11-29 14:16:51 UTC
Created attachment 128076 [details] [review]
Bug 29403: Unit tests

Add a unit test for proper iso date handling

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 6 Tomás Cohen Arazi 2021-11-29 14:17:09 UTC
Created attachment 128077 [details] [review]
Bug 29403: Match the whole string

This patch wraps the constructed regex such that we expect the entire
regex to match and don't allow leading or trailing garbage.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 7 Tomás Cohen Arazi 2021-11-29 14:17:19 UTC
Created attachment 128078 [details] [review]
Bug 29403: Fix api/v1/patrons.t

t::lib::Dates::compare expects either correct DateTime objects, or a
system formatted datetime strings to be passed.. but as we're testing
the API, we are consistently using RFC3339 strings for date-times and
ISO8601 strings for dates.  We need to therefore pre-process the strings
into DateTime objects for the comparison.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 8 Martin Renvoize 2021-12-06 09:07:44 UTC
Thanks for testing and SO :)
Comment 9 Tomás Cohen Arazi 2021-12-06 11:11:28 UTC
Created attachment 128272 [details] [review]
Bug 29403: Unit tests

Add a unit test for proper iso date handling

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 10 Tomás Cohen Arazi 2021-12-06 11:11:32 UTC
Created attachment 128273 [details] [review]
Bug 29403: Match the whole string

This patch wraps the constructed regex such that we expect the entire
regex to match and don't allow leading or trailing garbage.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 11 Tomás Cohen Arazi 2021-12-06 11:11:35 UTC
Created attachment 128274 [details] [review]
Bug 29403: Fix api/v1/patrons.t

t::lib::Dates::compare expects either correct DateTime objects, or a
system formatted datetime strings to be passed.. but as we're testing
the API, we are consistently using RFC3339 strings for date-times and
ISO8601 strings for dates.  We need to therefore pre-process the strings
into DateTime objects for the comparison.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 12 Jonathan Druart 2021-12-13 11:58:18 UTC
Created attachment 128450 [details] [review]
Bug 29403: Unit tests

Add a unit test for proper iso date handling

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 13 Jonathan Druart 2021-12-13 11:58:21 UTC
Created attachment 128451 [details] [review]
Bug 29403: Match the whole string

This patch wraps the constructed regex such that we expect the entire
regex to match and don't allow leading or trailing garbage.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 14 Jonathan Druart 2021-12-13 11:58:25 UTC
Created attachment 128452 [details] [review]
Bug 29403: Fix api/v1/patrons.t

t::lib::Dates::compare expects either correct DateTime objects, or a
system formatted datetime strings to be passed.. but as we're testing
the API, we are consistently using RFC3339 strings for date-times and
ISO8601 strings for dates.  We need to therefore pre-process the strings
into DateTime objects for the comparison.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 15 Jonathan Druart 2021-12-13 11:58:38 UTC
In my opinion we should not backport those patches.
Comment 16 Fridolin Somers 2021-12-15 21:51:00 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄
Comment 17 Nick Clemens 2021-12-16 16:07:04 UTC
(In reply to Fridolin Somers from comment #16)
> Pushed to master for 22.05, thanks to everybody involved 🦄

This is causing failures on t/db_dependent/Circulation.t

not ok 17 - CanBookBeRenewed tests
#   Failed test 'CanBookBeRenewed tests'
#   at t/db_dependent/Circulation.t line 1450.
The given date (2021-12-15T16:06:30) does not match the date format (us) at /kohadevbox/koha/Koha/DateUtils.pm line 197.
# Looks like your test exited with 11 just after 17.
Dubious, test returned 11 (wstat 2816, 0xb00)
Comment 18 Fridolin Somers 2021-12-16 23:09:44 UTC
O crap, I see it

Looks like it comes from test on no_auto_renewal_after_hard_limit
Comment 19 Fridolin Somers 2021-12-17 00:27:40 UTC
I revert from master
Comment 20 Tomás Cohen Arazi 2021-12-17 01:18:08 UTC
(In reply to Fridolin Somers from comment #19)
> I revert from master

This bug made things stricter. It highlighted we have fragile spots when it comes to date handling. It is not a bad thing per se on the master branch. But it is not bad to revert anyway.

There are bugs in which Jonathan proposed alternative solutions
Comment 21 Jonathan Druart 2021-12-17 07:54:50 UTC
(In reply to Fridolin Somers from comment #19)
> I revert from master

Ideally we prefer to not revert but fix. It will get messy if you revert every time jenkins is failing after a push :)
Comment 22 Jonathan Druart 2021-12-17 08:09:41 UTC
(In reply to Nick Clemens from comment #17)
> (In reply to Fridolin Somers from comment #16)
> > Pushed to master for 22.05, thanks to everybody involved 🦄
> 
> This is causing failures on t/db_dependent/Circulation.t
> 
> not ok 17 - CanBookBeRenewed tests
> #   Failed test 'CanBookBeRenewed tests'
> #   at t/db_dependent/Circulation.t line 1450.
> The given date (2021-12-15T16:06:30) does not match the date format (us) at
> /kohadevbox/koha/Koha/DateUtils.pm line 197.
> # Looks like your test exited with 11 just after 17.
> Dubious, test returned 11 (wstat 2816, 0xb00)

This one is fun. We are storing a DT into a varchar (rule_value) and so the DT object is stringified using ISO 8601.
And yes, this is closely related to bug 29718.
Comment 23 Martin Renvoize 2021-12-17 10:40:22 UTC
I had a feeling this might throw a few issues.. I kinda hoped to have it pushed and help deal with the repercussions.... or that we'd team together to try and spot repercussions and fix them before the push.  Honestly.. I thought I'd run the whole test suit though, so that's my bad.. I should have got the tests all passing first.
Comment 24 Fridolin Somers 2021-12-17 21:57:40 UTC
Ah good points.

I've dig about an hour trying to fix but couldn't get it work.

> This one is fun. We are storing a DT into a varchar (rule_value) and so the DT object is stringified using ISO 8601.
> And yes, this is closely related to bug 29718.
Ah that is complex for me.
We cant change how DT is stringified right ?
So we better change how we store.
Maybe a Koha::DateUtils dt_to_sql method ?
Comment 25 Jonathan Druart 2021-12-20 10:25:14 UTC
(In reply to Fridolin Somers from comment #24)
> We cant change how DT is stringified right ?

We can, this is the first option described on bug 29718. I don't think it's worth it.

> So we better change how we store.
> Maybe a Koha::DateUtils dt_to_sql method ?

Hum? I am not sure I understand that part. This is DBMS internals, we cannot modify how we store date/datetime/timestamp.
Comment 26 Fridolin Somers 2021-12-21 07:03:51 UTC
(In reply to Jonathan Druart from comment #25)
> 
> > So we better change how we store.
> > Maybe a Koha::DateUtils dt_to_sql method ?
> 
> Hum? I am not sure I understand that part. This is DBMS internals, we cannot
> modify how we store date/datetime/timestamp.

I mean a method to stringify a DT into a string that we store in database.
Called "dt_to_sql" as oposite of "dt_from_string($str, 'sql')".
We may call it "dt_to_string".
Comment 27 Martin Renvoize 2021-12-21 10:23:15 UTC
We don't need a to_sql_string.. that's handled already for db date/datetime/timestamp type fields.. in this case we're not dealing with such a field, we're dealing with a varchar.. which allows any ol' string.

Does bug 29718 not basically resolve the test failure this introduces?
Comment 28 Fridolin Somers 2021-12-21 10:31:17 UTC
(In reply to Martin Renvoize from comment #27)
> Does bug 29718 not basically resolve the test failure this introduces?
I'll test.

In error we see :
The given date (2021-12-15T16:06:30) does not match the date format (us)

Realy strange to see US format there, looks like its a call on "dt_from_string" without explicit format.
Comment 29 Martin Renvoize 2021-12-21 10:44:09 UTC
OK, I'll take a deeper look at the code to see what it's doing then.
Comment 30 Martin Renvoize 2021-12-21 11:15:55 UTC
OK.. I took a deeper look and my assumptions were correct.. so I applied the two patchsets and ran the tests again.. and for me.. they pass as expected?!

Are you sure you had both this patchset and bug 29718 applied?

It is a bit of a funky test... in smart-rules we actually grab the passed date, convert to a datetime, then convert to an iso string and drop the time component. In the test, we're just passing a DateTime object, so it'll include a time element and it'll contextually stringify using the datetime defaults (to ISO including Time - Hense the 'T').

What is happening in our DateUtils is that we try to match on preference format ('us' in this case which will fail) and then fallback to 'sql/iso'.
Comment 31 Martin Renvoize 2021-12-21 11:46:52 UTC Comment hidden (obsolete)
Comment 32 Martin Renvoize 2021-12-21 11:48:19 UTC Comment hidden (obsolete)
Comment 33 Martin Renvoize 2021-12-21 11:48:22 UTC Comment hidden (obsolete)
Comment 34 Martin Renvoize 2021-12-21 11:48:26 UTC Comment hidden (obsolete)
Comment 35 Martin Renvoize 2021-12-21 11:48:30 UTC
Created attachment 128829 [details] [review]
Bug 29403: Fix unit test for bug 29718

Including a 'T' is actually valid for ISO date format, I was being
naughty and writing the test to follow our code rather than writing the
test to conform to the ISO spec first time around. Now we are properly
supporting 'T' in ISO dates with bug 29718, we need to update the test
here to properly reflect an invalid ISO date string.
Comment 36 Martin Renvoize 2021-12-21 11:49:21 UTC
I reversed the dependencies and fixed a badly written test I included here in the first place.
Comment 37 Jonathan Druart 2022-01-13 06:01:30 UTC
Created attachment 129373 [details] [review]
Bug 29403: Unit tests

Add a unit test for proper iso date handling

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 38 Jonathan Druart 2022-01-13 06:01:36 UTC
Created attachment 129374 [details] [review]
Bug 29403: Match the whole string

This patch wraps the constructed regex such that we expect the entire
regex to match and don't allow leading or trailing garbage.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 39 Jonathan Druart 2022-01-13 06:01:47 UTC
Created attachment 129375 [details] [review]
Bug 29403: Fix api/v1/patrons.t

t::lib::Dates::compare expects either correct DateTime objects, or a
system formatted datetime strings to be passed.. but as we're testing
the API, we are consistently using RFC3339 strings for date-times and
ISO8601 strings for dates.  We need to therefore pre-process the strings
into DateTime objects for the comparison.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 40 Jonathan Druart 2022-01-13 06:01:55 UTC
Created attachment 129376 [details] [review]
Bug 29403: Fix unit test for bug 29718

Including a 'T' is actually valid for ISO date format, I was being
naughty and writing the test to follow our code rather than writing the
test to conform to the ISO spec first time around. Now we are properly
supporting 'T' in ISO dates with bug 29718, we need to update the test
here to properly reflect an invalid ISO date string.
Comment 41 Martin Renvoize 2022-01-13 09:30:23 UTC
Created attachment 129391 [details] [review]
Bug 29403: Unit tests

Add a unit test for proper iso date handling

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 42 Martin Renvoize 2022-01-13 09:30:27 UTC
Created attachment 129392 [details] [review]
Bug 29403: Match the whole string

This patch wraps the constructed regex such that we expect the entire
regex to match and don't allow leading or trailing garbage.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 43 Martin Renvoize 2022-01-13 09:30:30 UTC
Created attachment 129393 [details] [review]
Bug 29403: Fix api/v1/patrons.t

t::lib::Dates::compare expects either correct DateTime objects, or a
system formatted datetime strings to be passed.. but as we're testing
the API, we are consistently using RFC3339 strings for date-times and
ISO8601 strings for dates.  We need to therefore pre-process the strings
into DateTime objects for the comparison.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 44 Martin Renvoize 2022-01-13 09:30:34 UTC
Created attachment 129394 [details] [review]
Bug 29403: Fix unit test for bug 29718

Including a 'T' is actually valid for ISO date format, I was being
naughty and writing the test to follow our code rather than writing the
test to conform to the ISO spec first time around. Now we are properly
supporting 'T' in ISO dates with bug 29718, we need to update the test
here to properly reflect an invalid ISO date string.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 45 Martin Renvoize 2022-01-13 09:31:26 UTC
Just a minor rebase to fix the test count
Comment 46 Marcel de Rooy 2022-01-14 09:45:50 UTC
QA: Having a look here now
Comment 47 Marcel de Rooy 2022-01-14 09:54:07 UTC
(In reply to Martin Renvoize from comment #0)
> Our dt_from_string regex for time is optional.. as such we allow for
> non-matches.. but we don't check that the string as a whole matches a valid
> date/datetime string.. thus we don't fail when we should.

A first observation:

The bug title is not matching this description. This description is much wider than the title is suggesting.
Comment 48 Marcel de Rooy 2022-01-14 09:59:18 UTC
-        t::lib::Dates::compare( $updated_on_got, $updated_on_expected, 'updated_on values matched' );
+        t::lib::Dates::compare(
+            dt_from_string( $updated_on_got,      'rfc3339' ),
+            dt_from_string( $updated_on_expected, 'rfc3339' ),
+            'updated_on values matched'
+        );

But what does t::lib::Dates?

sub compare {
    my ( $got, $expected ) = @_;
    my $dt_got      = dt_from_string($got);
    my $dt_expected = dt_from_string($expected);
    my $diff = $dt_got->epoch - $dt_expected->epoch;
    if ( abs($diff) <= 5 ) { return 0 }
    return $diff > 0 ? 1 : -1;
}

So $got en $expected should be strings, right?
Does this actually show that we miss a parameter in sub compare, and we are solving it by twisting the test instead?
Comment 49 Marcel de Rooy 2022-01-14 10:01:50 UTC
(In reply to Jonathan Druart from comment #21)
> (In reply to Fridolin Somers from comment #19)
> > I revert from master
> 
> Ideally we prefer to not revert but fix. It will get messy if you revert
> every time jenkins is failing after a push :)

Sure !
Comment 50 Marcel de Rooy 2022-01-14 10:02:46 UTC
(In reply to Jonathan Druart from comment #15)
> In my opinion we should not backport those patches.

Seems to be wise. Will mark this as enh.
Comment 51 Marcel de Rooy 2022-01-14 10:08:59 UTC
t::lib::Dates::compare expects either correct DateTime objects, or a
system formatted datetime strings to be passed.. but as we're testing
the API, we are consistently using RFC3339 strings for date-times and
ISO8601 strings for dates.  We need to therefore pre-process the strings
into DateTime objects for the comparison.

See comment48
Imo this sub needs more attention.
Comment 52 Marcel de Rooy 2022-01-14 10:15:17 UTC
commit dfcb2b8071582a55e35c47926d4b5c1b8b2a9a59
Author: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Date:   Fri Aug 25 17:56:52 2017 -0300
    Bug 19176: Compare the number of seconds when comparing dates in tests

Five years go fast.
Comment 53 Marcel de Rooy 2022-01-14 10:27:48 UTC
Including a 'T' is actually valid for ISO date format, I was being
naughty and writing the test to follow our code rather than writing the
test to conform to the ISO spec first time around. Now we are properly
supporting 'T' in ISO dates with bug 29718, we need to update the test
here to properly reflect an invalid ISO date string.
-    $dt0 = dt_from_string('2021-11-03T10:16:59+00:00', 'iso');
+    $dt0 = dt_from_string('2021-11-03T10:16:59Z+00:00', 'iso');

This is rather unclear. The changes do not reflect the description.
You dont do anything with T here.
Adding the Z is making the test pass by failure on the combination of Z and +00.
I will add a comment on this line and another one to clarify.
Comment 54 Marcel de Rooy 2022-01-14 10:35:39 UTC
Created attachment 129473 [details] [review]
Bug 29403: (QA follow-up) Add comments why test should fail

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 55 Marcel de Rooy 2022-01-14 10:49:46 UTC
Setting the status to FQA for now. Please adjust or provide feedback on earlier comments.

I have various tests failing but could not quickly decide if that relates to this patch set. Please provide evidence that all tests using dt_from_string still pass.

Thanks!
Comment 56 Martin Renvoize 2022-01-14 12:13:04 UTC
* I've adapted the bug title as requested.
* Before the last submission, I ran the test suite for db_dependant/api/* and everything is passing. I also ran a few other tests that included dt_from_string.. but not all.. nothing failed so far
* The compare function is indeed a bit of a fudge.. but one from quite a while ago.. I'm not sure if we should be attempting to fix that here or not.. I'm also not entirely happy with the shared ISO/SQL/Fallback behaviour.. we're still not being explicit in our iso support (We're allowing for am/pm which is not allowed in ISO).

Not sure what more to do here.. happy to discus for clarifications
Comment 57 Martin Renvoize 2022-01-14 12:14:55 UTC
OK, Running the whole test suite against this now.. will report back
Comment 58 Jonathan Druart 2022-01-14 14:19:35 UTC
(In reply to Marcel de Rooy from comment #48)
> -        t::lib::Dates::compare( $updated_on_got, $updated_on_expected,
> 'updated_on values matched' );
> +        t::lib::Dates::compare(
> +            dt_from_string( $updated_on_got,      'rfc3339' ),
> +            dt_from_string( $updated_on_expected, 'rfc3339' ),
> +            'updated_on values matched'
> +        );
> 
> But what does t::lib::Dates?
> 
> sub compare {
>     my ( $got, $expected ) = @_;
>     my $dt_got      = dt_from_string($got);
>     my $dt_expected = dt_from_string($expected);
>     my $diff = $dt_got->epoch - $dt_expected->epoch;
>     if ( abs($diff) <= 5 ) { return 0 }
>     return $diff > 0 ? 1 : -1;
> }
> 
> So $got en $expected should be strings, right?
> Does this actually show that we miss a parameter in sub compare, and we are
> solving it by twisting the test instead?

Will fix on bug 29884.
Comment 59 Jonathan Druart 2022-01-14 14:25:06 UTC
Created attachment 129486 [details] [review]
Bug 29403: Fix api/v1/patrons.t

t::lib::Dates::compare expects either correct DateTime objects, or a
system formatted datetime strings to be passed.. but as we're testing
the API, we are consistently using RFC3339 strings for date-times and
ISO8601 strings for dates.  We need to therefore pre-process the strings
into DateTime objects for the comparison.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 60 Jonathan Druart 2022-01-14 14:25:11 UTC
Created attachment 129487 [details] [review]
Bug 29403: Fix unit test for bug 29718

Including a 'T' is actually valid for ISO date format, I was being
naughty and writing the test to follow our code rather than writing the
test to conform to the ISO spec first time around. Now we are properly
supporting 'T' in ISO dates with bug 29718, we need to update the test
here to properly reflect an invalid ISO date string.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 61 Jonathan Druart 2022-01-14 14:25:16 UTC
Created attachment 129488 [details] [review]
Bug 29403: (QA follow-up) Add comments why test should fail

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 62 Marcel de Rooy 2022-01-17 08:03:30 UTC
(In reply to Jonathan Druart from comment #58)
> (In reply to Marcel de Rooy from comment #48)
> > -        t::lib::Dates::compare( $updated_on_got, $updated_on_expected,
> > 'updated_on values matched' );
> > +        t::lib::Dates::compare(
> > +            dt_from_string( $updated_on_got,      'rfc3339' ),
> > +            dt_from_string( $updated_on_expected, 'rfc3339' ),
> > +            'updated_on values matched'
> > +        );
> > 
> > But what does t::lib::Dates?
> > 
> > sub compare {
> >     my ( $got, $expected ) = @_;
> >     my $dt_got      = dt_from_string($got);
> >     my $dt_expected = dt_from_string($expected);
> >     my $diff = $dt_got->epoch - $dt_expected->epoch;
> >     if ( abs($diff) <= 5 ) { return 0 }
> >     return $diff > 0 ? 1 : -1;
> > }
> > 
> > So $got en $expected should be strings, right?
> > Does this actually show that we miss a parameter in sub compare, and we are
> > solving it by twisting the test instead?
> 
> Will fix on bug 29884.

That seems to be a good fix, but does not fully address my comment. This compare sub runs dt_from_string on its input. So normally I would expect it to have string parameters. The patrons.t test here is doing a dt conversion already with rfc3339. (Compare calls it again.) It would be more consistent if we would pass strings to compare and possibly a type like RFC3339, ISO8601, etc.
I am seeing a hardcoded <=5, could be a parameter too or at least a constant?
So yes, we are leaving scope here. But compare needs attention, and this patch set triggers it.
Comment 63 Marcel de Rooy 2022-01-17 08:03:53 UTC
(In reply to Martin Renvoize from comment #57)
> OK, Running the whole test suite against this now.. will report back

Thanks. Any results?
Comment 64 Jonathan Druart 2022-01-17 10:12:35 UTC
(In reply to Marcel de Rooy from comment #62)
> (In reply to Jonathan Druart from comment #58)
> > (In reply to Marcel de Rooy from comment #48)
> > > -        t::lib::Dates::compare( $updated_on_got, $updated_on_expected,
> > > 'updated_on values matched' );
> > > +        t::lib::Dates::compare(
> > > +            dt_from_string( $updated_on_got,      'rfc3339' ),
> > > +            dt_from_string( $updated_on_expected, 'rfc3339' ),
> > > +            'updated_on values matched'
> > > +        );
> > > 
> > > But what does t::lib::Dates?
> > > 
> > > sub compare {
> > >     my ( $got, $expected ) = @_;
> > >     my $dt_got      = dt_from_string($got);
> > >     my $dt_expected = dt_from_string($expected);
> > >     my $diff = $dt_got->epoch - $dt_expected->epoch;
> > >     if ( abs($diff) <= 5 ) { return 0 }
> > >     return $diff > 0 ? 1 : -1;
> > > }
> > > 
> > > So $got en $expected should be strings, right?
> > > Does this actually show that we miss a parameter in sub compare, and we are
> > > solving it by twisting the test instead?
> > 
> > Will fix on bug 29884.
> 
> That seems to be a good fix, but does not fully address my comment. This
> compare sub runs dt_from_string on its input. So normally I would expect it
> to have string parameters. The patrons.t test here is doing a dt conversion
> already with rfc3339. (Compare calls it again.) It would be more consistent
> if we would pass strings to compare and possibly a type like RFC3339,
> ISO8601, etc.
> I am seeing a hardcoded <=5, could be a parameter too or at least a constant?
> So yes, we are leaving scope here. But compare needs attention, and this
> patch set triggers it.

It's not very nice I agree, but dt_from_string accepts DTs :)
Comment 65 Jonathan Druart 2022-01-17 10:13:49 UTC
'5' doesn't need to be a parameter. It's a global parameter for Jenkins, 1 and 2 was not enough, we push it to 5.
Comment 66 Martin Renvoize 2022-01-17 15:05:51 UTC
(In reply to Marcel de Rooy from comment #63)
> (In reply to Martin Renvoize from comment #57)
> > OK, Running the whole test suite against this now.. will report back
> 
> Thanks. Any results?

All passed, with the exception of a couple of conflict markers that were in a uncommited local file here.
Comment 67 Marcel de Rooy 2022-01-18 12:18:30 UTC
Created attachment 129552 [details] [review]
Bug 29403: Unit tests

Add a unit test for proper iso date handling

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 68 Marcel de Rooy 2022-01-18 12:18:34 UTC
Created attachment 129553 [details] [review]
Bug 29403: Match the whole string

This patch wraps the constructed regex such that we expect the entire
regex to match and don't allow leading or trailing garbage.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 69 Marcel de Rooy 2022-01-18 12:18:38 UTC
Created attachment 129554 [details] [review]
Bug 29403: Fix api/v1/patrons.t

t::lib::Dates::compare expects either correct DateTime objects, or a
system formatted datetime strings to be passed.. but as we're testing
the API, we are consistently using RFC3339 strings for date-times and
ISO8601 strings for dates.  We need to therefore pre-process the strings
into DateTime objects for the comparison.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 70 Marcel de Rooy 2022-01-18 12:18:42 UTC
Created attachment 129555 [details] [review]
Bug 29403: Fix unit test for bug 29718

Including a 'T' is actually valid for ISO date format, I was being
naughty and writing the test to follow our code rather than writing the
test to conform to the ISO spec first time around. Now we are properly
supporting 'T' in ISO dates with bug 29718, we need to update the test
here to properly reflect an invalid ISO date string.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
The commit message is misleading here. Look at the Z in the code changes.
Comment 71 Marcel de Rooy 2022-01-18 12:18:46 UTC
Created attachment 129556 [details] [review]
Bug 29403: (QA follow-up) Add comments why test should fail

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 72 Marcel de Rooy 2022-01-18 12:20:18 UTC
(In reply to Jonathan Druart from comment #65)
> '5' doesn't need to be a parameter. It's a global parameter for Jenkins, 1
> and 2 was not enough, we push it to 5.

Please cite me completely :) "Or a constant"? No big deal, but having this as a constant is surely better.
Comment 73 Marcel de Rooy 2022-01-18 12:20:51 UTC
(In reply to Martin Renvoize from comment #66)
> (In reply to Marcel de Rooy from comment #63)
> > (In reply to Martin Renvoize from comment #57)
> > > OK, Running the whole test suite against this now.. will report back
> > 
> > Thanks. Any results?
> 
> All passed, with the exception of a couple of conflict markers that were in
> a uncommited local file here.

Great.
Confident enough to pass qa in view of responses etc.
Comment 74 Fridolin Somers 2022-01-28 21:12:52 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄