Merge branch 'acunskis-handle-errors-paginate' into 'master'

E2E: Add retry capability for paginated request and correctly handle errors

See merge request gitlab-org/gitlab!67305

qa/qa/resource/issue.rb

@@ -84,10 +84,13 @@ def set_issue_assignees(assignee_ids:)
       # Get issue comments
       #
       # @return [Array]
-      def comments(auto_paginate: false)
+      def comments(auto_paginate: false, attempts: 0)
         return parse_body(api_get_from(api_comments_path)) unless auto_paginate
 
-        auto_paginated_response(Runtime::API::Request.new(api_client, api_comments_path, per_page: '100').url)
+        auto_paginated_response(
+          Runtime::API::Request.new(api_client, api_comments_path, per_page: '100').url,
+          attempts: attempts
+        )
       end
     end
   end

qa/qa/resource/merge_request.rb

@@ -160,10 +160,13 @@ def fabricate_large_merge_request
       # Get MR comments
       #
       # @return [Array]
-      def comments(auto_paginate: false)
+      def comments(auto_paginate: false, attempts: 0)
         return parse_body(api_get_from(api_comments_path)) unless auto_paginate
 
-        auto_paginated_response(Runtime::API::Request.new(api_client, api_comments_path, per_page: '100').url)
+        auto_paginated_response(
+          Runtime::API::Request.new(api_client, api_comments_path, per_page: '100').url,
+          attempts: attempts
+        )
       end
 
       private

qa/qa/resource/project.rb

@@ -264,21 +264,24 @@ def import_status
 
         result = parse_body(response)
 
-        Runtime::Logger.error("Import failed: #{result[:import_error]}") if result[:import_status] == "failed"
+        if result[:import_status] == "failed"
+          Runtime::Logger.error("Import failed: #{result[:import_error]}")
+          Runtime::Logger.error("Failed relations: #{result[:failed_relations]}")
+        end
 
         result[:import_status]
       end
 
-      def commits(auto_paginate: false)
+      def commits(auto_paginate: false, attempts: 0)
         return parse_body(api_get_from(api_commits_path)) unless auto_paginate
 
-        auto_paginated_response(request_url(api_commits_path, per_page: '100'))
+        auto_paginated_response(request_url(api_commits_path, per_page: '100'), attempts: attempts)
       end
 
-      def merge_requests(auto_paginate: false)
+      def merge_requests(auto_paginate: false, attempts: 0)
         return parse_body(api_get_from(api_merge_requests_path)) unless auto_paginate
 
-        auto_paginated_response(request_url(api_merge_requests_path, per_page: '100'))
+        auto_paginated_response(request_url(api_merge_requests_path, per_page: '100'), attempts: attempts)
       end
 
       def merge_request_with_title(title)
@@ -302,10 +305,10 @@ def packages
         parse_body(response)
       end
 
-      def repository_branches(auto_paginate: false)
+      def repository_branches(auto_paginate: false, attempts: 0)
         return parse_body(api_get_from(api_repository_branches_path)) unless auto_paginate
 
-        auto_paginated_response(request_url(api_repository_branches_path, per_page: '100'))
+        auto_paginated_response(request_url(api_repository_branches_path, per_page: '100'), attempts: attempts)
       end
 
       def repository_tags
@@ -328,22 +331,22 @@ def pipeline_schedules
         parse_body(response)
       end
 
-      def issues(auto_paginate: false)
+      def issues(auto_paginate: false, attempts: 0)
         return parse_body(api_get_from(api_issues_path)) unless auto_paginate
 
-        auto_paginated_response(request_url(api_issues_path, per_page: '100'))
+        auto_paginated_response(request_url(api_issues_path, per_page: '100'), attempts: attempts)
       end
 
-      def labels(auto_paginate: false)
+      def labels(auto_paginate: false, attempts: 0)
         return parse_body(api_get_from(api_labels_path)) unless auto_paginate
 
-        auto_paginated_response(request_url(api_labels_path, per_page: '100'))
+        auto_paginated_response(request_url(api_labels_path, per_page: '100'), attempts: attempts)
       end
 
-      def milestones(auto_paginate: false)
+      def milestones(auto_paginate: false, attempts: 0)
         return parse_body(api_get_from(api_milestones_path)) unless auto_paginate
 
-        auto_paginated_response(request_url(api_milestones_path, per_page: '100'))
+        auto_paginated_response(request_url(api_milestones_path, per_page: '100'), attempts: attempts)
       end
 
       def wikis

qa/qa/runtime/api/request.rb

@@ -6,6 +6,10 @@ module API
       class Request
         API_VERSION = 'v4'
 
+        def self.masked_url(url)
+          url.sub(/private_token=.*/, "private_token=[****]")
+        end
+
         def initialize(api_client, path, **query_string)
           query_string[:private_token] ||= api_client.personal_access_token unless query_string[:oauth_access_token]
           request_path = request_path(path, **query_string)
