Bug 18979 - Speed up 'valid-templates.t' tests
Summary: Speed up 'valid-templates.t' tests
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Test Suite (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
Depends on: 18055 18993
Blocks:
  Show dependency treegraph
 
Reported: 2017-07-24 20:25 UTC by Jonathan Druart
Modified: 2018-06-04 20:15 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 18979: Speed up 'valid-templates.t' tests (1.67 KB, patch)
2017-07-24 20:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18979: Speed up 'valid-templates.t' tests (1.66 KB, patch)
2017-09-29 18:36 UTC, David Bourgault
Details | Diff | Splinter Review
Bug 18979: Swap threads for Test2::Suite (1.96 KB, patch)
2017-11-15 08:03 UTC, Mason James
Details | Diff | Splinter Review
Bug 18979: Speed up 'valid-templates.t' tests (1.81 KB, patch)
2018-02-13 15:39 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2017-07-24 20:25:09 UTC

    
Comment 1 Jonathan Druart 2017-07-24 20:28:02 UTC
Created attachment 65208 [details] [review]
Bug 18979: Speed up 'valid-templates.t' tests

Test plan:
Confirm than prove xt/author/valid-templates.t is quicker with this
patch.

Before I got:
Files=1, Tests=840, 21 wallclock secs ( 0.18 usr  0.02 sys + 20.28 cusr
0.32 csys = 20.80 CPU)

After:
Files=1, Tests=840, 17 wallclock secs ( 0.06 usr  0.01 sys + 21.56 cusr
0.48 csys = 22.11 CPU)
Comment 2 Marc Véron 2017-07-26 13:34:40 UTC
Testing on top of Bug 18055

Testing without patch:
Files=1, Tests=838, 40 wallclock secs ( 0.17 usr  0.01 sys + 28.71 cusr  0.73 csys = 29.62 CPU)

With patch:
 Can't locate Parallel/ForkManager.pm (...) you may need to install the Parallel::ForkManager module
-> Installing Parallel::ForkManager

Can't locate Sys/CPU.pm
->Installing Sys::CPU


Re-testing without patch (4 times):
Files=1, Tests=838, 33 wallclock secs ( 0.15 usr  0.02 sys + 28.68 cusr  0.38 csys = 29.23 CPU)

Files=1, Tests=838, 30 wallclock secs ( 0.14 usr  0.01 sys + 28.08 cusr  0.32 csys = 28.55 CPU)

Files=1, Tests=838, 30 wallclock secs ( 0.14 usr  0.01 sys + 28.61 cusr  0.22 csys = 28.98 CPU)

Files=1, Tests=838, 31 wallclock secs ( 0.15 usr  0.01 sys + 28.34 cusr  0.27 csys = 28.77 CPU)


Testing with patch (4 times):
Files=1, Tests=838, 36 wallclock secs ( 0.06 usr  0.00 sys + 31.04 cusr  0.60 csys = 31.70 CPU)

Files=1, Tests=838, 34 wallclock secs ( 0.07 usr  0.00 sys + 29.98 cusr  0.53 csys = 30.58 CPU)

Files=1, Tests=838, 32 wallclock secs ( 0.06 usr  0.00 sys + 29.53 cusr  0.65 csys = 30.24 CPU)

Files=1, Tests=838, 32 wallclock secs ( 0.05 usr  0.01 sys + 29.74 cusr  0.53 csys = 30.33 CPU)


Result on my vm: 
It seems that installing Parallel::ForkManager and Sys::CPU speeded up xt/author/valid-templates.t
(31 - 33 wallclock sec instead of 40 sec, without patch)

With patch, xt/author/valid-templates.t was comparable or slower 
(32 - 36 wallclock sec, with patch)
Comment 3 David Bourgault 2017-09-29 18:36:39 UTC
Created attachment 67484 [details] [review]
Bug 18979: Speed up 'valid-templates.t' tests

Test plan:
Confirm than prove xt/author/valid-templates.t is quicker with this
patch.

Before I got:
Files=1, Tests=840, 21 wallclock secs ( 0.18 usr  0.02 sys + 20.28 cusr
0.32 csys = 20.80 CPU)

After:
Files=1, Tests=840, 17 wallclock secs ( 0.06 usr  0.01 sys + 21.56 cusr
0.48 csys = 22.11 CPU)


Signed-off by: David Bourgault <david.bourgault@inlibro.com>
Comment 4 David Bourgault 2017-09-29 18:37:59 UTC
I tested 10 times, before and after.

Before it averaged 18.4 wallclock seconds per run
After it average dropped slightly to 16.6 wallclock seconds

It's a very minor improvement, but there seems to be one.
Comment 5 Marcel de Rooy 2017-10-20 10:29:24 UTC
Files=1, Tests=842, 42 wallclock secs ( 0.20 usr  0.02 sys + 41.23 cusr  0.40 csys = 41.85 CPU)

Files=1, Tests=842, 37 wallclock secs ( 0.06 usr  0.01 sys + 42.91 cusr  0.84 csys = 43.82 CPU)

Does this justify another dependency ?
Comment 6 Jonathan Druart 2017-10-20 14:08:42 UTC
(In reply to Marcel de Rooy from comment #5)
> Files=1, Tests=842, 42 wallclock secs ( 0.20 usr  0.02 sys + 41.23 cusr 
> 0.40 csys = 41.85 CPU)
> 
> Files=1, Tests=842, 37 wallclock secs ( 0.06 usr  0.01 sys + 42.91 cusr 
> 0.84 csys = 43.82 CPU)
> 
> Does this justify another dependency ?

The dependency is used by bug 18055 as well, and we gain 10min
Comment 7 Marcel de Rooy 2017-10-23 06:41:46 UTC
About the "use threads;" line:

From the perl doc:

The "interpreter-based threads" provided by Perl are not the fast, lightweight system for multitasking that one might expect or hope for. Threads are implemented in a way that make them easy to misuse. Few people know how to use them correctly or will be able to provide help.
The use of interpreter-based threads in perl is officially discouraged.

Do we really need it ?
Comment 8 Marcel de Rooy 2017-10-24 13:49:51 UTC
Moving to Discussion for feedback. Feel free to move back later..
Comment 9 Mason James 2017-11-02 05:26:35 UTC
(In reply to Marcel de Rooy from comment #7)
> About the "use threads;" line:
> 
> Do we really need it ?

hi Marcel, i think it's not needed :)
i must have forgotten to remove the line before sending, i'll fix that now...
Comment 10 Mason James 2017-11-15 07:12:33 UTC
a little update here...
it turns out that the 'use threads' line was needed for patch to succeed, after all

the 'use threads' line seems to handle a known bug where Test::Simple generates the TAP tests out of order, and causes the tests to fail


the good news is that there are some options...

1/ use the Test2::Suite module, which handles parallel tests well...
   https://metacpan.org/pod/Test2::Suite

   the only catch here is that it's new-ish, so we'll need to backport the module to deb8/jessie too

2/ just allow the 'use threads' line. 
	Because no threads are ever actually created, it should be pretty safe
   

fyi: i did try using forks.pm too, but sadly got lots of random fails
Comment 11 Mason James 2017-11-15 08:03:34 UTC
Created attachment 69150 [details] [review]
Bug 18979: Swap threads for Test2::Suite

this patch swaps threads for the Test2::Suite module

to test...

1/ apply patch

2/ install Test2::Suite
    $ sudo apt-get install libtest2-suite-perl , or...

    $ cpanm Test2::Suite

3/ run test

    $ sudo koha-shell -c  " export PERL5LIB=/home/mason/git/koha/master;   prove -v xt/author/valid-templates.t" kmaster
    ok 1 - Testing opac bootstrap templates {
        ...
    }
    ok 2 - Testing staff prog templates {
        ...
    }
    1..2
    ok
    All tests successful.
    Result: PASS

4. note test passes
    Result: PASS
Comment 12 Marcel de Rooy 2017-11-15 12:07:02 UTC
(In reply to Mason James from comment #10)
> a little update here...
> it turns out that the 'use threads' line was needed for patch to succeed,
> after all
> 
> the 'use threads' line seems to handle a known bug where Test::Simple
> generates the TAP tests out of order, and causes the tests to fail
> 
> 
> the good news is that there are some options...
> 
> 1/ use the Test2::Suite module, which handles parallel tests well...
>    https://metacpan.org/pod/Test2::Suite
> 
>    the only catch here is that it's new-ish, so we'll need to backport the
> module to deb8/jessie too
> 
> 2/ just allow the 'use threads' line. 
> 	Because no threads are ever actually created, it should be pretty safe
>    
> 
> fyi: i did try using forks.pm too, but sadly got lots of random fails

Mason,
Which option has your preference ?

Marcel
Comment 13 Mason James 2017-11-16 06:48:09 UTC
(In reply to Marcel de Rooy from comment #12)
> (In reply to Mason James from comment #10)
> > a little update here...
> > it turns out that the 'use threads' line was needed for patch to succeed,
> > after all
> > 
> > the 'use threads' line seems to handle a known bug where Test::Simple
> > generates the TAP tests out of order, and causes the tests to fail
> > 
> > 
> > the good news is that there are some options...
> > 
> > 1/ use the Test2::Suite module, which handles parallel tests well...
> >    https://metacpan.org/pod/Test2::Suite
> > 
> >    the only catch here is that it's new-ish, so we'll need to backport the
> > module to deb8/jessie too
> > 
> > 2/ just allow the 'use threads' line. 
> > 	Because no threads are ever actually created, it should be pretty safe
> >    
> > 
> > fyi: i did try using forks.pm too, but sadly got lots of random fails
> 
> Mason,
> Which option has your preference ?
> 
> Marcel

hiya, i'd vote for option 1/ as quick fix, and then a separate bug for a leisurely switch to Test2::Suite

anyone else have an opinion?
Comment 14 Mason James 2017-11-16 08:08:01 UTC
> hiya, i'd vote for option 1/ as quick fix, and then a separate bug for a
> leisurely switch to Test2::Suite

oops, i meant option 2, then a follow-up for option 1 :)
Comment 15 Mason James 2018-02-09 06:07:18 UTC
(In reply to Mason James from comment #14)
> > hiya, i'd vote for option 1/ as quick fix, and then a separate bug for a
> > leisurely switch to Test2::Suite
> 
> oops, i meant option 2, then a follow-up for option 1 :)

Marcel, lets go for option 2 here...

i'll obsolete the newer Test2::Suite patch, for now
..and set the status back to SIGNED-OFF
Comment 16 Katrin Fischer 2018-02-11 15:26:39 UTC
Marcel, could you have another look here?
Comment 17 Marcel de Rooy 2018-02-13 15:39:07 UTC
Created attachment 71552 [details] [review]
Bug 18979: Speed up 'valid-templates.t' tests

Test plan:
Confirm than prove xt/author/valid-templates.t is quicker with this
patch.

Before I got:
Files=1, Tests=840, 21 wallclock secs ( 0.18 usr  0.02 sys + 20.28 cusr
0.32 csys = 20.80 CPU)

After:
Files=1, Tests=840, 17 wallclock secs ( 0.06 usr  0.01 sys + 21.56 cusr
0.48 csys = 22.11 CPU)

Signed-off-by: David Bourgault <david.bourgault@inlibro.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 18 Marcel de Rooy 2018-02-13 15:42:38 UTC
(In reply to Katrin Fischer from comment #16)
> Marcel, could you have another look here?

I added a forgotten signoff line from David.
I have no objection to this patch, but also note that forking makes us unaware of the warnings raised by the tests like:
# excluding template /usr/share/koha/devclone/koha-tmpl/opac-tmpl/bootstrap/en/includes/opac-bottom.inc because it cannot stand on its own
etc.
Comment 19 Jonathan Druart 2018-04-02 21:14:46 UTC
Pushed to master for 18.05, thanks to everybody involved!