Bug 10499 - UT: VirtualShelves.t needs a database transaction
Summary: UT: VirtualShelves.t needs a database transaction
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Test Suite (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal (vote)
Assignee: kenza
QA Contact:
URL:
Keywords:
Depends on: 10290
Blocks:
  Show dependency treegraph
 
Reported: 2013-06-20 13:05 UTC by kenza
Modified: 2014-12-07 20:03 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 10499: VirtualShelves.t - wrap tests in a database connection (3.78 KB, patch)
2013-06-20 13:20 UTC, kenza
Details | Diff | Splinter Review
Bug 10499: VirtualShelves.t - wrap tests in a database connection (3.84 KB, patch)
2013-06-25 08:05 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 10499: VirtualShelves.t - wrap tests in a database connection (3.36 KB, patch)
2013-07-01 08:51 UTC, kenza
Details | Diff | Splinter Review
Bug 10499: VirtualShelves.t - wrap tests in a database connection (3.41 KB, patch)
2013-07-02 14:00 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 10499: VirtualShelves.t - wrap tests in a database connection (3.49 KB, patch)
2013-07-14 07:37 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description kenza 2013-06-20 13:05:22 UTC

    
Comment 1 kenza 2013-06-20 13:20:17 UTC Comment hidden (obsolete)
Comment 2 Marcel de Rooy 2013-06-20 13:52:55 UTC
Could you have a look at bug 10386? It was already there..
Comment 3 kenza 2013-06-21 09:38:57 UTC
The patch 10386 doesn't add the rollback, so i think these two patches are complementary.
Comment 4 kenza 2013-06-25 07:47:24 UTC
I just try the patch and it applies.
Comment 5 Chris Cormack 2013-06-25 08:05:53 UTC Comment hidden (obsolete)
Comment 6 kenza 2013-07-01 08:51:02 UTC Comment hidden (obsolete)
Comment 7 kenza 2013-07-01 09:04:28 UTC
This patch needs signoff again.In fact, the patch 10386 is pushed to master so i rebase this one.
Comment 8 Kyle M Hall 2013-07-02 14:00:19 UTC Comment hidden (obsolete)
Comment 9 Katrin Fischer 2013-07-14 07:37:24 UTC
Created attachment 19619 [details] [review]
[PASSED QA] Bug 10499: VirtualShelves.t - wrap tests in a database connection

Before this patch, the queries in VirtualShelves.t were committed in the
database and have to be removed at the end.
This patch wraps tests in a database connection.

Test plan:
prove t/db_dependent/VirtualShelves.t
VirtualShelves.t .. ok
All tests successful.
Files=1, Tests=72,  1 wallclock secs ( 0.06 usr  0.00 sys +  0.72 cusr  0.06 csys =  0.84 CPU)
Result: PASS

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
All tests pass.
Comment 10 Katrin Fischer 2013-07-14 07:39:45 UTC
And welcome Kenza - thx for your patches :)
Comment 11 Galen Charlton 2013-07-16 14:16:15 UTC
(In reply to kenza from comment #3)
> The patch 10386 doesn't add the rollback, so i think these two patches are
> complementary.

As a general note, it is not necessary to explicitly call $dbh->rollback.  When AutoCommit is off, DBD::mysql automatically issues a rollback when the script ends, and if the script gets killed, the transaction never gets committed either.

That said, it doesn't hurt anything to have the explicit rollback.
Comment 12 Galen Charlton 2013-07-16 14:19:48 UTC
Pushed to master.  Thanks, Kenza!
Comment 13 Marcel de Rooy 2013-07-17 13:57:09 UTC
Although this patch is already pushed, I want to add some remarks:

The code deletes the following lines:
-for(my $i=0; $i<10;$i++){
-    my $shelfnumber = $shelves[$i]->{number};
-    if($shelfnumber<0) {
-        ok(1, 'Skip DelShelf for shelf -1');
-        next;
-    }
-    my $status = DelShelf($shelfnumber);
-    ok(1 == $status, "deleted shelf $shelfnumber and its contents");
-}
I do not agree with deleting those lines. Testing the DelShelf function should be done! Scrutiny :)

I do agree with deleting this line:
-DelBiblio($_) for @biblionumbers;

I do not suggest to revert the patch however or send a followup. Why not? I am rebasing my patch on report 10441 that needs 9032 which is in Signed Off state.
In this patch I will restore the DelShelf code.
Comment 14 Tomás Cohen Arazi 2013-07-25 17:14:48 UTC
This patch has been pushed to 3.12.x, will be in 3.12.3.

Thanks Kenza!