Bug 24817

Summary: Fix timing issues in t/db_dependant/Koha/Cash/Register.t
Product: Koha Reporter: Martin Renvoize <martin.renvoize>
Component: Test SuiteAssignee: Martin Renvoize <martin.renvoize>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: critical    
Priority: P5 - low CC: jonathan.druart+koha, joy, tomascohen
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.05.00
Bug Depends on:    
Bug Blocks: 24815    
Attachments: Bug 24817: Add sleep between db create actions
Bug 24817: Horrible cludge date setting to remove sleep
Bug 24817: Add sleep between db create actions
Bug 24817: Horrible cludge date setting to remove sleep
Bug 24817: Add sleep between db create actions
Bug 24817: Horrible cludge date setting to remove sleep
Bug 24817: Add sleep between db create actions
Bug 24817: Replace sleep calls with date setting in test data

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