Description
Jonathan Druart
2020-11-10 08:48:42 UTC
Created attachment 113547 [details] [review] Bug 26984: Don't delete data in Koha/Item.t tests I don't really understand why I had to lower the number of tests Created attachment 113550 [details] [review] Bug 26984: Make other tests pass if AnonymousPatron is set There is only 1 place where it can be tricky to not remove them as we want exactly 10 patrons to be present in the DB Created attachment 113551 [details] [review] Bug 26984: Don't delete data in Koha/Item.t tests I don't really understand why I had to lower the number of tests Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 113552 [details] [review] Bug 26984: Make other tests pass if AnonymousPatron is set There is only 1 place where it can be tricky to not remove them as we want exactly 10 patrons to be present in the DB Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> The first patch here is similar to bug 26963 - please consider that patch as it solves other issues as well (In reply to Nick Clemens from comment #5) > The first patch here is similar to bug 26963 - please consider that patch as > it solves other issues as well I obsoleted it. Created attachment 113662 [details] [review] Bug 26984: Make tests pass if AnonymousPatron is set There is only 1 place where it can be tricky to not remove them as we want exactly 10 patrons to be present in the DB Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> I expected this patch to touch t::lib::TestBuilder. Am I missing something? (In reply to Tomás Cohen Arazi from comment #8) > I expected this patch to touch t::lib::TestBuilder. Am I missing something? I don't understand, can you explain? If the AnonymousPatron syspref is defined BEFORE you run the tests, then the deletion will fail. We need to adjust the tests to not remove the existing patrons we have in DB. This is an assumption we should keep in mind for all tests and all data (there are few exceptions however). (In reply to Jonathan Druart from comment #9) > (In reply to Tomás Cohen Arazi from comment #8) > > I expected this patch to touch t::lib::TestBuilder. Am I missing something? > > I don't understand, can you explain? > > If the AnonymousPatron syspref is defined BEFORE you run the tests, then the > deletion will fail. We need to adjust the tests to not remove the existing > patrons we have in DB. This is an assumption we should keep in mind for all > tests and all data (there are few exceptions however). To make sure that by default we don't generate an anonymous user. I agree we shouldn't delete all, generally. We won't generate an anonymous user, unless weird case (you have the pref set but the AI for borrowers is not there yet...) Created attachment 113704 [details] [review] Bug 26984: (follow-up) Don't delete patrons and checkouts in Objects.t Rather than counting all and assuriing we get 10, we can count all, add 10, and ensure the numbers still match (In reply to Nick Clemens from comment #12) > Created attachment 113704 [details] [review] [review] > Bug 26984: (follow-up) Don't delete patrons and checkouts in Objects.t > > Rather than counting all and assuriing we get 10, we can count all, add 10, > and ensure the numbers still match We want 10 and only 10 patrons, we don't want pagination: my $patrons = Koha::Patrons->search(); is( $patrons->count, 10, 'Search returns all patrons' ); ok( !$patrons->is_paged, 'Search is not paged' ); I haven't tried, but I am expecting your patch to make the tests fail if there are existing patrons in the DB. Don't they? If so the test is buggy I'd say... (In reply to Jonathan Druart from comment #13) > (In reply to Nick Clemens from comment #12) > > Created attachment 113704 [details] [review] [review] [review] > > Bug 26984: (follow-up) Don't delete patrons and checkouts in Objects.t > > > > Rather than counting all and assuriing we get 10, we can count all, add 10, > > and ensure the numbers still match > > We want 10 and only 10 patrons, we don't want pagination: > > my $patrons = Koha::Patrons->search(); > is( $patrons->count, 10, 'Search returns all patrons' ); > ok( !$patrons->is_paged, 'Search is not paged' ); > > I haven't tried, but I am expecting your patch to make the tests fail if > there are existing patrons in the DB. Don't they? If so the test is buggy > I'd say... As I understand, the resultset shouldn't be paged unless we asked for it to be paged, I don't believe it happens automatically: https://metacpan.org/pod/DBIx::Class::ResultSet#page https://metacpan.org/pod/DBIx::Class::ResultSet#is_paged I don't believe we have any reason to need strictly 10 patrons Yes, you are right, Nick. I misread the test. (In reply to Jonathan Druart from comment #11) > We won't generate an anonymous user, unless weird case (you have the pref > set but the AI for borrowers is not there yet...) We'll see :-D Created attachment 113817 [details] [review] Bug 26984: Make tests pass if AnonymousPatron is set There is only 1 place where it can be tricky to not remove them as we want exactly 10 patrons to be present in the DB Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 113818 [details] [review] Bug 26984: (follow-up) Don't delete patrons and checkouts in Objects.t Rather than counting all and assuriing we get 10, we can count all, add 10, and ensure the numbers still match Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Pushed to master for 20.11, thanks to everybody involved! Pushed to 20.05.x for 20.05.07 Missing dependencies for 19.11.x, can't backport. |