Bug 17934 - Various issues with auth_values_input_www.t
Summary: Various issues with auth_values_input_www.t
Status: In Discussion
Alias: None
Product: Koha
Classification: Unclassified
Component: Test Suite (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Mark Tompsett
QA Contact: Galen Charlton
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-01-18 15:30 UTC by Mark Tompsett
Modified: 2017-02-14 12:54 UTC (History)
2 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 17934: Make t/db_dependent/www/auth_values_input_www.t awesome (12.27 KB, patch)
2017-01-18 18:30 UTC, Mark Tompsett
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17934: Make t/db_dependent/www/auth_values_input_www.t awesome (12.33 KB, patch)
2017-02-01 20:47 UTC, Josef Moravec
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Tompsett 2017-01-18 15:30:38 UTC
Kohadevbox is the best way to duplicate this problem, as tests need to be run in a koha-shell to work, and running from the kohaclone directory owned by vagrant as kohadev means hitting them.

Bug 17920 dealt with Sitemapper which I encountered. Marcel suggested in
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17920#c6
to potentially fix:
t/db_dependent/SuggestionEngine_ExplodedTerms.t -- nope
t/db_dependent/www/auth_values_input_www.t -- Yes (unused line)
t/db_dependent/www/batch.t -- No, but some mrc test files in path might be useful
t/db_dependent/www/search_utf8.t -- Hmmm, same issue as batch.t
t/db_dependent/zebra_config.pl -- Why are __FOOBAR__ items in this file?

This means I will fix the similar problem to Sitemapper in this bug, and push the missing MRC files and confusing file to another bug, which someone else can submit if they wish.

Short version: make t/db_dependent/www/auth_values_input_www.t more awesome.
Comment 1 Mark Tompsett 2017-01-18 18:30:56 UTC
Created attachment 59173 [details] [review]
Bug 17934: Make t/db_dependent/www/auth_values_input_www.t awesome

There were two lines of code which declared unused variables.
They were removed. Next, the issue of double planning encountered
on bug 17540
(https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17540#c3)
was encountered. So this corrects that and adds a little more
error handling into the whole C4::Context using process so that
perlcritic -4 is achieved.
Also typo on Korean in a comment.

TEST PLAN
---------
2) follow these commands:

$ mv C4/Context.pm C4/Context_backup.pm
$ sudo koha-shell -c "prove t/db_dependent/www/auth_values_input_www.t" kohadev
-- Nasty fail

$ mv C4/Context_backup.pm C4/Context.pm
$ sudo koha-shell -c "export PERL5LIB=\"\";prove t/db_dependent/www/auth_values_input_www.t" kohadev
-- Nasty fail

$ sudo koha-shell -c "export KOHA_CONF=\"\";prove t/db_dependent/www/auth_values_input_www.t" kohadev
-- Nasty fail

$ sudo koha-shell -c "export KOHA_INTRANET_URL=\"\";prove t/db_dependent/www/auth_values_input_www.t" kohadev
$ sudo koha-shell -c "prove t/db_dependent/www/auth_values_input_www.t" kohadev
-- Nasty fail

$ sudo koha-shell -c "export KOHA_INTRANET_URL=\"http://localhost:8080/\";prove t/db_dependent/www/auth_values_input_www.t" kohadev
-- This should work.

$ perlcritic -4 t/db_dependent/www/auth_values_input_www.t
-- This should fail

2) apply patch
3) follow these commands:

$ mv C4/Context.pm C4/Context_backup.pm
$ sudo koha-shell -c "prove t/db_dependent/www/auth_values_input_www.t" kohadev
-- This should trigger the C4::Context not working message

$ mv C4/Context_backup.pm C4/Context.pm
$ sudo koha-shell -c "export PERL5LIB=\"\";prove t/db_dependent/www/auth_values_input_www.t" kohadev
-- This should trigger the C4::Contxt not loading message

$ sudo koha-shell -c "export KOHA_CONF=\"\";prove t/db_dependent/www/auth_values_input_www.t" kohadev
-- This should trigger the KOHA_CONF not set message

