Bug 26984

Summary: Tests are failing if AnonymousPatron is configured
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Test SuiteAssignee: Jonathan Druart <jonathan.druart>
Status: RESOLVED FIXED QA Contact: Tomás Cohen Arazi <tomascohen>
Severity: normal    
Priority: P5 - low CC: andrew, nick, tomascohen, victor
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Text to go in the release notes:
Version(s) released in:
20.11.00, 20.05.07
Bug Depends on: 14708    
Bug Blocks: 27062    
Attachments: Bug 26984: Don't delete data in Koha/Item.t tests
Bug 26984: Make other tests pass if AnonymousPatron is set
Bug 26984: Don't delete data in Koha/Item.t tests
Bug 26984: Make other tests pass if AnonymousPatron is set
Bug 26984: Make tests pass if AnonymousPatron is set
Bug 26984: (follow-up) Don't delete patrons and checkouts in Objects.t
Bug 26984: Make tests pass if AnonymousPatron is set
Bug 26984: (follow-up) Don't delete patrons and checkouts in Objects.t

Description Jonathan Druart 2020-11-10 08:48:42 UTC
After bug 14708 we are blocking the removal of the anonymous patron. In some tests we are deleting data, like patrons, and the tests fail with "Deleting patron failed, AnonymousPatron is not deleteable"

One example is t/db_dependent/Koha/Item.t
#   Failed test 'No tests run for subtest "pickup_locations"'
#   at t/db_dependent/Koha/Item.t line 340.
[Deleting patron failed, AnonymousPatron is not deleteable]# Looks like your test exited with 255 just after 4.

We must not remove data before the tests are run.
Comment 1 Jonathan Druart 2020-11-12 11:23:29 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
Comment 2 Jonathan Druart 2020-11-12 12:12:15 UTC
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
Comment 3 Kyle M Hall 2020-11-12 12:48:55 UTC
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>
Comment 4 Kyle M Hall 2020-11-12 12:49:03 UTC
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>
Comment 5 Nick Clemens 2020-11-12 13:12:18 UTC
The first patch here is similar to bug 26963 - please consider that patch as it solves other issues as well
Comment 6 Jonathan Druart 2020-11-16 13:23:32 UTC
(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.
Comment 7 Jonathan Druart 2020-11-16 13:24:01 UTC
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>
Comment 8 Tomás Cohen Arazi 2020-11-16 23:58:52 UTC
I expected this patch to touch t::lib::TestBuilder. Am I missing something?
Comment 9 Jonathan Druart 2020-11-17 09:46:23 UTC
(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).
Comment 10 Tomás Cohen Arazi 2020-11-17 10:58:59 UTC
(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.
Comment 11 Jonathan Druart 2020-11-17 11:04:13 UTC
We won't generate an anonymous user, unless weird case (you have the pref set but the AI for borrowers is not there yet...)
Comment 12 Nick Clemens 2020-11-17 12:16:45 UTC
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
Comment 13 Jonathan Druart 2020-11-17 12:36:27 UTC
(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...
Comment 14 Nick Clemens 2020-11-17 12:44:35 UTC
(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
Comment 15 Jonathan Druart 2020-11-17 14:29:37 UTC
Yes, you are right, Nick. I misread the test.
Comment 16 Tomás Cohen Arazi 2020-11-19 14:19:07 UTC
(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
Comment 17 Tomás Cohen Arazi 2020-11-19 14:24:12 UTC
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>
Comment 18 Tomás Cohen Arazi 2020-11-19 14:24:17 UTC
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>
Comment 19 Jonathan Druart 2020-11-20 07:18:59 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 20 Andrew Fuerste-Henry 2020-12-09 21:49:37 UTC
Pushed to 20.05.x for 20.05.07
Comment 21 Victor Grousset/tuxayo 2020-12-10 17:00:38 UTC
Missing dependencies for 19.11.x, can't backport.