Bug 27007 - GetMarcSubfieldStructure called with "unsafe" in tests
Summary: GetMarcSubfieldStructure called with "unsafe" in tests
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Test Suite (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
: 26972 (view as bug list)
Depends on: 23807
Blocks:
  Show dependency treegraph
 
Reported: 2020-11-13 08:32 UTC by Jonathan Druart
Modified: 2020-12-10 17:03 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
Version(s) released in:
20.11.00, 20.05.07


Attachments
Bug 27007: Don't call GetMarcSubfieldStructure with unsafe in tests (1.97 KB, patch)
2020-11-13 08:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27007: Don't call GetMarcSubfieldStructure with unsafe in tests (2.03 KB, patch)
2020-11-13 09:40 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2020-11-13 08:32:25 UTC
It does not feel correct to call GetMarcSubfieldStructure with the unsafe flag set in tests:
   my $mss = C4::Biblio::GetMarcSubfieldStructure( '', { unsafe => 1 } );

Some tests are failing is run twice

Use of uninitialized value $code_wanted in string eq at /usr/share/perl5/MARC/Field.pm line 314.
[x~10]
Use of uninitialized value $code_wanted in string eq at /usr/share/perl5/MARC/Field.pm line 314.
                                                                       
    #   Failed test 'Value is mapped correctly for column biblionumber'
    #   at t/db_dependent/Koha/Item.t line 115.          
    #          got: undef                                                                    
    #     expected: '483'                              
[etc.]
Comment 1 Jonathan Druart 2020-11-13 08:33:59 UTC
Created attachment 113585 [details] [review]
Bug 27007: Don't call GetMarcSubfieldStructure with unsafe in tests

It does not feel correct to call GetMarcSubfieldStructure with the unsafe flag set in tests:
   my $mss = C4::Biblio::GetMarcSubfieldStructure( '', { unsafe => 1 } );

Some tests are failing is run twice

Use of uninitialized value $code_wanted in string eq at /usr/share/perl5/MARC/Field.pm line 314.
[x~10]
Use of uninitialized value $code_wanted in string eq at /usr/share/perl5/MARC/Field.pm line 314.

    #   Failed test 'Value is mapped correctly for column biblionumber'
    #   at t/db_dependent/Koha/Item.t line 115.
    #          got: undef
    #     expected: '483'
[etc.]

Test plan:
Run the tests twice, without the patch it fails, with the patch it
passes
Comment 2 Martin Renvoize 2020-11-13 09:40:26 UTC
Created attachment 113587 [details] [review]
Bug 27007: Don't call GetMarcSubfieldStructure with unsafe in tests

It does not feel correct to call GetMarcSubfieldStructure with the unsafe flag set in tests:
   my $mss = C4::Biblio::GetMarcSubfieldStructure( '', { unsafe => 1 } );

Some tests are failing is run twice

Use of uninitialized value $code_wanted in string eq at /usr/share/perl5/MARC/Field.pm line 314.
[x~10]
Use of uninitialized value $code_wanted in string eq at /usr/share/perl5/MARC/Field.pm line 314.

    #   Failed test 'Value is mapped correctly for column biblionumber'
    #   at t/db_dependent/Koha/Item.t line 115.
    #          got: undef
    #     expected: '483'
[etc.]

Test plan:
Run the tests twice, without the patch it fails, with the patch it
passes

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 3 Martin Renvoize 2020-11-13 09:41:26 UTC
I dug into the code and agree with this change.. tests well.. Signing off
Comment 4 Tomás Cohen Arazi 2020-11-13 12:31:05 UTC
I never went back to this, sorry. The idea was that eventually the caller would fetch the mss data, accumulate it as needed, and pass the right one to the method as a parameter.

The context is you will iterate on the biblio's items and it doesn't make sense to retrieve the whole thing.

I promise I'll be back to this on this new cycle.

I agree with the change BTW.
Comment 5 Jonathan Druart 2020-11-13 13:30:24 UTC
*** Bug 26972 has been marked as a duplicate of this bug. ***
Comment 6 Jonathan Druart 2020-11-20 09:37:42 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 7 Andrew Fuerste-Henry 2020-12-09 21:56:46 UTC
Pushed to 20.05.x for 20.05.07
Comment 8 Victor Grousset/tuxayo 2020-12-10 17:03:49 UTC
Not backported to oldoldstable (19.11.x). Feel free to ask if it's needed.