$ sudo koha-shell -c "export KOHA_INTRANET_URL=\"\";prove t/db_dependent/www/auth_values_input_www.t" kohadev
$ sudo koha-shell -c "prove t/db_dependent/www/auth_values_input_www.t" kohadev
-- These should both trigger the KOHA_INTRANET_URL not set message

$ sudo koha-shell -c "export KOHA_INTRANET_URL=\"http://localhost:8080/\";prove t/db_dependent/www/auth_values_input_www.t" kohadev
-- This should work.

$ perlcritic -4 t/db_dependent/www/auth_values_input_www.t
-- This should be OK
3) run koha qa test tools
Comment 2 Mark Tompsett 2017-01-18 18:42:57 UTC
Comment on attachment 59173 [details] [review]
Bug 17934: Make t/db_dependent/www/auth_values_input_www.t awesome

Review of attachment 59173 [details] [review]:
-----------------------------------------------------------------

After rereading the code changes, perhaps enhancement is better. You'll note that perlcritic -1 on this will be nice and short.

::: t/db_dependent/www/auth_values_input_www.t
@@ -30,2 @@
>  
> -my $testdir = File::Spec->rel2abs( dirname(__FILE__) );

not used.

@@ +35,5 @@
> +    $skip_all = "Tests skipped. You must have a working C4::Context.\n";
> +}
> +elsif ( ! can_load( modules => { 'C4::Context' => undef } ) ) {
> +    $skip_all = "Tests skipped. You must be able to load C4::Context.\n";
> +}

Added to determine cause of failure better.

@@ +45,5 @@
> +    $user     = $ENV{KOHA_USER} || $xml->{config}->{user};
> +    $password = $ENV{KOHA_PASS} || $xml->{config}->{pass};
> +}
> +elsif ($skip_all eq q{}) {
> +    $skip_all = "Tests skipped. You must set env. variable KOHA_CONF to run tests.\n";

Added check.

@@ +56,4 @@
>  }
>  
> +if (length $skip_all > 0) {
> +    croak $skip_all; # to keep Test Summary output

Any failures due to prerequisites will happen here.

@@ +63,3 @@
>  my $dbh = C4::Context->dbh;
>  
> +$intranet =~ s/\/$//xsm;

perlcritic -1 prefers /'s and xsm added.

@@ -54,3 @@
>  
>  my $agent = Test::WWW::Mechanize->new( autocheck => 1 );
> -my $jsonresponse;

Not used.

@@ +72,5 @@
>  $agent->get_ok( "$intranet/cgi-bin/koha/mainpage.pl", 'connect to intranet' );
>  $agent->form_name('loginform');
>  $agent->field( 'password', $password );
>  $agent->field( 'userid',   $user );
> +$agent->field( 'branch',   q{} );

q{} is perlcritic -1 friendly.

@@ +78,3 @@
>  $agent->get_ok( "$intranet/cgi-bin/koha/mainpage.pl", 'load main page' );
>  
> +#--------------------------------------------------- Test with Korean and greek chars

Typo!

@@ +88,4 @@
>  $agent->get_ok( "$intranet/cgi-bin/koha/admin/authorised_values.pl?op=add_form", 'Open to create a new category' );
>  $agent->form_name('Aform');
>  $agent->field('category', $category);
> +$agent->click_ok( q{}, 'Create new AV category ' );

Using single quotes for non-interpolated strings is perlcritic friendlier.

@@ +94,3 @@
>  $add_form_link_exists = 0;
>  for my $link ( $agent->links() ) {
> +    if ( $link->url =~ m/authorised_values.pl\x3Fop=add_form&category=$category/xsm ) {

perlcritic didn't like \?, so I hex'd it.

@@ +123,5 @@
>  $agent->get_ok( "$intranet/cgi-bin/koha/admin/authorised_values.pl", 'Return to Authorized values page' );
>  $agent->get_ok( "$intranet/cgi-bin/koha/admin/authorised_values.pl?searchfield=学協会μμ&offset=0", 'Search the values inserted' );
>  my $text = $agent->text() ;
>  #Tests on UTF-8
> +ok ( ( length Encode::encode('UTF-8', $text) != length $text ) , 'UTF-8 are multi-byte. Good') ;

length is a built in function, no need for brackets on it. Removed brackets for perlcritic friendliness.

@@ +173,5 @@
>          $delete_form_link_exists = 1;
>      }
>  }
> +is( $add_form_link_exists, 1, 'Add form link exists');
> +is( $delete_form_link_exists, 1, 'Delete form link exists');

Added a reasonable test description string to make perlcritic happier.
Comment 3 Josef Moravec 2017-02-01 20:47:53 UTC
Created attachment 59753 [details] [review]
[SIGNED-OFF] Bug 17934: Make t/db_dependent/www/auth_values_input_www.t awesome

There were two lines of code which declared unused variables.
They were removed. Next, the issue of double planning encountered
on bug 17540
(https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17540#c3)
was encountered. So this corrects that and adds a little more
error handling into the whole C4::Context using process so that
perlcritic -4 is achieved.
Also typo on Korean in a comment.

TEST PLAN
---------
2) follow these commands:

$ mv C4/Context.pm C4/Context_backup.pm
$ sudo koha-shell -c "prove t/db_dependent/www/auth_values_input_www.t" kohadev
-- Nasty fail

$ mv C4/Context_backup.pm C4/Context.pm
$ sudo koha-shell -c "export PERL5LIB=\"\";prove t/db_dependent/www/auth_values_input_www.t" kohadev
-- Nasty fail

$ sudo koha-shell -c "export KOHA_CONF=\"\";prove t/db_dependent/www/auth_values_input_www.t" kohadev
-- Nasty fail

$ sudo koha-shell -c "export KOHA_INTRANET_URL=\"\";prove t/db_dependent/www/auth_values_input_www.t" kohadev
$ sudo koha-shell -c "prove t/db_dependent/www/auth_values_input_www.t" kohadev
-- Nasty fail

$ sudo koha-shell -c "export KOHA_INTRANET_URL=\"http://localhost:8080/\";prove t/db_dependent/www/auth_values_input_www.t" kohadev
-- This should work.

$ perlcritic -4 t/db_dependent/www/auth_values_input_www.t
-- This should be OK

2) apply patch
3) follow these commands:

