Fix #141: Add re-tries to TestRail::API and friends

Changes

@@ -3,6 +3,7 @@ Revision history for Perl module TestRail::API
 0.042 2018-04-30 TEODESIAN
     - Fix uninitialized value warning when no plan is emitted
     - Die on HTTP 401/403, we likely will never recover from this
+    - Add ability to re-try failed requests in TestRail::API, App::Prove::Plugin::TestRail and testrail-report
 
 0.041 2017-06-06 TEODESIAN
     - Fix MCE usage issue with confusion based on array -> hash inputs in TestRail::Utils::Find

bin/testrail-report

@@ -62,6 +62,8 @@ This should provide sufficient uniqueness to get any run using names.
     --section [section_name] : When spawning, restrict the cases used in the provided testsuite ID to these sections.
       Option may be passed multiple times to specify multiple sections.
 
+    --num_tries [X] : Number of times to attempt an API request.  Default 1.
+
 =head3 CONFIG OPTIONS
 
 In your $HOME (or the current directory, if your system has no concept of a home directory), put a file called .testrailrc with key=value
@@ -147,6 +149,7 @@ sub run {
         'section=s@'     => \$opts{sections},
         'autoclose'      => \$opts{autoclose},
         'e|encoding=s'   => \$opts{encoding},
+        'max_tries=i'    => \$opts{max_tries},
         'help'           => \$opts{help},
     );
 
@@ -195,6 +198,7 @@ sub run {
             'sections'       => $opts{sections},
             'autoclose'      => $opts{autoclose},
             'encoding'       => $opts{encoding},
+            'max_tries'      => $opts{max_tries},
             'merge'          => 1
         });
         $tap->run();

lib/App/Prove/Plugin/TestRail.pm

@@ -42,6 +42,7 @@ If \$HOME/.testrailrc exists, it will be parsed for any of these values in a new
     encoding=UTF-8
     configuration_group=Operating Systems
     test_bad_status=blocked
+    max_tries=3
 
 Note that passing configurations as filters for runs inside of plans are separated by colons.
 
@@ -125,6 +126,7 @@ sub load {
     $ENV{'TESTRAIL_ENCODING'}  = $params->{encoding};
     $ENV{'TESTRAIL_CGROUP'}    = $params->{'configuration_group'};
     $ENV{'TESTRAIL_TBAD'}      = $params->{'test_bad_status'};
+    $ENV{'TESTRAIL_MAX_TRIES'} = $params->{'max_tries'};
     return $class;
 }
 

lib/Test/Rail/Harness.pm

