Bug 19463

Summary: TestBuilder.t is failing randomly
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Test SuiteAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: m.de.rooy
Version: unspecified   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 25551    
Attachments: Bug 19463: Reduce the chance to have TestBuilder generate twice the same value
Bug 19463: Reduce the chance to have TestBuilder generate twice the same value
Bug 19463: Additional changes to gen_text
Bug 19463: (Follow-up) Cosmetic changes

Description Jonathan Druart 2017-10-13 15:59:54 UTC
Violation of unique constraint in Branch at /kohadevbox/koha/t/lib/TestBuilder.pm line 233.

        #   Failed test 'Testing Koha::Old::Checkouts'
        #   at t/db_dependent/TestBuilder.t line 389.
        #          got: ''
        #     expected: 'Koha::Old::Checkout'
        # Looks like you failed 1 test of 67.

    #   Failed test 'Test all classes'
    #   at t/db_dependent/TestBuilder.t line 391.
    # Looks like you failed 1 test of 6.

#   Failed test 'build_object() tests'
#   at t/db_dependent/TestBuilder.t line 392.
# Looks like you failed 1 test of 12.
[07:55:06] t/db_dependent/TestBuilder.t ................................... 

I guess it tries to generate twice the same value branchcode (unlucky!)
Comment 1 Marcel de Rooy 2017-10-16 07:11:00 UTC
(In reply to Jonathan Druart from comment #0)
> Violation of unique constraint in Branch at
> /kohadevbox/koha/t/lib/TestBuilder.pm line 233.
> 
> I guess it tries to generate twice the same value branchcode (unlucky!)

It tries three times now. Random is not always that random.. Normally, the two additional tries are sufficient, but I have seen it happen too.
We could increase that number?
Comment 2 Jonathan Druart 2017-10-16 11:51:50 UTC
That would mean we have too many objects created then...
Comment 3 Jonathan Druart 2017-10-16 15:31:44 UTC
Violation of unique constraint in Category at /kohadevbox/koha/t/lib/TestBuilder.pm line 240.

        #   Failed test 'Testing Koha::Ratings'
        #   at t/db_dependent/TestBuilder.t line 390.
        #          got: ''
        #     expected: 'Koha::Rating'
        # Looks like you failed 1 test of 70.

    #   Failed test 'Test all classes'
    #   at t/db_dependent/TestBuilder.t line 392.
    # Looks like you failed 1 test of 6.

#   Failed test 'build_object() tests'
#   at t/db_dependent/TestBuilder.t line 393.
# Looks like you failed 1 test of 13.
[15:07:54] t/db_dependent/TestBuilder.t
Comment 4 Jonathan Druart 2017-10-17 14:31:57 UTC
Created attachment 68217 [details] [review]
Bug 19463: Reduce the chance to have TestBuilder generate twice the same value

When an id is generated by TestBuilder (branchcode for instance) and the
size of the generated string is 1, we have too many chances to get
"Violation of unique constraint in Branch".

This patch will enforce the size of the string to be > 1 and the number
of tries to get a different strings will be 5 (instead of 3).
Comment 5 Marcel de Rooy 2017-10-17 15:02:48 UTC
Created attachment 68218 [details] [review]
Bug 19463: Reduce the chance to have TestBuilder generate twice the same value

When an id is generated by TestBuilder (branchcode for instance) and the
size of the generated string is 1, we have too many chances to get
"Violation of unique constraint in Branch".

This patch increases the number of retries to 5.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Amended to make room for follow-up.
Comment 6 Marcel de Rooy 2017-10-17 15:02:52 UTC
Created attachment 68219 [details] [review]
Bug 19463: Additional changes to gen_text

Use Bytes::Random::Secure instead of perl rand.
Return a string from 50 to 100% of $size.
Comment 7 Jonathan Druart 2017-10-17 15:17:54 UTC
Thanks Marcel!

Pushed to master for 17.11.
Comment 8 Marcel de Rooy 2017-10-18 12:08:15 UTC
Created attachment 68244 [details] [review]
Bug 19463: (Follow-up) Cosmetic changes

String::Random version 0.26 (on Jessie) does not yet support the rand_gen
parameter (0.27 does, newest is 0.29 on CPAN now).
So alt_rand is only used in determining the size on Jessie.
That might be enough though.
Adding a documention line in this regard.

Removing the obsolete max parameter.

Note: I timed alt_rand for the creation of a new Bytes::Random::Secure
object each time. But each call is about 0.1 milliseconds. So that
should be fine.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>