Bug 15599

Summary: Unit tests output warnings
Product: Koha Reporter: Kyle M Hall <kyle>
Component: Architecture, internals, and plumbingAssignee: Kyle M Hall <kyle>
Status: CLOSED WORKSFORME QA Contact: Testopia <testopia>
Severity: minor    
Priority: P5 - low CC: ellermaugustus, jonathan.druart, mtompset, tomascohen
Version: unspecified   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 14598    
Bug Blocks:    
Attachments: Bug 15599 - Unit tests output warnings
Bug 15599 - Unit tests output warnings
Bug 15599: Unit tests output warnings

Description Kyle M Hall 2016-01-19 12:54:30 UTC
The unit tests t/db_dependent/Circulation/Returns.t and t/db_dependent/Items.t print out eh warning "item-level_itypes set but no itemtype set for item". This should not happen.
Comment 1 Kyle M Hall 2016-01-19 13:49:15 UTC Comment hidden (obsolete)
Comment 2 Gus Ellerm 2016-01-20 03:28:12 UTC
Tried to test this and still generated errors.
Not sure if it was due to me applying the process wrong or if patch is unusable.
Comment 3 Kyle M Hall 2016-01-20 11:53:38 UTC
(In reply to Gus Ellerm from comment #2)
> Tried to test this and still generated errors.
> Not sure if it was due to me applying the process wrong or if patch is
> unusable.

Everything still works for me. Did you apply bug 14598 first? If so, can you paste the output of the unit tests here?
Comment 4 Gus Ellerm 2016-01-20 19:59:06 UTC
Results for first prove - without patches installed 

kohadev-koha@kohadevbox:/home/vagrant/kohaclone$ prove t/db_dependent/Circulation/Returns.t
t/db_dependent/Circulation/Returns.t .. Subroutine C4::Context::userenv redefined at t/db_dependent/Circulation/Returns.t line 15.
t/db_dependent/Circulation/Returns.t .. ok   
All tests successful.
Files=1, Tests=2,  1 wallclock secs ( 0.03 usr  0.00 sys +  0.98 cusr  0.09 csys =  1.10 CPU)
Result: PASS

Results for second prove - without patches installed 

t/db_dependent/Items.t .. 7/9     
    #   Failed test 'Got Koha::Library from home_branch method'
    #   at t/db_dependent/Items.t line 456.
    #          got: 'Koha::Branch'
    #     expected: 'Koha::Library'
    
    #   Failed test 'Got Koha::Library from holding_branch method'
    #   at t/db_dependent/Items.t line 460.
    #          got: 'Koha::Branch'
    #     expected: 'Koha::Library'
    # Looks like you failed 2 tests of 5.

#   Failed test 'Koha::Item(s) tests'
#   at t/db_dependent/Items.t line 464.
# Looks like you failed 1 test of 9.
t/db_dependent/Items.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/9 subtests 

Test Summary Report
-------------------
t/db_dependent/Items.t (Wstat: 256 Tests: 9 Failed: 1)
  Failed test:  8
  Non-zero exit status: 1
Files=1, Tests=9,  2 wallclock secs ( 0.03 usr  0.03 sys +  1.04 cusr  0.19 csys =  1.29 CPU)
Result: FAIL

Looks like I am missing some sample data. These tests were all done before application off patches.
Comment 5 Mark Tompsett 2016-01-20 20:59:39 UTC
Can't duplicate problem as described in test plan.

mtompset@debian:~/kohaclone$ git checkout -b bug_15599 origin/master
Branch bug_15599 set up to track remote branch master from origin.
Switched to a new branch 'bug_15599'
mtompset@debian:~/kohaclone$ prove t/db_dependent/Circulation/Returns.t
t/db_dependent/Circulation/Returns.t .. Subroutine C4::Context::userenv redefined at t/db_dependent/Circulation/Returns.t line 15.
t/db_dependent/Circulation/Returns.t .. ok
All tests successful.
Files=1, Tests=2,  1 wallclock secs ( 0.02 usr  0.00 sys +  0.90 cusr  0.04 csys =  0.96 CPU)
Result: PASS
mtompset@debian:~/kohaclone$ prove t/db_dependent/Items.t
t/db_dependent/Items.t .. ok
All tests successful.
Files=1, Tests=9,  2 wallclock secs ( 0.02 usr  0.00 sys +  1.16 cusr  0.03 csys =  1.21 CPU)
Result: PASS

Patch does not apply.
mtompset@debian:~/kohaclone$ git bz apply 15599
Bug 15599 - Unit tests output warnings

46869 - Bug 15599 - Unit tests output warnings

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 15599 - Unit tests output warnings
fatal: sha1 information is lacking or useless (t/db_dependent/Circulation/Returns.t).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 15599 - Unit tests output warnings
The copy of the patch that failed is found in:
   /home/mtompset/kohaclone/.git/rebase-apply/patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-15599---Unit-tests-output-warnings-lEGWsI.patch
mtompset@debian:~/kohaclone$

NOTE: I read the patch, and it looks like an improvement to me, but can't sign off. Please rebase first.
Comment 6 Mark Tompsett 2016-01-25 05:30:39 UTC Comment hidden (obsolete)
Comment 7 Jonathan Druart 2016-01-25 11:51:30 UTC
Kyle, the warnings appear for many other tests, see t/db_dependent/Reserves.t for instance.
I don't think this is a right way to fix it.
Comment 8 Mark Tompsett 2016-01-25 13:59:50 UTC
(In reply to Jonathan Druart from comment #7)
> Kyle, the warnings appear for many other tests, see
> t/db_dependent/Reserves.t for instance.
> I don't think this is a right way to fix it.

Perhaps not overall, however, taking "string $foo->bar" and turning it to "string $foobar" is a start. Catching warnings is a good test. Perhaps "$debug and" prepended to the {warn mess} would be fitting, and still allow for the testing added?
Comment 9 Kyle M Hall 2016-01-29 12:18:28 UTC
(In reply to M. Tompsett from comment #8)
> (In reply to Jonathan Druart from comment #7)
> > Kyle, the warnings appear for many other tests, see
> > t/db_dependent/Reserves.t for instance.
> > I don't think this is a right way to fix it.
> 
> Perhaps not overall, however, taking "string $foo->bar" and turning it to
> "string $foobar" is a start. Catching warnings is a good test. Perhaps
> "$debug and" prepended to the {warn mess} would be fitting, and still allow
> for the testing added?

I'm open to whatever solution may be proposed. This is starting to go out of scope. The bug is for just two unit tests. I think the $debug and warn idea sounds good! We could have it set to 1 by default and set it to 0 in our unit tests.
Comment 10 Tomás Cohen Arazi 2016-04-01 15:31:34 UTC
I don't agree with Jonathan on this patch. I think tests (like Reserves.t) should create well-formed scenarios. If they are raising warnings, then they need to be fixed.
And that's not the situation here. because we are explicitly creating this 'bad' scenario, to test the behaviour in that specific scenario. So, worth testing for the warning this time. I might be missing something anyway, so I'm open to discuss this.

It doesn't apply BTW, because of the latest rebase of bug 14598.
Comment 11 Mark Tompsett 2016-04-01 21:32:38 UTC
There are currently a lot of itemtype things going on right now. These two test cases are rather quiet currently.
Comment 12 Tomás Cohen Arazi 2016-04-06 14:02:01 UTC
Created attachment 49973 [details] [review]
Bug 15599: Unit tests output warnings

The unit tests t/db_dependent/Circulation/Returns.t and
t/db_dependent/Items.t print out eh warning "item-level_itypes set but
no itemtype set for item". This should not happen.

Test Plan:
1) prove t/db_dependent/Circulation/Returns.t
2) Note the warning
3) prove t/db_dependent/Items.t
4) Note the warning
5) Apply this patch
6) prove t/db_dependent/Circulation/Returns.t
7) Note there is no warning
8) prove t/db_dependent/Items.t
9) Note there is no warning

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@unc.edu.ar>
Meaningful chages to the tests. Passes qa tests.
Comment 13 Kyle M Hall 2016-05-04 14:00:59 UTC
I pretty sure this patch is no longer necessary.