@@ -13,7 +17,7 @@ def initialize(api_client, path, **query_string)
         end
 
         def mask_url
-          @session_address.address.sub(/private_token=.*/, "private_token=[****]")
+          QA::Runtime::API::Request.masked_url(url)
         end
 
         def url

qa/qa/specs/features/api/1_manage/import_large_github_repo_spec.rb

@@ -135,10 +135,12 @@ module QA
         imported_project # import the project
         fetch_github_objects # fetch all objects right after import has started
 
-        expect { imported_project.reload!.import_status }.to eventually_eq('finished').within(
-          duration: 3600,
-          interval: 30
-        )
+        import_status = lambda do
+          imported_project.reload!.import_status.tap do |status|
+            raise "Import of '#{imported_project.name}' failed!" if status == 'failed'
+          end
+        end
+        expect(import_status).to eventually_eq('finished').within(duration: 3600, interval: 30)
         @import_time = Time.now - start
 
         aggregate_failures do
@@ -264,7 +266,7 @@ def gl_branches
       def gl_commits
         @gl_commits ||= begin
           logger.debug("= Fetching commits =")
-          imported_project.commits(auto_paginate: true).map { |c| c[:id] }
+          imported_project.commits(auto_paginate: true, attempts: 2).map { |c| c[:id] }
         end
       end
 
@@ -294,7 +296,7 @@ def gl_milestones
       def mrs
         @mrs ||= begin
           logger.debug("= Fetching merge requests =")
-          imported_mrs = imported_project.merge_requests(auto_paginate: true)
+          imported_mrs = imported_project.merge_requests(auto_paginate: true, attempts: 2)
           logger.debug("= Transforming merge request objects for comparison =")
           imported_mrs.each_with_object({}) do |mr, hash|
             resource = Resource::MergeRequest.init do |resource|
