Bug 24817 - Fix timing issues in t/db_dependant/Koha/Cash/Register.t
Summary: Fix timing issues in t/db_dependant/Koha/Cash/Register.t
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Test Suite (show other bugs)
Version: master
Hardware: All All
: P5 - low critical (vote)
Assignee: Martin Renvoize
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 24815
  Show dependency treegraph
 
Reported: 2020-03-06 08:25 UTC by Martin Renvoize
Modified: 2020-11-30 21:48 UTC (History)
3 users (show)

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


Attachments
Bug 24817: Add sleep between db create actions (2.85 KB, patch)
2020-03-06 08:29 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24817: Horrible cludge date setting to remove sleep (2.55 KB, patch)
2020-03-06 14:47 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24817: Add sleep between db create actions (2.88 KB, patch)
2020-03-07 03:38 UTC, David Nind
Details | Diff | Splinter Review
Bug 24817: Horrible cludge date setting to remove sleep (2.60 KB, patch)
2020-03-07 03:38 UTC, David Nind
Details | Diff | Splinter Review
Bug 24817: Add sleep between db create actions (2.95 KB, patch)
2020-03-09 09:34 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24817: Horrible cludge date setting to remove sleep (2.67 KB, patch)
2020-03-09 09:34 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24817: Add sleep between db create actions (3.01 KB, patch)
2020-03-09 10:39 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24817: Replace sleep calls with date setting in test data (2.74 KB, patch)
2020-03-09 10:39 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize 2020-03-06 08:25:30 UTC
The tests can run at sub-second speeds, but add_cashup and outstanding_accountlines expect as this is a human interaction upon a cash register then we cannot end up with overlapping actions.

We cannot use Time::Fake here as the timestamps are being added at the database trigger level.
Comment 1 Martin Renvoize 2020-03-06 08:29:14 UTC
Created attachment 100215 [details] [review]
Bug 24817: Add sleep between db create actions

We need to allow some time to pass between calls that add cash register
lines to accountlines or cash_register_actions. We use database level
triggers to maintain the timestamp fields, so cannot use Time::Fake, and
the methods we are testing assume that, as human interactions, there
will be at least a second between the last 'sale' on a cash register and
the 'cashup' of that same cash register.
Comment 2 Jonathan Druart 2020-03-06 10:06:32 UTC
Nope, using sleep in tests is wrong.
Why do not you force the timestamp (NOW-1h for instance) for the account lines?
Comment 3 Martin Renvoize 2020-03-06 14:33:14 UTC
(In reply to Jonathan Druart from comment #2)
> Nope, using sleep in tests is wrong.
> Why do not you force the timestamp (NOW-1h for instance) for the account
> lines?

Because I don't want to change add_cashup to accept a timestamp to be passed just to pass the test.
Comment 4 Martin Renvoize 2020-03-06 14:47:32 UTC
Created attachment 100275 [details] [review]
Bug 24817: Horrible cludge date setting to remove sleep
Comment 5 Martin Renvoize 2020-03-06 14:48:24 UTC
That second patch does the trick.. but in my opinion feels much worse than using a sleep.
Comment 6 David Nind 2020-03-07 03:38:24 UTC
Created attachment 100303 [details] [review]
Bug 24817: Add sleep between db create actions

We need to allow some time to pass between calls that add cash register
lines to accountlines or cash_register_actions. We use database level
triggers to maintain the timestamp fields, so cannot use Time::Fake, and
the methods we are testing assume that, as human interactions, there
will be at least a second between the last 'sale' on a cash register and
the 'cashup' of that same cash register.

Signed-off-by: David Nind <david@davidnind.com>
Comment 7 David Nind 2020-03-07 03:38:27 UTC
Created attachment 100304 [details] [review]
Bug 24817: Horrible cludge date setting to remove sleep

Signed-off-by: David Nind <david@davidnind.com>
Comment 8 Jonathan Druart 2020-03-09 09:34:51 UTC
Created attachment 100326 [details] [review]
Bug 24817: Add sleep between db create actions

We need to allow some time to pass between calls that add cash register
lines to accountlines or cash_register_actions. We use database level
triggers to maintain the timestamp fields, so cannot use Time::Fake, and
the methods we are testing assume that, as human interactions, there
will be at least a second between the last 'sale' on a cash register and
the 'cashup' of that same cash register.

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 9 Jonathan Druart 2020-03-09 09:34:55 UTC
Created attachment 100327 [details] [review]
Bug 24817: Horrible cludge date setting to remove sleep

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 10 Jonathan Druart 2020-03-09 09:35:48 UTC
I do not think that's horrible. It's basically what we really want to test.
Comment 11 Martin Renvoize 2020-03-09 10:37:44 UTC
(In reply to Jonathan Druart from comment #10)
> I do not think that's horrible. It's basically what we really want to test.

Thanks Jonathan.. think I'll reword the commit message on push.. I was in a down mood by the end of Friday..
Comment 12 Martin Renvoize 2020-03-09 10:39:30 UTC
Created attachment 100335 [details] [review]
Bug 24817: Add sleep between db create actions

We need to allow some time to pass between calls that add cash register
lines to accountlines or cash_register_actions. We use database level
triggers to maintain the timestamp fields, so cannot use Time::Fake, and
the methods we are testing assume that, as human interactions, there
will be at least a second between the last 'sale' on a cash register and
the 'cashup' of that same cash register.

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 13 Martin Renvoize 2020-03-09 10:39:33 UTC
Created attachment 100336 [details] [review]
Bug 24817: Replace sleep calls with date setting in test data

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 14 Martin Renvoize 2020-03-09 10:40:14 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 15 Joy Nelson 2020-04-02 17:37:57 UTC
'Does not apply' to 19.11.x branch.  not backporting