Bug 33870

Created attachment 151875 [details] [review]
Bug 33870: Make $MAX in Sitemapper::Writer global

This should preferably not be a lexical variable (in case we ever
should call this under Plack). A global variable will also simplify
testing (see next patch).

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 151876 [details] [review]
Bug 33870: Polishing Sitemapper.t

Create subtest.
Remove (useless) mock on DateTime.
Remove T::D::C module, add TestBuilder.
Replace local slurp function by use File::Slurp.

Test plan:
Run t/db_dependent/Sitemapper.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 151877 [details] [review]
Bug 33870: Fix indentation in subtest

Test plan:
git diff -w HEAD~1.. t/db_dependent/Sitemapper.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Hi Marcel, I'm looking at this to attempt an SO but I'm not 100% familiar with tests or Sitemapper.t.

My understanding of this patch is that we're cleaning up Sitemapper.t and bringing it up to par a bit more with the most recent way of writing tests, e.g. Slurp.

$schema->storage->txn_rollback; at the end also seems like something that was missing and I believe it should've always been there?

I don't understand or see the need for the "FIXME" comment, if for the tests we need to make sure the only biblios that exist are the 2 biblios we're creating in the tests, isn't "Koha::Biblios->delete;" a perfectly fine way of doing it?

(In reply to Pedro Amorim from comment #4)
> Hi Marcel, I'm looking at this to attempt an SO but I'm not 100% familiar
> with tests or Sitemapper.t.
> 
> My understanding of this patch is that we're cleaning up Sitemapper.t and
> bringing it up to par a bit more with the most recent way of writing tests,
> e.g. Slurp.
> 
> $schema->storage->txn_rollback; at the end also seems like something that
> was missing and I believe it should've always been there?
Yes

> I don't understand or see the need for the "FIXME" comment, if for the tests
> we need to make sure the only biblios that exist are the 2 biblios we're
> creating in the tests, isn't "Koha::Biblios->delete;" a perfectly fine way
> of doing it?
Yes, it works. And we do it more often in tests. But it is not the most elegant way. So maybe TODO could have been more appropriate?

Created attachment 153453 [details] [review]
Bug 33870: Make $MAX in Sitemapper::Writer global

This should preferably not be a lexical variable (in case we ever
should call this under Plack). A global variable will also simplify
testing (see next patch).

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>

Created attachment 153454 [details] [review]
Bug 33870: Polishing Sitemapper.t

Create subtest.
Remove (useless) mock on DateTime.
Remove T::D::C module, add TestBuilder.
Replace local slurp function by use File::Slurp.

Test plan:
Run t/db_dependent/Sitemapper.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>

Created attachment 153455 [details] [review]
Bug 33870: Fix indentation in subtest

Test plan:
git diff -w HEAD~1.. t/db_dependent/Sitemapper.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>

    # FIXME Would be nice to remove both deletes again
    Koha::Checkouts->delete;
    Koha::Biblios->delete;


We should not do that, even with a FIXME.

(In reply to Jonathan Druart from comment #9)
>     # FIXME Would be nice to remove both deletes again
>     Koha::Checkouts->delete;
>     Koha::Biblios->delete;
> 
> 
> We should not do that, even with a FIXME.

They get removed in bug 33871

Created attachment 155094 [details] [review]
Bug 33870: Make $MAX in Sitemapper::Writer global

This should preferably not be a lexical variable (in case we ever
should call this under Plack). A global variable will also simplify
testing (see next patch).

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Created attachment 155095 [details] [review]
Bug 33870: Polishing Sitemapper.t

Create subtest.
Remove (useless) mock on DateTime.
Remove T::D::C module, add TestBuilder.
Replace local slurp function by use File::Slurp.

Test plan:
Run t/db_dependent/Sitemapper.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Created attachment 155096 [details] [review]
Bug 33870: Fix indentation in subtest

Test plan:
git diff -w HEAD~1.. t/db_dependent/Sitemapper.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>

JD amended patch: tidy

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Pushed to master for 23.11.

Nice work everyone, thanks!

Not backported to 23.05.x