Merge lp:~kfader/mvhub/pages.t-too-slow into lp:mvhub

[Keith Fader]

See commit log.

[Dan MacNeil (omacneil)]

curious as to purpose of line #49 in diff

48 + 'mech'=>'',
49 + 'http_response'=>'',

review: Needs Information

[Dan MacNeil (omacneil)]

A few problems:

You take out all the:

  $mech->get( $$page{url}

..so the tests are all invalid, they test a mech object that doesn't contain a page. We save a lot of time by not traveling over the network to fetch a web page, but we cant test the links or the validity of the html if we don't have the page.

perl objects are references, roughly akin to pointers.

So you assign a pointer to the same thing to each mech object in the @pages array.

if a is pointer that points to address 33558 and you say a=b then b also points to address 33558

To help see these problems paste this code into main at some point.

foreach my $page_href (@total_pages){
    my $mech=$$page_href{mech};
       print "mech object is:",$$page_href{mech}, q/$mech->title() is: '/,$mech$
}
;
exit 0;

One patial solution would be to create a new() object in your add_mech_objects() sub and to call $mech->get( $$page{url} in that sub.

review: Needs Fixing

[lp:~kfader/mvhub/pages.t-too-slow]

504. By Keith Fader on 2010-10-08

Modified these files to initialize mech objects correctly. TestHelper will now grab the mech objects out of the pages hash, for consistency.

505. By Keith Fader on 2010-10-08

Fixed add_mech_objects to initialize new mech object each time. Modified TestHelper to use cached mech objects for consistency

[Dan MacNeil (omacneil)]

39 + #TestHelper::pages_get_ok( $mech, @total_pages );

On general principles, you shouldn't submit commented out code for merge request.

It adds noise, With version control you can get it back

review: Needs Fixing

[Dan MacNeil (omacneil)]

tests fail.

Before submitting merge request, all should pass

run them with:

   mv_prove -r app-mvhub/t lib-mvhub/t

app-mvhub/t/perltidy.t fails.

...and anything that uses TestHelper::pages_get_ok()

sub routine was in a library for a reason...

Need to either add caching to routine (quick little & little ugly) or change every test file that uses it.

review: Needs Fixing

[lp:~kfader/mvhub/pages.t-too-slow]

506. By Keith Fader on 2010-10-15

Reverted TestHelper.pm. There was no need to change it in the first
place. Ran perltidy on pages.t. All tests pass, speed is good.

507. By Keith Fader on 2010-10-29

Changes from two weeks ago.

508. By Keith Fader on 2010-10-29

merged trunk

[Dan MacNeil (omacneil)]

in pages.t

   sub add_mech_objects {

... is never called.

review: Needs Fixing

[Dan MacNeil (omacneil)]

8 - my $mech = shift or die 'pages_get_ok() missing param: $mech';
9 - my @pages = @_ or die 'pages_get_ok() missing param: @pages';
10 + my $mech = shift or die 'run_tests() missing param: $mech';
11 + my @pages = @_ or die 'run_tests() missing param: @pages';

Good in passing fix, closer to perfect would be to mention in commit log

[Dan MacNeil (omacneil)]

Otherwise good.

[Dan MacNeil (omacneil)]

merge fairy will remove the kruft.

review: Approve

=== modified file 'app-mvhub/t/bugs/admin_logout_bug.t'
--- app-mvhub/t/bugs/admin_logout_bug.t	2010-09-27 13:28:16 +0000
+++ app-mvhub/t/bugs/admin_logout_bug.t	2010-10-29 19:42:40 +0000
@@ -45,8 +45,8 @@
 use Test::Harness;
 
 sub run_tests {
-    my $mech  = shift or die 'pages_get_ok() missing param: $mech';
-    my @pages = @_    or die 'pages_get_ok() missing param: @pages';
+    my $mech  = shift or die 'run_tests() missing param: $mech';
+    my @pages = @_    or die 'run_tests() missing param: @pages';
 
     foreach my $page (@pages) {
         $mech->add_header( Authorization => $$page{auth} )

=== modified file 'app-mvhub/t/bugs/categorize_link_missing_when_agency_login.t'
--- app-mvhub/t/bugs/categorize_link_missing_when_agency_login.t	2010-09-27 13:28:16 +0000
+++ app-mvhub/t/bugs/categorize_link_missing_when_agency_login.t	2010-10-29 19:42:40 +0000
@@ -22,11 +22,9 @@
     my $number_of_tests = ( ( scalar @pages ) * 4 );
 
     plan tests => $number_of_tests;
-    my $mech = Test::WWW::Mechanize->new();
-    $mech->default_header( Authorization => TestHelper::create_temp_auth() );
 
-    TestHelper::pages_get_ok( $mech, @pages );
-    check_for_categorize_link( $mech, @pages );
+    TestHelper::pages_add_ok(@pages);
+    check_for_categorize_link(@pages);
 }    # main
 
 sub init_pages {
@@ -45,17 +43,17 @@
                 "Agency Account Home Page - North Shore Community Action Programs",
             'tests_to_skip' => '',
             'todo_reason'   => 'Need to fix diplay agency home page',
+            'mech'          => undef,
         },
     );
     return @pages;
 }
 
 sub check_for_categorize_link {
-    my $mech  = shift or die 'page_links_ok() missing param: $mech';
-    my @pages = @_    or die 'page_links_ok() missing param: @pages';
-
+    my @pages = @_ or die 'page_links_ok() missing param: @pages';
+    my $mech;
     foreach my $page (@pages) {
-        $mech->get( $$page{url} );
+        $mech = $$page{mech};
         ok( $mech->find_link( text => 'categorize' ),
             "Found categorize link on the page"
         );

=== modified file 'app-mvhub/t/bugs/ql_display_agency_home_interaction.t'
--- app-mvhub/t/bugs/ql_display_agency_home_interaction.t	2010-09-27 13:28:16 +0000
+++ app-mvhub/t/bugs/ql_display_agency_home_interaction.t	2010-10-29 19:42:40 +0000
@@ -22,10 +22,8 @@
     my $number_of_tests = ( ( scalar @pages ) * 3 );
 
     plan tests => $number_of_tests;
-    my $mech = Test::WWW::Mechanize->new();
-    $mech->default_header( Authorization => TestHelper::create_temp_auth() );
 
-    TestHelper::pages_get_ok( $mech, @pages );
+    TestHelper::pages_add_ok(@pages);
 
 }    # main
 
@@ -44,6 +42,7 @@
                 "Agency Account Home Page - North Shore Community Action Programs",
             'tests_to_skip' => '',
             'skip_reason'   => '',
+            'mech'          => undef
         },
 
         {   'url' =>
@@ -51,6 +50,7 @@
             'expected_content' => "You must log in to access this page",
             'tests_to_skip'    => '',
             'skip_reason'      => '',
+            'mech'             => undef
         },
 
         {   'url' =>
@@ -58,6 +58,7 @@
             'expected_content' => "Transitions",
             'tests_to_skip'    => '',
             'skip_reason'      => '',
+            'mech'             => undef
         },
 
     );

=== modified file 'app-mvhub/t/bugs/ql_works_after_admin_login.t'
--- app-mvhub/t/bugs/ql_works_after_admin_login.t	2010-09-13 00:58:44 +0000
+++ app-mvhub/t/bugs/ql_works_after_admin_login.t	2010-10-29 19:42:40 +0000
@@ -25,10 +25,8 @@
     my $number_of_tests = ( ( scalar @pages ) * 3 );
 
     plan tests => $number_of_tests;
-    my $mech = Test::WWW::Mechanize->new();
-    $mech->default_header( Authorization => TestHelper::create_temp_auth() );
 
-    TestHelper::pages_get_ok( $mech, @pages );
+    TestHelper::pages_add_ok(@pages);
 }    # main
 
 sub init_pages {
@@ -43,6 +41,7 @@
             'expected_content' => "Welcome to the Administrative Home Page",
             'tests_to_skip'    => '',
             'skip_reason'      => '',
+            'mech'             => undef,
         },
         {   'url' =>
                 "http://$host/cgi-bin/mvhub/agency.pl?rm=ql&aid=$agency_id&qlid=$quick_login_id",
@@ -50,6 +49,7 @@
                 "Agency Account Home Page - North Shore Community Action Programs",
             'tests_to_skip' => '',
             'skip_reason'   => '',
+            'mech'          => undef,
         },
 
     );