@@ -58,6 +58,7 @@ sub make_parser {
     $args->{'testsuite'}       = $ENV{'TESTRAIL_TESTSUITE'};
     $args->{'config_group'}    = $ENV{'TESTRAIL_CGROUP'};
     $args->{'test_bad_status'} = $ENV{'TESTRAIL_TBAD'};
+    $args->{'max_tries'}       = $ENV{'TESTRAIL_MAX_TRIES'};
 
     @sections = split(/:/,$ENV{'TESTRAIL_SECTIONS'}) if $ENV{'TESTRAIL_SECTIONS'};
     $args->{'sections'}  = \@sections if scalar(@sections);

lib/Test/Rail/Parser.pm

@@ -80,6 +80,8 @@ Get the TAP Parser ready to talk to TestRail, and register a bunch of callbacks
 
 =item B<test_bad_status> - STRING (optional): 'internal' name of whatever status you want to mark compile failures & no plan + no assertion tests.
 
+=item B<max_tries> - INTEGER (optional): number of times to try failing requests.  Defaults to 1 (don't re-try).
+
 =back
 
 =back
@@ -160,12 +162,14 @@ sub new {
         'result_options'        => delete $opts->{'result_options'},
         'result_custom_options' => delete $opts->{'result_custom_options'},
         'test_bad_status'       => delete $opts->{'test_bad_status'},
+        'max_tries'             => delete $opts->{'max_tries'} || 1,
     };
 
     confess("plan passed, but no run passed!") if !$tropts->{'run'} && $tropts->{'plan'};
 
     #Allow natural confessing from constructor
-    my $tr = TestRail::API->new($tropts->{'apiurl'},$tropts->{'user'},$tropts->{'pass'},$tropts->{'encoding'},$tropts->{'debug'});
+    #Force-on POST redirects for maximum compatibility
+    my $tr = TestRail::API->new($tropts->{'apiurl'},$tropts->{'user'},$tropts->{'pass'},$tropts->{'encoding'},$tropts->{'debug'},1,$tropts->{max_tries});
     $tropts->{'testrail'} = $tr;
     $tr->{'browser'} = $tropts->{'browser'} if defined($tropts->{'browser'}); #allow mocks
     $tr->{'debug'} = 0; #Always suppress in production

lib/TestRail/API.pm

@@ -18,8 +18,7 @@ It is by no means exhaustively implementing every TestRail API function.
 =head1 IMPORTANT
 
 All the methods aside from the constructor should not die, but return a false value upon failure (see exceptions below).
-When the server is not responsive, expect a -500 response, and retry accordingly.
-I recommend using the excellent L<Attempt> module for this purpose.
+When the server is not responsive, expect a -500 response, and retry accordingly by setting the num_tries parameter in the constructor.
 
 Also, all *ByName methods are vulnerable to duplicate naming issues.  Try not to use the same name for:
 
@@ -77,6 +76,8 @@ Creates new C<TestRail::API> object.
 
 =item BOOLEAN C<DO_POST_REDIRECT> (optional) - Follow redirects on POST requests (most add/edit/delete calls are POSTs).  Default false.
 
+=item INTEGER C<MAX_TRIES> (optional) - Try requests up to X number of times if they fail with anything other than 401/403.  Useful with flaky external auth, or timeout issues.  Default 1.
+
 =back
 
 Returns C<TestRail::API> object if login is successful.
@@ -89,8 +90,8 @@ Does not do above checks if debug is passed.
 =cut
 
 sub new {
-    state $check = compile(ClassName, Str, Str, Str, Optional[Maybe[Str]], Optional[Maybe[Bool]], Optional[Maybe[Bool]]);
-    my ($class,$apiurl,$user,$pass,$encoding,$debug, $do_post_redirect) = $check->(@_);
+    state $check = compile(ClassName, Str, Str, Str, Optional[Maybe[Str]], Optional[Maybe[Bool]], Optional[Maybe[Bool]], Optional[Maybe[Int]]);
+    my ($class,$apiurl,$user,$pass,$encoding,$debug, $do_post_redirect,$max_tries) = $check->(@_);
 
     die("Invalid URI passed to constructor") if !is_uri($apiurl);
     $debug //= 0;
@@ -111,7 +112,9 @@ sub new {
         browser          => new LWP::UserAgent(
             keep_alive => 10,
         ),
-        do_post_redirect => $do_post_redirect
+        do_post_redirect => $do_post_redirect,
+        max_tries        => $max_tries // 1,
+        retry_delay      => 5,
     };
 
     #Allow POST redirects
@@ -172,11 +175,24 @@ sub debug {
     return $self->{'debug'};
 }
 
+=head2 B<retry_delay>
+
+There is no getter/setter for this parameter, but it is worth mentioning.
+This is the number of seconds to wait between failed request retries when max_retries > 1.
+
+    #Do something other than the default of 5s, like spam the server mercilessly
+    $tr->{retry_delay} = 0;
+    ...
+
+=cut
+
 #Convenient JSON-HTTP fetcher
 sub _doRequest {
     state $check = compile(Object, Str, Optional[Maybe[Str]], Optional[Maybe[HashRef]]);
     my ($self,$path,$method,$data) = $check->(@_);
 
+    $self->{num_tries}++;
+
     my $req = clone $self->{'default_request'};
     $method //= 'GET';
 
@@ -193,7 +209,7 @@ sub _doRequest {
     $req->content($content);
     $req->header( "Content-Type" => "application/json; charset=".$self->{'encoding'} );
 
-    my $response = $self->{'browser'}->request($req);
+    my $response = eval { $self->{'browser'}->request($req) };
 
     #Uncomment to generate mocks
     #use Data::Dumper;
@@ -204,7 +220,19 @@ sub _doRequest {
     #print $fh "\n\n}\n\n";
     #close $fh;
 
+    if ($@) {
+        #LWP threw an ex, probably a timeout
+        if ($self->{num_tries} >= $self->{max_tries}) {
+            $self->{num_tries} = 0;
+            confess "Failed to satisfy request after $self->{num_tries} tries!";
+        }
+        cluck "WARNING: TestRail API request failed due to timeout, or other LWP fatal condition, re-trying request...\n";
+        sleep $self->{retry_delay} if $self->{retry_delay};
+        goto &_doRequest;
+    }
+
     return $response if !defined($response); #worst case
+
     if ($response->code == 403) {
         confess "ERROR 403: Access Denied: ".$response->content;
     }
@@ -213,9 +241,18 @@ sub _doRequest {
     }
 
     if ($response->code != 200) {
-        cluck "ERROR: Arguments Bad? (got code ".$response->code."): ".$response->content;
-        return -int($response->code);
+        #LWP threw an ex, probably a timeout
+        if ($self->{num_tries} >= $self->{max_tries}) {
+            $self->{num_tries} = 0;
+            cluck "ERROR: Arguments Bad? (got code ".$response->code."): ".$response->content;
+            return -int($response->code);
+        }
+        cluck "WARNING: TestRail API request failed (got code ".$response->code."), re-trying request...\n";
+        sleep $self->{retry_delay} if $self->{retry_delay};
+        goto &_doRequest;
+
     }
+    $self->{num_tries} = 0;
 
     try {
         return $coder->decode($response->content);

t/TestRail-API-mockOnly.t

@@ -6,11 +6,11 @@ use lib "$FindBin::Bin/lib";
 
 #Test things we can only mock, because the API doesn't support them.
 
-use Test::More 'tests' => 16;
+use Test::More 'tests' => 18;
 use TestRail::API;
 use Test::LWP::UserAgent::TestRailMock;
 use Scalar::Util qw{reftype};
-use Capture::Tiny qw{capture};
+use Capture::Tiny qw{capture capture_stderr};
 use Test::Fatal;
 
 my $browser = $Test::LWP::UserAgent::TestRailMock::mockObject;
@@ -66,3 +66,12 @@ $tr->{'debug'} = 0;
 
 like( exception { $tr->getUsers() } , qr/could not find pants/i, "API dies on no auth or auth backend failure");
 
+$tr = TestRail::API->new('http://bork.bork','fake','fake',undef,1,undef,2);
+$tr->{'browser'} = $browser;
+$tr->{'debug'} = 0;
+$tr->{retry_delay} = 0;
+
+my $rc;
+my $oot = capture_stderr { $rc = $tr->getUsers() };
+like ($oot,qr/re-trying request/i, "Tried twice to make the call work");
+is($rc,-500,"Right code returned when re-tries run out");

t/lib/Test/LWP/UserAgent/TestRailMock.pm

@@ -38,6 +38,25 @@ my ($VAR1,$VAR2,$VAR3,$VAR4,$VAR5);
 
 {
 
+$VAR1 = 'http://bork.bork/index.php?/api/v2/get_users';
+$VAR2 = 500;
+$VAR3 = 'Server petrified in hot grits';
+$VAR4 = bless( {
+                 'client-warning' => 'Internal response',
+                 'client-date' => 'Tue, 23 Dec 2014 20:02:08 GMT',
+                 'content-type' => 'text/plain',
+                 '::std_case' => {
+                                   'client-warning' => 'Client-Warning',
+                                   'client-date' => 'Client-Date'
+                                 }
+               }, 'HTTP::Headers' );
+$VAR5 = 'Server petrified in hot grits';
+$mockObject->map_response(qr/\Q$VAR1\E/,HTTP::Response->new($VAR2, $VAR3, $VAR4, $VAR5));
+
+}
+
+{
+
 $VAR1 = 'http://locked.out/index.php?/api/v2/get_users';
 $VAR2 = 403;
 $VAR3 = 'Stay out you red menace';
@@ -1332,7 +1351,7 @@ $VAR4 = bless( {
                  'content-type' => 'application/json; charset=utf-8',
                  'server' => 'Apache/2.4.7 (Ubuntu)'
                }, 'HTTP::Headers' );
-$VAR5 = '[{"id":15,"case_id":8,"status_id":3,"assignedto_id":null,"run_id":22,"title":"fake.test","type_id":6,"priority_id":4,"estimate":null,"estimate_forecast":null,"refs":null,"milestone_id":null,"custom_preconds":null,"custom_steps":null,"custom_expected":null},{"id":15,"case_id":8,"status_id":3,"assignedto_id":null,"run_id":22,"title":"skip.test"},{"id":15,"case_id":8,"status_id":3,"assignedto_id":1,"run_id":22,"title":"NOT SO SEARED AFTER ARR"},{"id":15,"case_id":8,"status_id":3,"assignedto_id":1,"run_id":22,"title":"skipall.test"}]';
+$VAR5 = '[{"id": 2534324, "title":"NoSuchTest.t"}, {"id":15,"case_id":8,"status_id":3,"assignedto_id":null,"run_id":22,"title":"fake.test","type_id":6,"priority_id":4,"estimate":null,"estimate_forecast":null,"refs":null,"milestone_id":null,"custom_preconds":null,"custom_steps":null,"custom_expected":null},{"id":15,"case_id":8,"status_id":3,"assignedto_id":null,"run_id":22,"title":"skip.test"},{"id":15,"case_id":8,"status_id":3,"assignedto_id":1,"run_id":22,"title":"NOT SO SEARED AFTER ARR"},{"id":15,"case_id":8,"status_id":3,"assignedto_id":1,"run_id":22,"title":"skipall.test"}]';
 $mockObject->map_response(qr/\Q$VAR1\E$/,HTTP::Response->new($VAR2, $VAR3, $VAR4, $VAR5));
 
 }

t/no_such_test.tap

@@ -0,0 +1,3 @@
+NoSuchTest.t ..
+1..1
+ok 1 yippeee

t/testrail-report.t

@@ -5,7 +5,7 @@ use FindBin;
 use lib $FindBin::Bin.'/../bin';
 require 'testrail-report';
 
-use Test::More 'tests' => 15;
+use Test::More 'tests' => 17;
 use Capture::Tiny qw{capture_merged};
 
 use lib $FindBin::Bin.'/lib';
@@ -51,6 +51,12 @@ is($matches,1,"Attempts to spawn work: testsuite name");
 is($code, 0, "Exit code OK when doing autoclose");
 like($out,qr/closing plan/i,"Run closure reported to user");
 
+#Test that the max_tries option works
+@args = (qw{--apiurl http://testrail.local --user test@fake.fake --password fake --project TestProject --run FinalRun --plan FinalPlan --config testConfig --max_tries 2 t/no_such_test.tap});
+($out,(undef,$code)) = capture_merged {TestRail::Bin::Report::run('browser' => $Test::LWP::UserAgent::TestRailMock::mockObject, 'args' => \@args)};
+is($code, 0, "Exit code OK ");
+like($out,qr/re-trying request/i,"Re-try attepmt reported to user");
+
 #Test that help works
 @args = qw{--help};
 $0 = $FindBin::Bin.'/../bin/testrail-report';