https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20478
Martin Renvoize <[email protected]> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA CC| |martin.renvoize@ptfs-europe | |.com --- Comment #23 from Martin Renvoize <[email protected]> --- (In reply to Jonathan Druart from comment #18) > Great to see tests for this code, even if done in an unusual way. > > Some quick remarks: > 1. Transactions must be done using txn_begin (also note that you are setting > AutoCommit off in the sub and rollback at the end => no sense) > > 2. $ENV{"OVERRIDE_SYSPREF_dateformat"} = 'metric'; > Why not set_preference? Note that for test we usually use mock_preference > (from t::lib::Mocks) to avoid to mess with the cache. > > 3. You could use build_sample_item from bug 21971 to simplify the objects > creation (not pushed yet) > > 4. use Koha::DateUtils instead of DateTime directly > > 5. You could use File::Slurp::read_file to simplify a bit the code > > I would have preferred to see the code moved to a module. It would have > eased the write of the tests and made the code reusable. > > Keeping the SO status to get other QA opinions. I've cleaned up the test a little to make it a bit more 'koha', but I've not gone as far as Jonathan suggests above.. I feel these additional clean ups could be handled separately, like the factoring out of code into a module for example. Code works and passes all tests.. It's also great to see a cronscript with tests. Passing QA -- You are receiving this mail because: You are watching all bug changes. _______________________________________________ Koha-bugs mailing list [email protected] http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