=== modified file 'app-mvhub/t/mech/pages.t'
--- app-mvhub/t/mech/pages.t	2010-09-13 00:58:44 +0000
+++ app-mvhub/t/mech/pages.t	2010-10-29 19:42:40 +0000
@@ -14,6 +14,7 @@
 
 {    #main
     my $host = TestHelper::get_test_website_name('mvh');
+    my $mech = Test::WWW::Mechanize->new();
 
     my @mvh_pages_from_both = init_pages_for_both($host);
     my @pages_for_mvh       = init_pages_for_mvh_only($host);
@@ -33,17 +34,15 @@
     );
 
     my $NUMBER_OF_TESTS_PER_PAGE = 6;
+
     my $number_of_tests
         = ( ( scalar @total_pages ) * $NUMBER_OF_TESTS_PER_PAGE );
-
     plan tests => $number_of_tests;
-    my $mech = Test::WWW::Mechanize->new();
-    $mech->default_header( Authorization => TestHelper::create_temp_auth() );
 
-    TestHelper::pages_get_ok( $mech, @total_pages );
-    pages_links_ok( $mech, @total_pages );
-    pages_lint_ok( $mech, @total_pages );
-    pages_html_tidy_ok( $mech, @total_pages );
+    TestHelper::pages_add_ok(@total_pages);
+    pages_links_ok(@total_pages);
+    pages_lint_ok(@total_pages);
+    pages_html_tidy_ok(@total_pages);
 
 }    #main
 
