Bug 33833 - Remove Test::DBIx::Class from t/SocialData.t
Summary: Remove Test::DBIx::Class from t/SocialData.t
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Test Suite (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 34336
  Show dependency treegraph
 
Reported: 2023-05-25 13:07 UTC by Marcel de Rooy
Modified: 2023-12-28 20:47 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.11.00
Circulation function:


Attachments
Bug 33833: Use database in t/Socialdata.t (4.24 KB, patch)
2023-05-25 13:29 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33833: Add two FIXMEs to SocialData::get_report (1.31 KB, patch)
2023-05-25 13:29 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 40002: Move to db_dependent (645 bytes, patch)
2023-05-25 13:30 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33833: Move to db_dependent (645 bytes, patch)
2023-05-25 13:31 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33833: Use database in t/Socialdata.t (4.25 KB, patch)
2023-05-25 14:48 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 33833: Add two FIXMEs to SocialData::get_report (1.32 KB, patch)
2023-05-25 14:48 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 33833: Move to db_dependent (655 bytes, patch)
2023-05-25 14:48 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 33833: Use database in t/Socialdata.t (4.32 KB, patch)
2023-09-01 10:07 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33833: Add two FIXMEs to SocialData::get_report (1.39 KB, patch)
2023-09-01 10:07 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33833: Move to db_dependent (729 bytes, patch)
2023-09-01 10:07 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2023-05-25 13:07:02 UTC
Removing T::D::C and some polishing. The module is a bit weird.
Comment 1 Marcel de Rooy 2023-05-25 13:29:56 UTC
Created attachment 151679 [details] [review]
Bug 33833: Use database in t/Socialdata.t

Remove T::D::C.
Add subtests.
Remove useless parameter from get_report call.
Add two additional trivial tests.

Note: The module C4::SocialData looks a bit weird.

Test plan:
Run t/SocialData.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 2 Marcel de Rooy 2023-05-25 13:29:58 UTC
Created attachment 151680 [details] [review]
Bug 33833: Add two FIXMEs to SocialData::get_report

Just comments, no test plan.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 3 Marcel de Rooy 2023-05-25 13:30:00 UTC Comment hidden (obsolete)
Comment 4 Marcel de Rooy 2023-05-25 13:31:43 UTC
Created attachment 151682 [details] [review]
Bug 33833: Move to db_dependent

Test plan:
Run t/db_dependent/SocialData.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 5 Owen Leonard 2023-05-25 14:48:36 UTC
Created attachment 151689 [details] [review]
Bug 33833: Use database in t/Socialdata.t

Remove T::D::C.
Add subtests.
Remove useless parameter from get_report call.
Add two additional trivial tests.

Note: The module C4::SocialData looks a bit weird.

Test plan:
Run t/SocialData.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 6 Owen Leonard 2023-05-25 14:48:37 UTC
Created attachment 151690 [details] [review]
Bug 33833: Add two FIXMEs to SocialData::get_report

Just comments, no test plan.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 7 Owen Leonard 2023-05-25 14:48:39 UTC
Created attachment 151691 [details] [review]
Bug 33833: Move to db_dependent

Test plan:
Run t/db_dependent/SocialData.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 8 Marcel de Rooy 2023-05-26 06:01:32 UTC
(In reply to Owen Leonard from comment #7)
> Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Thanks!
Comment 9 Jonathan Druart 2023-06-02 09:42:38 UTC
> Use database in t/Socialdata.t

Why?
Comment 10 Marcel de Rooy 2023-06-02 09:46:20 UTC
(In reply to Jonathan Druart from comment #9)
> > Use database in t/Socialdata.t
> 
> Why?

They are in fact data dependent. And we can simply replace TDC by TestBuilder here.
Comment 11 Jonathan Druart 2023-06-06 10:18:04 UTC
It's data dependent, not database dependent. We should actually use more Test::DBIx::Class IMO. I don't get why we should remove it from these tests.
Comment 12 Marcel de Rooy 2023-06-13 14:20:21 UTC
(In reply to Jonathan Druart from comment #11)
> It's data dependent, not database dependent. We should actually use more
> Test::DBIx::Class IMO. I don't get why we should remove it from these tests.

Yeah, every test is probably data dependent :)
Well, I would rather opt for removing Test::DBIx::Class. I do not see why we should use it for a few tests while we normally build the needed data with TestBuilder. (Sometimes we still access the db while perhaps thinking we dont.) Code maintenance wise easier to have one approach. TDC can oversimplify things too, and eliminate constraints that we actually do need.

In this specific case not very hard to see what happens. The module is strongly connected to the associated table.
I would rather use t for tests that do not need any access to the database or some mock via TDC.
Comment 13 Jonathan Druart 2023-07-19 13:30:51 UTC
Test::DBIx::Class was supposed to be widely used
It allows to write non DB dependent tests, and those non DB deps tests are run when we generate the packages.
So they are tests than can be caught failures early, at least it was their original goal.
I don't mind removing Test::DBIx::Class and rethink the real need of non DB tests, but then we discuss it, vote for it, and remove Test::DBIx::Class completely.
Comment 14 Jonathan Druart 2023-09-01 10:07:13 UTC
Created attachment 155091 [details] [review]
Bug 33833: Use database in t/Socialdata.t

Remove T::D::C.
Add subtests.
Remove useless parameter from get_report call.
Add two additional trivial tests.

Note: The module C4::SocialData looks a bit weird.

Test plan:
Run t/SocialData.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 15 Jonathan Druart 2023-09-01 10:07:16 UTC
Created attachment 155092 [details] [review]
Bug 33833: Add two FIXMEs to SocialData::get_report

Just comments, no test plan.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 16 Jonathan Druart 2023-09-01 10:07:18 UTC
Created attachment 155093 [details] [review]
Bug 33833: Move to db_dependent

Test plan:
Run t/db_dependent/SocialData.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 17 Tomás Cohen Arazi 2023-09-05 17:37:53 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 18 Fridolin Somers 2023-09-08 06:43:43 UTC
Enhancement not backported to 23.05.x