$ mv C4/Context.pm C4/Context_backup.pm
$ sudo koha-shell -c "prove t/db_dependent/www/auth_values_input_www.t" kohadev
-- This should trigger the C4::Context not working message

$ mv C4/Context_backup.pm C4/Context.pm
$ sudo koha-shell -c "export PERL5LIB=\"\";prove t/db_dependent/www/auth_values_input_www.t" kohadev
-- This should trigger the C4::Contxt not loading message

$ sudo koha-shell -c "export KOHA_CONF=\"\";prove t/db_dependent/www/auth_values_input_www.t" kohadev
-- This should trigger the KOHA_CONF not set message

$ sudo koha-shell -c "export KOHA_INTRANET_URL=\"\";prove t/db_dependent/www/auth_values_input_www.t" kohadev
$ sudo koha-shell -c "prove t/db_dependent/www/auth_values_input_www.t" kohadev
-- These should both trigger the KOHA_INTRANET_URL not set message

$ sudo koha-shell -c "export KOHA_INTRANET_URL=\"http://localhost:8080/\";prove t/db_dependent/www/auth_values_input_www.t" kohadev
-- This should work.

$ perlcritic -4 t/db_dependent/www/auth_values_input_www.t
-- This should be OK
3) run koha qa test tools

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 4 Jonathan Druart 2017-02-02 09:19:45 UTC
Comment on attachment 59753 [details] [review]
[SIGNED-OFF] Bug 17934: Make t/db_dependent/www/auth_values_input_www.t awesome

Review of attachment 59753 [details] [review]:
-----------------------------------------------------------------

::: t/db_dependent/www/auth_values_input_www.t
@@ +167,4 @@
>  $add_form_link_exists = 0;
>  $delete_form_link_exists = 0;
>  for my $link ( $agent->links() ) {
> +    if ( $link->url =~ m/authorised_values.pl\x3Fop=add_form&category=$category/xsm ) {

Really? This is terrible.
I do not see the point to follow perlcritic -1 if it's to introduce such things.
Comment 5 Jonathan Druart 2017-02-14 12:54:27 UTC
And why do you test the validity of C4::Context?