@@ -55,62 +54,74 @@
             'expected_content' => " Search for Social Services",
             'tests_to_skip'    => 'tidy lint link get',
             'skip_reason'      => 'dan 2008-02-06 testing ability to skip',
+            'mech'             => undef,
         },
         {   'url'              => "http://$host",
             'expected_content' => "Search for Social Services",
             'tests_to_skip'    => '',
             'skip_reason'      => '',
+            'mech'             => undef,
         },
         {   'url'              => "http://$host/html/about.shtml",
             'expected_content' => "Our Purpose",
             'tests_to_skip'    => '',
             'skip_reason'      => '',
+            'mech'             => undef,
         },
         {   'url'              => "http://$host/html/help.shtml",
             'expected_content' => "Agency Account Help",
             'tests_to_skip'    => '',
             'skip_reason'      => '',
+            'mech'             => undef,
         },
         {   'url'              => "http://$host/html/register.shtml",
             'expected_content' => "Why register your agency",
             'tests_to_skip'    => '',
             'skip_reason'      => '',
+            'mech'             => undef,
         },
         {   'url'              => "http://$host/reports",
             'expected_content' => "PDF Reports",
             'tests_to_skip'    => '',
             'skip_reason'      => '',
+            'mech'             => undef,
         },
         {   'url'              => "http://$host/reports/",
             'expected_content' => "PDF Reports",
             'tests_to_skip'    => '',
             'skip_reason'      => '',
+            'mech'             => undef,
         },
 
         {   'url'              => "http://$host/cgi-bin/mvhub/agency_form.pl",
             'expected_content' => "Password",
             'tests_to_skip'    => '',
             'skip_reason'      => '',
+            'mech'             => undef,
         },
         {   'url' => "http://$host/cgi-bin/mvhub/guide.pl?rm=browse_headings",
             'expected_content' => "Arts/Culture/Entertainment",
             'tests_to_skip'    => '',
             'skip_reason'      => '',
+            'mech'             => undef,
         },
         {   'url' => "http://$host/cgi-bin/mvhub/admin/admin_reports.pl",
             'expected_content' => "Administrative Reports",
             'tests_to_skip'    => '',
             'skip_reason'      => '',
+            'mech'             => undef,
         },
         {   'url'              => "http://$host/cgi-bin/mvhub/admin/admin.pl",
             'expected_content' => "Add a New Agency",
             'tests_to_skip'    => '',
             'skip_reason'      => '',
+            'mech'             => undef,
         },
         {   'url'              => "http://$host/cgi-bin/mvhub/logout.pl",
             'expected_content' => "You have been logged out",
             'tests_to_skip'    => '',
             'skip_reason'      => '',
+            'mech'             => undef,
         },
         {   'url' =>
                 "http://$host/cgi-bin/mvhub/agency.pl?rm=ql&aid=00&qlid=00",
@@ -118,12 +129,14 @@
                 "Invalid quick log-in ID, please log in manually",
             'tests_to_skip' => '',
             'skip_reason'   => '',
+            'mech'          => undef,
         },
         {   'url' => "http://$host/cgi-bin/mvhub/agency.pl?rm=ql&id=00",
             'expected_content' =>
                 "You used an old login URL that is no longer valid",
             'tests_to_skip' => '',
             'skip_reason'   => '',
+            'mech'          => undef,
         },
     );
     return @pages;
