Bug 13457 - Suggestions.t expects sample itemtypes
Summary: Suggestions.t expects sample itemtypes
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:
 
Reported: 2014-12-12 22:02 UTC by Mark Tompsett
Modified: 2015-12-03 22:02 UTC (History)
6 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 13457: Suggestions.t expects sample itemtypes (1.59 KB, patch)
2014-12-12 22:06 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 13457: Suggestions.t expects sample itemtypes (1.69 KB, patch)
2014-12-15 14:06 UTC, Paola Rossi
Details | Diff | Splinter Review
[PASSED QA] Bug 13457: Suggestions.t expects sample itemtypes (1.73 KB, patch)
2014-12-19 16:07 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13457 - Followup for CPL and S codes (2.73 KB, patch)
2014-12-19 17:33 UTC, Mark Tompsett
Details | Diff | Splinter Review
[PASSED QA] Bug 13457: Suggestions.t expects sample itemtypes (1.75 KB, patch)
2014-12-30 13:18 UTC, Paola Rossi
Details | Diff | Splinter Review
Bug 13457 - Followup for CPL and S codes (2.88 KB, patch)
2014-12-30 13:18 UTC, Paola Rossi
Details | Diff | Splinter Review
[PASSED QA] Bug 13457: Suggestions.t expects sample itemtypes (1.76 KB, patch)
2015-01-04 22:45 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 13457 - Followup for CPL and S codes (2.85 KB, patch)
2015-01-04 22:45 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 13457 - Followup for CPL and S codes (2.86 KB, patch)
2015-01-14 15:51 UTC, Mark Tompsett
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Tompsett 2014-12-12 22:02:41 UTC
If you have more than the 8 default itemtypes, prove t/db_dependent/Suggestions.t fails.
Comment 1 Mark Tompsett 2014-12-12 22:06:04 UTC Comment hidden (obsolete)
Comment 2 Paola Rossi 2014-12-15 14:06:33 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2014-12-19 10:45:37 UTC
The tests don't pass if you don't use the sample data.
To me, this patch is not relevant: we should not create all data before launching tests.

For instance the same test file (Suggestions.t) won't pass if you don't have the CPL branchcode and S categorycode.
Comment 4 Kyle M Hall 2014-12-19 16:06:37 UTC
Not sure if I understand what you mean. Wouldn't it be best if we made the UT independent of what's in the database? I think this patch is useful, as I often test with the default data, but may add new branches to it. So while it would be best to have complete data independence, this is at least a small step in the right direction.

(In reply to Jonathan Druart from comment #3)
> The tests don't pass if you don't use the sample data.
> To me, this patch is not relevant: we should not create all data before
> launching tests.
> 
> For instance the same test file (Suggestions.t) won't pass if you don't have
> the CPL branchcode and S categorycode.
Comment 5 Kyle M Hall 2014-12-19 16:07:59 UTC Comment hidden (obsolete)
Comment 6 Mark Tompsett 2014-12-19 16:27:26 UTC
Just tested by deleting CPL and S. Comment #3 requires another patch. Working on it.
Comment 7 Mark Tompsett 2014-12-19 17:33:25 UTC Comment hidden (obsolete)
Comment 8 Jonathan Druart 2014-12-22 09:05:37 UTC
(In reply to Kyle M Hall from comment #4)
> Not sure if I understand what you mean. Wouldn't it be best if we made the
> UT independent of what's in the database?

Yes, of course.

> I think this patch is useful, as I
> often test with the default data, but may add new branches to it. So while
> it would be best to have complete data independence, this is at least a
> small step in the right direction.

I am not sure adding >20 lines per test file is a good idea. Especially because we will c/p the same lines.
If we consider we need to create all data before writing one test, the tests will be more difficult to read than now.
Comment 9 Paola Rossi 2014-12-30 13:18:18 UTC Comment hidden (obsolete)
Comment 10 Paola Rossi 2014-12-30 13:18:53 UTC Comment hidden (obsolete)
Comment 11 Katrin Fischer 2015-01-04 22:45:12 UTC
Created attachment 34919 [details] [review]
[PASSED QA] Bug 13457: Suggestions.t expects sample itemtypes

TEST PLAN
---------
1) Make sure you have more than 8 item types, and preferably
   something with a non-sample default code for itemtypes.
2) prove t/db_dependent/Suggestions.t
   -- this will fail
3) Apply patch
4) prove t/db_dependent/Suggestions.t
   -- this will succeed
5) run koha qa test tools

Signed-off-by: Paola Rossi <paola.rossi@cineca.it>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 12 Katrin Fischer 2015-01-04 22:45:15 UTC Comment hidden (obsolete)
Comment 13 Katrin Fischer 2015-01-04 22:47:15 UTC
It's probably not the best solution, but I think it can be easily redone, once we figured out better ways.
Comment 14 Jonathan Druart 2015-01-05 09:13:48 UTC
(In reply to Katrin Fischer from comment #13)
> It's probably not the best solution, but I think it can be easily redone,
> once we figured out better ways.

IMO what we do now (only pass with specific data) is not perfect but better than the idea behind this patch (copy and paste the data insertion).
Comment 15 Katrin Fischer 2015-01-05 09:15:12 UTC
So you feel it adds too much data? Sorry, not sure I understand yet!
Comment 16 Tomás Cohen Arazi 2015-01-14 13:20:42 UTC
Hi Mark, it doesn't seem to apply on my master branch. Could you please take a look?
Comment 17 Paola Rossi 2015-01-14 15:15:34 UTC
In master 3.19.00.006 t/db_dependent/Suggestions.t has 1 more line:

use C4::Budgets;

So the 2-nd patch would need to be rebased.
Comment 18 Mark Tompsett 2015-01-14 15:51:32 UTC
Created attachment 35254 [details] [review]
[PASSED QA] Bug 13457 - Followup for CPL and S codes

Based on comment #3, this corrects CPL and S issues if they do
not exist in the DB.

TEST PLAN
---------
0) Backup your DB.
1) Clear CPL and S from your DB.
   - delete from borrowers where categorycode='S';
   - delete from categories where categorycode='S';
   - delete from borrowers where branchcode='CPL';
   - delete from branch where branchcode='CPL';
2) prove t/db_dependent/Suggestions.t
   -- This should fail.
3) Apply patch
4) prove t/db_dependent/Suggestions.t
   -- This should work.
5) Intentionally add categorycode 'S' and branchcode 'CPL' back
   into the database.
6) prove t/db_dependent/Suggestions.t
   -- This should work.
7) run koha qa test tools.
8) Restore your DB :)

Signed-off-by: Paola Rossi <paola.rossi@cineca.it>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Rebased. Paola Rossi's comment #17 was correct. Simple fix.
Comment 19 Tomás Cohen Arazi 2015-02-05 18:00:16 UTC
Patches pushed to master.

Thanks Mark!
Comment 20 Chris Cormack 2015-02-09 21:49:09 UTC
Pushed to 3.18.x will be in 3.18.4