Right now, we need to add the syspref both in C4::UsageStats and in the UsageStats.t file. The tests should use the list from the lib. To test: 1. Add some syspref to the C4::UsageStats::BuildReport loop 2. Run: $ kshell k$ prove t/db_dependent/UsageStats.t => FAIL: The new syspref is not in the tests, things break
Created attachment 121938 [details] [review] Bug 28565: Move the sysprefs to a reusable sub This patch moves the big sysprefs list to a sub. Behavior shouldn't change. To test: 1. Run: $ kshell k$ prove t/db_dependent/UsageStats.t => SUCCESS: Things work as expected, no changes. 2. Sign off :-D
Created attachment 121939 [details] [review] Bug 28565: Simplify tests This patch makes the tests for UsageStats. This way the tests don't need to be adjusted everytime a syspref is added. To test: 1. Apply the first patch 2. Add some random syspref to C4::UsageStats 3. Run: $ kshell k$ prove t/db_dependent/UsageStats.t => FAIL: Things explode 4. Apply this patch 5. Repeat 3 => SUCCESS: It doesn't explode anymore! 6. Sign off :-D
Created attachment 121986 [details] [review] Bug 28565: Move the sysprefs to a reusable sub This patch moves the big sysprefs list to a sub. Behavior shouldn't change. To test: 1. Run: $ kshell k$ prove t/db_dependent/UsageStats.t => SUCCESS: Things work as expected, no changes. 2. Sign off :-D Signed-off-by: David Nind <david@davidnind.com>
Created attachment 121987 [details] [review] Bug 28565: Simplify tests This patch makes the tests for UsageStats. This way the tests don't need to be adjusted everytime a syspref is added. To test: 1. Apply the first patch 2. Add some random syspref to C4::UsageStats 3. Run: $ kshell k$ prove t/db_dependent/UsageStats.t => FAIL: Things explode 4. Apply this patch 5. Repeat 3 => SUCCESS: It doesn't explode anymore! 6. Sign off :-D Signed-off-by: David Nind <david@davidnind.com>
What's the point of this test? :D
(In reply to Jonathan Druart from comment #5) > What's the point of this test? :D I was wondering :-D I guess we want the test if we are not leaking things (bug 23849)
But it's not the test that will catch it, even less if the list is in a single place now.
(In reply to Jonathan Druart from comment #7) > But it's not the test that will catch it, even less if the list is in a > single place now. I'm fine with dropping the test instead of my patches here. Let me know if you want me to do that. My first instinct was to keep what we have, but make it simpler to maintain.
I would keep the first patch but remove the tests in a second patch.
Created attachment 122289 [details] [review] Bug 28565: Refactor tests Tests were too complex with no gain. They even deleted the whole DB to run (inside a transaction). I refactored to test the same thing, and make it rely (mostly) on existing data. _count behavior is tested implicitly by comparing with Koha::*->count. To test: 1. Run: $ kshell k$ prove t/db_dependent/UsageStats.t => SUCCESS: Tests pass 2. Apply this patch 3. Repeat 1 => SUCCESS: Tests still pass 4. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 122353 [details] [review] Bug 28565: Move the sysprefs to a reusable sub This patch moves the big sysprefs list to a sub. Behavior shouldn't change. To test: 1. Run: $ kshell k$ prove t/db_dependent/UsageStats.t => SUCCESS: Things work as expected, no changes. 2. Sign off :-D Signed-off-by: David Nind <david@davidnind.com>
Created attachment 122354 [details] [review] Bug 28565: Refactor tests Tests were too complex with no gain. They even deleted the whole DB to run (inside a transaction). I refactored to test the same thing, and make it rely (mostly) on existing data. _count behavior is tested implicitly by comparing with Koha::*->count. To test: 1. Run: $ kshell k$ prove t/db_dependent/UsageStats.t => SUCCESS: Tests pass 2. Apply this patch 3. Repeat 1 => SUCCESS: Tests still pass 4. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 123167 [details] [review] Bug 28565: Move the sysprefs to a reusable sub This patch moves the big sysprefs list to a sub. Behavior shouldn't change. To test: 1. Run: $ kshell k$ prove t/db_dependent/UsageStats.t => SUCCESS: Things work as expected, no changes. 2. Sign off :-D Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 123168 [details] [review] Bug 28565: Refactor tests Tests were too complex with no gain. They even deleted the whole DB to run (inside a transaction). I refactored to test the same thing, and make it rely (mostly) on existing data. _count behavior is tested implicitly by comparing with Koha::*->count. To test: 1. Run: $ kshell k$ prove t/db_dependent/UsageStats.t => SUCCESS: Tests pass 2. Apply this patch 3. Repeat 1 => SUCCESS: Tests still pass 4. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Pushed to master for 21.11, thanks to everybody involved!
Enhancement not backported for 21.05