@@ -139,6 +152,7 @@
             'expected_content' => "You must log in to access this page",
             'tests_to_skip'    => '',
             'skip_reason'      => '',
+            'mech'             => undef,
         },
         {   'url' =>
                 "http://$host//cgi-bin/mvhub/guide.pl?rm=show_agency&agency_id=100088",
@@ -146,23 +160,27 @@
             'tests_to_skip'    => 'tidy lint',
             'skip_reason' =>
                 'dan 2008-02-06 record uses smart quotes we dont strip ',
+            'mech' => undef,
         },
         {   'url' =>
                 "http://$host/cgi-bin/mvhub/guide.pl?rm=show_agency&agency_id=101342",
             'expected_content' => "(The CSL)",
             'tests_to_skip'    => '',
             'skip_reason'      => '',
+            'mech'             => undef,
         },
         {   'url' =>
                 "http://$host/cgi-bin/mvhub/guide.pl?rm=show_program&program_id=503916",
             'expected_content' => "Community Software Lab",
             'tests_to_skip'    => '',
             'skip_reason'      => '',
+            'mech'             => undef,
         },
         {   'url'              => "http://$host/html/reports.shtml",
             'expected_content' => "formatted for printing",
             'tests_to_skip'    => 'link',
             'skip_reason' => 'dan 2008-02-06 link to pdf not implemented',
+            'mech'        => undef,
         },
         {   'url' =>
                 "http://$host/cgi-bin/mvhub/guide.pl?rm=show_program_results&category_id=804796",
@@ -170,6 +188,7 @@
                 "programs related to: Visual Arts Classes, Youth/Teen",
             'tests_to_skip' => '',
             'skip_reason'   => '',
+            'mech'          => undef,
         },
     );
 }
@@ -189,6 +208,7 @@
             'expected_content' => "You must log in to access this page",
             'tests_to_skip'    => '',
             'skip_reason'      => '',
+            'mech'             => undef,
         },
         {   'url' =>
                 "http://$host//cgi-bin/mvhub/guide.pl?rm=show_agency&agency_id=103531",
@@ -196,18 +216,21 @@
             'tests_to_skip'    => 'tidy lint',
             'skip_reason' =>
                 'dan 2008-02-06 record uses smart quotes we dont strip ',
+            'mech' => undef,
         },
         {   'url' =>
                 "http://$host/cgi-bin/mvhub/guide.pl?rm=show_agency&agency_id=103542",
             'expected_content' => "Law Center of Massachusetts",
             'tests_to_skip'    => '',
             'skip_reason'      => '',
+            'mech'             => undef,
         },
         {   'url' =>
                 "http://$host/cgi-bin/mvhub/guide.pl?rm=show_program&program_id=509515",
             'expected_content' => "(NSCAP)",
             'tests_to_skip'    => '',
             'skip_reason'      => '',
+            'mech'             => undef,
         },
         {   'url' =>
                 "http://$host/cgi-bin/mvhub/guide.pl?rm=show_program_results&category_id=801584",
@@ -215,12 +238,14 @@
                 "programs related to: Adult Basic Education",
             'tests_to_skip' => '',
             'skip_reason'   => '',
+            'mech'          => undef,
         },
         {   'url' => "http://$host/$agency_id/$quick_login_id",
             'expected_content' =>
                 "Agency Account Home Page - North Shore Community Action Programs, Inc.",
             'tests_to_skip' => '',
             'skip_reason'   => '',
+            'mech'          => undef,
         },
         {   'url' =>
                 "http://$host/cgi-bin/mvhub/agency.pl?rm=ql&aid=$agency_id&qlid=$quick_login_id",
@@ -228,18 +253,31 @@
                 "Agency Account Home Page - North Shore Community Action Programs, Inc.",
             'tests_to_skip' => '',
             'skip_reason'   => '',
+            'mech'          => undef,
         },
     );
 }
 