@@ -305,7 +307,7 @@ def mrs
 
             hash[mr[:title]] = {
               body: mr[:description],
-              comments: resource.comments(auto_paginate: true)
+              comments: resource.comments(auto_paginate: true, attempts: 2)
                 # remove system notes
                 .reject { |c| c[:system] || c[:body].match?(/^(\*\*Review:\*\*)|(\*Merged by:).*/) }
                 .map { |c| sanitize(c[:body]) }
@@ -320,7 +322,7 @@ def mrs
       def gl_issues
         @gl_issues ||= begin
           logger.debug("= Fetching issues =")
-          imported_issues = imported_project.issues(auto_paginate: true)
+          imported_issues = imported_project.issues(auto_paginate: true, attempts: 2)
           logger.debug("= Transforming issue objects for comparison =")
           imported_issues.each_with_object({}) do |issue, hash|
             resource = Resource::Issue.init do |issue_resource|
@@ -331,7 +333,7 @@ def gl_issues
 
             hash[issue[:title]] = {
               body: issue[:description],
-              comments: resource.comments(auto_paginate: true).map { |c| sanitize(c[:body]) }
+              comments: resource.comments(auto_paginate: true, attempts: 2).map { |c| sanitize(c[:body]) }
             }
           end
         end

qa/qa/support/api.rb

@@ -79,16 +79,27 @@ def return_response_or_raise(error)
         error.response
       end
 
-      def auto_paginated_response(url)
+      def auto_paginated_response(url, attempts: 0)
         pages = []
-        with_paginated_response_body(url) { |response| pages << response }
+        with_paginated_response_body(url, attempts: attempts) { |response| pages << response }
 
         pages.flatten
       end
 
-      def with_paginated_response_body(url)
+      def with_paginated_response_body(url, attempts: 0)
+        not_ok_error = lambda do |resp|
+          raise "Failed to GET #{QA::Runtime::API::Request.masked_url(url)} - (#{resp.code}): `#{resp}`."
+        end
+
         loop do
-          response = get(url)
+          response = if attempts > 0
+                       Retrier.retry_on_exception(max_attempts: attempts, log: false) do
+                         get(url).tap { |resp| not_ok_error.call(resp) if resp.code != HTTP_STATUS_OK }
+                       end
+                     else
+                       get(url).tap { |resp| not_ok_error.call(resp) if resp.code != HTTP_STATUS_OK }
+                     end
+
           page, pages = response.headers.values_at(:x_page, :x_total_pages)
           api_endpoint = url.match(%r{v4/(\S+)\?})[1]
 
@@ -104,7 +115,10 @@ def with_paginated_response_body(url)
       end
 
       def pagination_links(response)
-        response.headers[:link].split(',').map do |link|
+        link = response.headers[:link]
+        return unless link
+
+        link.split(',').map do |link|
           match = link.match(/<(?<url>.*)>; rel="(?<rel>\w+)"/)
           break nil unless match
 

qa/qa/support/repeater.rb

@@ -11,7 +11,15 @@ module Repeater
       RetriesExceededError = Class.new(RepeaterConditionExceededError)
       WaitExceededError = Class.new(RepeaterConditionExceededError)
 
-      def repeat_until(max_attempts: nil, max_duration: nil, reload_page: nil, sleep_interval: 0, raise_on_failure: true, retry_on_exception: false, log: true)
+      def repeat_until(
+        max_attempts: nil,
+        max_duration: nil,
+        reload_page: nil,
+        sleep_interval: 0,
+        raise_on_failure: true,
+        retry_on_exception: false,
+        log: true
+      )
         attempts = 0
         start = Time.now
 
@@ -29,17 +37,19 @@ def repeat_until(max_attempts: nil, max_duration: nil, reload_page: nil, sleep_i
           raise unless retry_on_exception
 
           attempts += 1
-          if remaining_attempts?(attempts, max_attempts) && remaining_time?(start, max_duration)
-            sleep_and_reload_if_needed(sleep_interval, reload_page)
+          raise unless remaining_attempts?(attempts, max_attempts) && remaining_time?(start, max_duration)
 
-            retry
-          else
-            raise
-          end
+          sleep_and_reload_if_needed(sleep_interval, reload_page)
+          retry
         end
 
         if raise_on_failure
-          raise RetriesExceededError, "Retry condition not met after #{max_attempts} #{'attempt'.pluralize(max_attempts)}" unless remaining_attempts?(attempts, max_attempts)
+          unless remaining_attempts?(attempts, max_attempts)
+            raise(
+              RetriesExceededError,
+              "Retry condition not met after #{max_attempts} #{'attempt'.pluralize(max_attempts)}"
+            )
+          end
 
           raise WaitExceededError, "Wait condition not met after #{max_duration} #{'second'.pluralize(max_duration)}"
         end

qa/qa/support/retrier.rb

@@ -7,21 +7,21 @@ module Retrier
 
       module_function
 
-      def retry_on_exception(max_attempts: 3, reload_page: nil, sleep_interval: 0.5)
-        QA::Runtime::Logger.debug(
-          <<~MSG.tr("\n", ' ')
-            with retry_on_exception: max_attempts: #{max_attempts};
-            reload_page: #{reload_page};
-            sleep_interval: #{sleep_interval}
-        MSG
-        )
+      def retry_on_exception(max_attempts: 3, reload_page: nil, sleep_interval: 0.5, log: true)
+        if log
+          msg = ["with retry_on_exception: max_attempts: #{max_attempts}"]
+          msg << "reload_page: #{reload_page}" if reload_page
+          msg << "sleep_interval: #{sleep_interval}"
+          QA::Runtime::Logger.debug(msg.join('; '))
+        end
 
         result = nil
         repeat_until(
           max_attempts: max_attempts,
           reload_page: reload_page,
           sleep_interval: sleep_interval,
-          retry_on_exception: true
+          retry_on_exception: true,
+          log: log
         ) do
           result = yield
 
@@ -29,7 +29,7 @@ def retry_on_exception(max_attempts: 3, reload_page: nil, sleep_interval: 0.5)
           # We set it to `true` so that it doesn't repeat if there's no exception
           true
         end
-        QA::Runtime::Logger.debug("ended retry_on_exception")
+        QA::Runtime::Logger.debug("ended retry_on_exception") if log
 
         result
       end

qa/spec/support/retrier_spec.rb

@@ -70,8 +70,9 @@
   describe '.retry_on_exception' do
     context 'when the condition is true' do
       it 'logs max_attempts, reload_page, and sleep_interval parameters' do
-        expect { subject.retry_on_exception(max_attempts: 1, reload_page: nil, sleep_interval: 0) { true } }
-          .to output(/with retry_on_exception: max_attempts: 1; reload_page: ; sleep_interval: 0/).to_stdout_from_any_process
+        message = /with retry_on_exception: max_attempts: 1; reload_page: true; sleep_interval: 0/
+        expect { subject.retry_on_exception(max_attempts: 1, reload_page: true, sleep_interval: 0) { true } }
+          .to output(message).to_stdout_from_any_process
       end
 
       it 'logs the end' do
@@ -82,8 +83,9 @@
 
     context 'when the condition is false' do
       it 'logs the start' do
-        expect { subject.retry_on_exception(max_attempts: 1, reload_page: nil, sleep_interval: 0) { false } }
-          .to output(/with retry_on_exception: max_attempts: 1; reload_page: ; sleep_interval: 0/).to_stdout_from_any_process
+        message = /with retry_on_exception: max_attempts: 1; reload_page: true; sleep_interval: 0/
+        expect { subject.retry_on_exception(max_attempts: 1, reload_page: true, sleep_interval: 0) { false } }
+          .to output(message).to_stdout_from_any_process
       end
 
       it 'logs the end' do