Merge pull request #600 from bugsnag/tms/client-refactor

Retry creation of BitBar Appium sessions

CHANGELOG.md

@@ -1,8 +1,9 @@
-# 8.9.1 - TBD
+# 8.10.0 - 2023/10/25
 
 ## Enhancements
 
 - Add appium options in more than one place to work with older appium servers [599](https://github.com/bugsnag/maze-runner/pull/599)
+- Selectively retry creation of BitBar Appium sessions depending on the error encountered [600](https://github.com/bugsnag/maze-runner/pull/600)
 
 ## Fixes
 

Gemfile.lock

@@ -9,7 +9,7 @@ GIT
 PATH
   remote: .
   specs:
-    bugsnag-maze-runner (8.9.1)
+    bugsnag-maze-runner (8.10.0)
       appium_lib (~> 12.0.0)
       appium_lib_core (~> 5.4.0)
       bugsnag (~> 6.24)

lib/maze.rb

@@ -7,7 +7,7 @@
 # Glues the various parts of MazeRunner together that need to be accessed globally,
 # providing an alternative to the proliferation of global variables or singletons.
 module Maze
-  VERSION = '8.9.1'
+  VERSION = '8.10.0'
 
   class << self
     attr_accessor :check, :driver, :internal_hooks, :mode, :start_time, :dynamic_retry, :public_address,

lib/maze/client/appium/base_client.rb

@@ -9,6 +9,7 @@ class BaseClient
 
         def initialize
           @session_ids = []
+          @start_attempts = 0
         end
 
         def start_session
@@ -47,51 +48,38 @@ def retry_start_driver?
           raise 'Method not implemented by this class'
         end
 
-        def start_driver(config)
-          retry_failure = retry_start_driver?
-          driver = nil
-          until Maze.driver
-            begin
-
-
-              # Proc to start the driver
-              start_driver_closure = Proc.new do
-                begin
-                  config.capabilities = device_capabilities
-                  driver = Maze::Driver::Appium.new config.appium_server_url,
-                                                    config.capabilities,
-                                                    config.locator
-
-                  result = driver.start_driver
-                  if result
-                    # Log details of this session
-                    $logger.info "Created Appium session: #{driver.session_id}"
-                    @session_ids << driver.session_id
-                    udid = driver.session_capabilities['udid']
-                    $logger.info "Running on device: #{udid}" unless udid.nil?
-                  end
-                  result
-                rescue => error
-                  Bugsnag.notify error
-                  $logger.error "Session creation failed: #{error}"
-                  false
-                end
-              end
+        def attempt_start_driver(config)
+          config.capabilities = device_capabilities
+          driver = Maze::Driver::Appium.new config.appium_server_url,
+                                            config.capabilities,
+                                            config.locator
+
+          result = driver.start_driver
+          if result
+            # Log details of this session
+            $logger.info "Created Appium session: #{driver.session_id}"
+            @session_ids << driver.session_id
+            udid = driver.session_capabilities['udid']
+            $logger.info "Running on device: #{udid}" unless udid.nil?
+          end
+          driver
+        end
 
+        def start_driver(config, max_attempts = 5)
 
-              # Invoke the proc, with or without retries
-              if retry_failure
-                wait = Maze::Wait.new(interval: 10, timeout: 60)
-                success = wait.until(&start_driver_closure)
-              else
-                success = start_driver_closure.call
-              end
+          attempts = 0
 
-              unless success
-                $logger.error 'Failed to create Appium driver, exiting'
-                exit(::Maze::Api::ExitCode::SESSION_CREATION_FAILURE)
-              end
+          while attempts < max_attempts && Maze.driver.nil?
+            attempts += 1
+            start_error = nil
+            begin
+              Maze.driver = attempt_start_driver(config)
+            rescue => error
+              $logger.error "Session creation failed: #{error}"
+              start_error = error
+            end
 
+            if Maze.driver
               # Infer OS version if necessary when running locally
               if Maze.config.farm == :local && Maze.config.os_version.nil?
                 version = case Maze.config.os
@@ -103,23 +91,23 @@ def start_driver(config)
                 $logger.info "Inferred OS version to be #{version}"
                 Maze.config.os_version = version
               end
-
-              Maze.driver = driver
-            rescue ::Selenium::WebDriver::Error::UnknownError => original_exception
-              $logger.warn "Attempt to acquire #{config.device} device from farm #{config.farm} failed"
-              $logger.warn "Exception: #{original_exception.message}"
-              if config.device_list.empty?
-                $logger.error 'No further devices to try - raising original exception'
-                raise original_exception
+            else
+              interval = handle_error(start_error)
+              if interval && attempts < max_attempts
+                $logger.warn "Failed to create Appium driver, retrying in #{interval} seconds"
+                Kernel::sleep interval
               else
-                config.device = config.device_list.first
-                config.device_list = config.device_list.drop(1)
-                $logger.warn "Retrying driver initialisation using next device: #{config.device}"
+                $logger.error 'Failed to create Appium driver, exiting'
+                Kernel::exit(::Maze::Api::ExitCode::SESSION_CREATION_FAILURE)
               end
             end
           end
         end
 
+        def handle_error(error)
+          raise 'Method not implemented by this class'
+        end
+
         def start_scenario
           # Launch the app on macOS
           Maze.driver.get(Maze.config.app) if Maze.config.os == 'macos'

lib/maze/client/appium/bb_client.rb

@@ -13,8 +13,29 @@ def prepare_session
           end
         end
 
-        def retry_start_driver?
-          false
+        def handle_error(error)
+          # Retry interval depends on the error message
+          return nil if error.nil?
+
+          interval = nil
+          notify = true
+
+          if error.message.include? 'no sessionId in returned payload'
+            # This will happen naturally due to a race condition in how we access devices
+            # Do not notify, but wait long enough for most ghost sessions on BitBar to terminate.
+            interval = 60
+            notify = false
+          elsif error.message.include? 'You reached the account concurrency limit'
+            # In theory this shouldn't happen, but back off if it does
+            interval = 300
+          elsif error.message.include? 'There are no devices available'
+            interval = 120
+          else
+            # Do not retry in any other case
+          end
+
+          Bugsnag.notify error if notify
+          interval
         end
 
         def start_scenario

lib/maze/client/appium/bb_devices.rb

@@ -20,8 +20,7 @@ def get_available_device(device_or_group_names)
               $logger.trace "Got group ids #{device_group_ids} for #{device_or_group_names}"
               group_count, device = api_client.find_device_in_groups(device_group_ids)
               if device.nil?
-                # TODO: Retry rather than fail, see PLAT-7377
-                Maze::Helper.error_exit 'There are no devices available'
+                raise 'There are no devices available'
               else
                 $logger.info "#{group_count} device(s) currently available in group(s) '#{device_or_group_names}'"
               end

lib/maze/client/appium/bs_client.rb

@@ -15,8 +15,18 @@ def prepare_session
           end
         end
 
-        def retry_start_driver?
-          Maze.config.device_list.nil? || Maze.config.device_list.empty?
+        # On BrowserStack, wait 10 seconds before retrying if there is another device in the list
+        def handle_error(error)
+          Bugsnag.notify error unless error.nil?
+          if Maze.config.device_list.nil? || config.device_list.empty?
+            $logger.error 'No further devices to try'
+            nil
+          else
+            config.device = config.device_list.first
+            config.device_list = config.device_list.drop(1)
+            $logger.warn "Retrying driver initialisation using next device: #{config.device}"
+            10
+          end
         end
 
         def start_scenario

test/client/appium/bb_client_test.rb

@@ -0,0 +1,147 @@
+require 'bugsnag'
+require_relative '../../test_helper'
+require_relative '../../../lib/maze'
+require_relative '../../../lib/maze/api/exit_code'
+require_relative '../../../lib/maze/configuration'
+require_relative '../../../lib/maze/driver/appium'
+require_relative '../../../lib/maze/client/bb_api_client'
+require_relative '../../../lib/maze/client/bb_client_utils'
+require_relative '../../../lib/maze/client/appium/base_client'
+require_relative '../../../lib/maze/client/appium/bb_client'
+require_relative '../../../lib/maze/client/appium/bb_devices'
+require_relative '../../../lib/utils/deep_merge'
+
+module Maze
+  module Client
+    module Appium
+      class BitBarClientTest < Test::Unit::TestCase
+
+        def add_attempt_expectations(attempts = 1)
+          # Dashboard caps
+          dashboard_caps = {
+            'bitbar:options' => {
+              bitbar_project: 'project',
+              bitbar_testrun: 'test_run'
+            }
+          }
+          Maze::Client::BitBarClientUtils.expects(:dashboard_capabilities).times(attempts).returns dashboard_caps
+
+          # Device caps
+          device_caps = {
+            'platformName' => 'iOS'
+          }
+          Maze::Client::Appium::BitBarDevices.expects(:get_available_device).times(attempts).returns device_caps
+
+          # Driver creation
+          expected_caps = {
+            'appium' => {
+              'noReset' => true,
+              'newCommandTimeout' => 600
+            },
+            'appium:options' => {
+              'noReset' => true,
+              'newCommandTimeout' => 600
+            },
+            'bitbar:options' => {
+              'apiKey' => 'apiKey',
+              'app' => 'app',
+              'findDevice' => false,
+              'testTimeout' => 7200,
+              :bitbar_project => 'project',
+              :bitbar_testrun => 'test_run'},
+            'platformName' => 'iOS',
+            'config' => 'capabilities'
+          }
+          Maze::Driver::Appium.expects(:new).with(Maze.config.appium_server_url,
+                                                  expected_caps,
+                                                  Maze.config.locator).times(attempts).returns @mock_driver
+        end
+
+        def setup
+          BitBarApiClient.stubs(:new)
+          $logger = mock('logger')
+          @mock_driver = mock('driver')
+
+          # Setup inputs
+          Maze.driver = nil
+          Maze.config.app = 'app'
+          Maze.config.access_key = 'apiKey'
+          Maze.config.appium_server_url = 'appium server'
+          Maze.config.capabilities_option = '{"config":"capabilities"}'
+          Maze.config.locator = :id
+        end
+
+        def test_start_driver_success
+
+          add_attempt_expectations
+
+          # Logging
+          @mock_driver.expects(:start_driver).returns true
+          $logger.expects(:info).with('Created Appium session: session_id')
+
+          # Successful starting of the driver
+          @mock_driver.expects(:session_id).twice.returns 'session_id'
+          @mock_driver.expects(:session_capabilities).returns({ 'uuid' => 'uuid' })
+          Bugsnag.expects(:notify).never
+
+          client = BitBarClient.new
+          client.start_driver Maze.config
+        end
+
+        def test_start_driver_recovers
+
+          add_attempt_expectations 2
+
+          #
+          # First attempt - failure
+          #
+          message = 'no sessionId in returned payload'
+          @mock_driver.expects(:start_driver).twice.raises(message).then.returns(true)
+          $logger.expects(:error).with("Session creation failed: #{message}")
+          interval = 60
+          $logger.expects(:warn).with("Failed to create Appium driver, retrying in #{interval} seconds")
+          Kernel.expects(:sleep).with(interval)
+
+          #
+          # Second attempt - success
+          #
+          $logger.expects(:info).with('Created Appium session: session_id')
+          @mock_driver.expects(:session_id).twice.returns 'session_id'
+          @mock_driver.expects(:session_capabilities).returns({ 'uuid' => 'uuid' })
+          Bugsnag.expects(:notify).never
+
+          client = BitBarClient.new
+          client.start_driver Maze.config
+        end
+
+        def test_start_driver_failure
+
+          add_attempt_expectations 2
+          message_1 = 'You reached the account concurrency limit'
+          message_2 = 'There are no devices available'
+
+          #
+          # First attempt - failure
+          #
+          @mock_driver.expects(:start_driver).twice.raises(message_1).then.raises(message_2)
+          $logger.expects(:error).with("Session creation failed: #{message_1}")
+
+          interval = 300
+          $logger.expects(:warn).with("Failed to create Appium driver, retrying in #{interval} seconds")
+          Kernel.expects(:sleep).with(interval)
+
+          #
+          # Second attempt - also fails
+          #
+          $logger.expects(:error).with("Session creation failed: #{message_2}")
+          Kernel.expects(:exit).with(::Maze::Api::ExitCode::SESSION_CREATION_FAILURE)
+          $logger.expects(:error).with("Failed to create Appium driver, exiting")
+          Bugsnag.expects(:notify).twice
+
+          client = BitBarClient.new
+          client.start_driver Maze.config, 2
+        end
+      end
+    end
+  end
+end

test/client/bb_client_utils_test.rb

@@ -1,6 +1,6 @@
 # frozen_string_literal: true
 
-require 'test_helper'
+require_relative '../test_helper'
 require_relative '../../lib/maze'
 require_relative '../../lib/maze/client/bb_client_utils'
 require_relative '../../lib/maze/runner'