+sub add_mech_objects {
+    my @pages = @_ or die 'add_mech_objects() missing param: @pages';
+
+    foreach my $page (@pages) {
+        my $mech = Test::WWW::Mechanize->new();
+        $mech->default_header(
+            Authorization => TestHelper::create_temp_auth() );
+        $mech->get( $$page{url} );
+        $$page{mech} = $mech;
+    }
+    @pages;
+}
+
 sub pages_links_ok {
-    my $mech  = shift or die 'page_links_ok() missing param: $mech';
-    my @pages = @_    or die 'page_links_ok() missing param: @pages';
-
+    my @pages = @_ or die 'page_links_ok() missing param: @pages';
+    my $mech = '';
 PAGES:
     foreach my $page (@pages) {
     SKIP: {
-            $mech->get( $$page{url} );
+            $mech = $$page{mech};
             if ( $$page{tests_to_skip} =~ 'link' ) {
                 skip "$$page{url} $$page{skip_reason}", 1;
                 next PAGES;
@@ -296,12 +334,11 @@
 }
 
 sub pages_lint_ok {
-    my $mech  = shift or die 'page_lint_ok() missing param: $mech';
-    my @pages = @_    or die 'page_lint_ok() missing param: @pages';
+    my @pages = @_ or die 'page_lint_ok() missing param: @pages';
 
     foreach my $page (@pages) {
     SKIP: {
-            $mech->get( $$page{url} );
+            my $mech = $$page{mech};
             skip $$page{skip_reason}, 1
                 if $$page{tests_to_skip} =~ 'lint';
             $mech->html_lint_ok("Lint: ");
@@ -310,12 +347,11 @@
 }
 
 sub pages_html_tidy_ok {
-    my $mech  = shift or die 'pages_html_tidy_ok() missing param: $mech';
-    my @pages = @_    or die 'pages_html_tidy_ok() missing param: @pages';
+    my @pages = @_ or die 'pages_html_tidy_ok() missing param: @pages';
 
     foreach my $page (@pages) {
     SKIP: {
-            $mech->get( $$page{url} );
+            my $mech = $$page{mech};
             skip $$page{skip_reason}, 1
                 if $$page{tests_to_skip} =~ 'tidy';
             my $short_page = shorten_pagename( $$page{url} );

=== modified file 'lib-mvhub/t/lib/TestHelper.pm'
--- lib-mvhub/t/lib/TestHelper.pm	2010-10-16 00:29:43 +0000
+++ lib-mvhub/t/lib/TestHelper.pm	2010-10-29 19:42:40 +0000
@@ -189,6 +189,37 @@
     }
 }
 
+sub pages_add_ok {
+    my @pages = @_ or die 'pages_add_ok() missing param: @pages';
+    my $mech;
+
+    foreach my $page (@pages) {
+    SKIP: {
+            skip "$page->{url} $$page{skip_reason}", 3
+                if $$page{tests_to_skip} =~ 'get';
+
+            if ( not defined $$page{mech} ) {
+                $$page{mech} = Test::WWW::Mechanize->new();
+                $$page{mech}->default_header(
+                    Authorization => TestHelper::create_temp_auth() );
+            }
+
+            $mech = $$page{mech};
+            $mech->get_ok( $$page{url}, "got ok: $$page{url}" );
+            $mech->content_lacks(
+                'an error in processing your request',
+                "no error in:            
+            " . $page->{url}
+            );
+            $mech->content_contains(
+                $$page{expected_content},
+                " $$page{expected_content}            
+            found in: $page->{url}"
+            );
+        }
+    }
+}
+
 sub run_submit_forms_ok {
     my %args          = @_;
     my $prefix        = $args{prefix} or croak "missing param: \$prefix";