diff --git a/deliver/lib/deliver/options.rb b/deliver/lib/deliver/options.rb index 5ff0929d977..809f003ce86 100644 --- a/deliver/lib/deliver/options.rb +++ b/deliver/lib/deliver/options.rb @@ -162,6 +162,11 @@ def self.available_options description: "Clear all previously uploaded screenshots before uploading the new ones", type: Boolean, default_value: false), + FastlaneCore::ConfigItem.new(key: :screenshot_processing_timeout, + env_name: "DELIVER_SCREENSHOT_PROCESSING_TIMEOUT", + description: "Timeout in seconds to wait before considering screenshot processing as failed, used to handle cases where uploads to the App Store are stuck in processing", + type: Integer, + default_value: 3600), FastlaneCore::ConfigItem.new(key: :sync_screenshots, env_name: "DELIVER_SYNC_SCREENSHOTS", description: "Sync screenshots with local ones. This is currently beta option so set true to 'FASTLANE_ENABLE_BETA_DELIVER_SYNC_SCREENSHOTS' environment variable as well", diff --git a/deliver/lib/deliver/upload_screenshots.rb b/deliver/lib/deliver/upload_screenshots.rb index 9046ce6f152..575e6e81892 100644 --- a/deliver/lib/deliver/upload_screenshots.rb +++ b/deliver/lib/deliver/upload_screenshots.rb @@ -55,7 +55,7 @@ def upload(options, screenshots) localizations = version.get_app_store_version_localizations end - upload_screenshots(localizations, screenshots_per_language) + upload_screenshots(localizations, screenshots_per_language, options[:screenshot_processing_timeout]) Helper.show_loading_indicator("Sorting screenshots uploaded...") sort_screenshots(localizations) @@ -109,7 +109,7 @@ def delete_screenshots(localizations, screenshots_per_language, tries: 5) end end - def upload_screenshots(localizations, screenshots_per_language, tries: 5) + def upload_screenshots(localizations, screenshots_per_language, timeout_seconds, tries: 5) tries -= 1 # Upload screenshots @@ -158,15 +158,16 @@ def upload_screenshots(localizations, screenshots_per_language, tries: 5) UI.verbose('Uploading jobs are completed') Helper.show_loading_indicator("Waiting for all the screenshots to finish being processed...") - states = wait_for_complete(iterator) + states = wait_for_complete(iterator, timeout_seconds) Helper.hide_loading_indicator - retry_upload_screenshots_if_needed(iterator, states, total_number_of_screenshots, tries, localizations, screenshots_per_language) + retry_upload_screenshots_if_needed(iterator, states, total_number_of_screenshots, tries, timeout_seconds, localizations, screenshots_per_language) UI.message("Successfully uploaded all screenshots") end # Verify all screenshots have been processed - def wait_for_complete(iterator) + def wait_for_complete(iterator, timeout_seconds) + start_time = Time.now loop do states = iterator.each_app_screenshot.map { |_, _, app_screenshot| app_screenshot }.each_with_object({}) do |app_screenshot, hash| state = app_screenshot.asset_delivery_state['state'] @@ -177,16 +178,22 @@ def wait_for_complete(iterator) is_processing = states.fetch('UPLOAD_COMPLETE', 0) > 0 return states unless is_processing + if Time.now - start_time > timeout_seconds + UI.important("Screenshot upload reached the timeout limit of #{timeout_seconds} seconds. We'll now retry uploading the screenshots that couldn't be uploaded in time.") + return states + end + UI.verbose("There are still incomplete screenshots - #{states}") sleep(5) end end # Verify all screenshots states on App Store Connect are okay - def retry_upload_screenshots_if_needed(iterator, states, number_of_screenshots, tries, localizations, screenshots_per_language) + def retry_upload_screenshots_if_needed(iterator, states, number_of_screenshots, tries, timeout_seconds, localizations, screenshots_per_language) is_failure = states.fetch("FAILED", 0) > 0 + is_processing = states.fetch('UPLOAD_COMPLETE', 0) > 0 is_missing_screenshot = !screenshots_per_language.empty? && !verify_local_screenshots_are_uploaded(iterator, screenshots_per_language) - return unless is_failure || is_missing_screenshot + return unless is_failure || is_missing_screenshot || is_processing if tries.zero? iterator.each_app_screenshot.select { |_, _, app_screenshot| app_screenshot.error? }.each do |localization, _, app_screenshot| @@ -200,7 +207,7 @@ def retry_upload_screenshots_if_needed(iterator, states, number_of_screenshots, iterator.each_app_screenshot do |_, _, app_screenshot| app_screenshot.delete! unless app_screenshot.complete? end - upload_screenshots(localizations, screenshots_per_language, tries: tries) + upload_screenshots(localizations, screenshots_per_language, timeout_seconds, tries: tries) end end diff --git a/deliver/spec/upload_screenshots_spec.rb b/deliver/spec/upload_screenshots_spec.rb index bcdbfda144b..27e281d33f3 100644 --- a/deliver/spec/upload_screenshots_spec.rb +++ b/deliver/spec/upload_screenshots_spec.rb @@ -79,7 +79,7 @@ allow(described_class).to receive(:calculate_checksum).and_return('checksum') expect(app_screenshot_set).to receive(:upload_screenshot).with(path: local_screenshot.path, wait_for_processing: false) - subject.upload_screenshots([localization], screenshots_per_language) + subject.upload_screenshots([localization], screenshots_per_language, 3600) end end @@ -100,7 +100,7 @@ allow(FastlaneCore::Helper).to receive(:show_loading_indicator).and_return(true) expect(app_screenshot_set).to receive(:upload_screenshot).with(path: local_screenshot.path, wait_for_processing: false) - subject.upload_screenshots([localization], screenshots_per_language) + subject.upload_screenshots([localization], screenshots_per_language, 3600) end end end @@ -122,7 +122,7 @@ allow(FastlaneCore::Helper).to receive(:show_loading_indicator).and_return(true) expect(app_screenshot_set).to receive(:upload_screenshot).with(path: local_screenshot.path, wait_for_processing: false) - subject.upload_screenshots([localization], screenshots_per_language) + subject.upload_screenshots([localization], screenshots_per_language, 3600) end end @@ -145,7 +145,7 @@ allow(FastlaneCore::Helper).to receive(:show_loading_indicator).and_return(true) expect(app_screenshot_set).to_not(receive(:upload_screenshot)) - subject.upload_screenshots([localization], screenshots_per_language) + subject.upload_screenshots([localization], screenshots_per_language, 3600) end end @@ -166,7 +166,7 @@ allow(FastlaneCore::Helper).to receive(:show_loading_indicator).and_return(true) expect(app_screenshot_set).to receive(:upload_screenshot).exactly(10).times - subject.upload_screenshots([localization], screenshots_per_language) + subject.upload_screenshots([localization], screenshots_per_language, 3600) end end @@ -194,7 +194,7 @@ allow(FastlaneCore::Helper).to receive(:show_loading_indicator).and_return(true) expect(app_screenshot_set).to_not(receive(:upload_screenshot)) - subject.upload_screenshots([localization], screenshots_per_language) + subject.upload_screenshots([localization], screenshots_per_language, 3600) end end @@ -224,7 +224,7 @@ allow(FastlaneCore::Helper).to receive(:show_loading_indicator).and_return(true) expect(app_screenshot_set).to_not(receive(:upload_screenshot)) - subject.upload_screenshots([localization], screenshots_per_language) + subject.upload_screenshots([localization], screenshots_per_language, 3600) end end end @@ -242,8 +242,10 @@ get_app_screenshot_sets: [app_screenshot_set]) iterator = Deliver::AppScreenshotIterator.new([localization]) + allow(Time).to receive(:now).and_return(0) + expect(::Kernel).to_not(receive(:sleep)) - expect(subject.wait_for_complete(iterator)).to eq('COMPLETE' => 1) + expect(subject.wait_for_complete(iterator, 3600)).to eq('COMPLETE' => 1) end end @@ -258,9 +260,29 @@ get_app_screenshot_sets: [app_screenshot_set]) iterator = Deliver::AppScreenshotIterator.new([localization]) + allow(Time).to receive(:now).and_return(0) + expect_any_instance_of(Object).to receive(:sleep).with(kind_of(Numeric)).once expect(app_screenshot).to receive(:asset_delivery_state).and_return({ 'state' => 'UPLOAD_COMPLETE' }, { 'state' => 'COMPLETE' }) - expect(subject.wait_for_complete(iterator)).to eq('COMPLETE' => 1) + expect(subject.wait_for_complete(iterator, 3600)).to eq('COMPLETE' => 1) + end + end + + context 'when timeout is exceeded' do + it 'should exit the loop and return the current states' do + app_screenshot = double('Spaceship::ConnectAPI::AppScreenshot', asset_delivery_state: { 'state' => 'UPLOAD_COMPLETE' }) + app_screenshot_set = double('Spaceship::ConnectAPI::AppScreenshotSet', + screenshot_display_type: Spaceship::ConnectAPI::AppScreenshotSet::DisplayType::APP_IPHONE_55, + app_screenshots: [app_screenshot]) + localization = double('Spaceship::ConnectAPI::AppStoreVersionLocalization', + locale: 'en-US', + get_app_screenshot_sets: [app_screenshot_set]) + iterator = Deliver::AppScreenshotIterator.new([localization]) + states = { 'UPLOAD_COMPLETE' => 1 } + + allow(Time).to receive(:now).and_return(0, 3601) + + expect(subject.wait_for_complete(iterator, 3600)).to eq(states) end end end @@ -279,7 +301,7 @@ states = { 'FAILD' => 0, 'COMPLETE' => 1 } expect(subject).to_not(receive(:upload_screenshots)) - subject.retry_upload_screenshots_if_needed(iterator, states, 1, 1, [], {}) + subject.retry_upload_screenshots_if_needed(iterator, states, 1, 1, 0, [], {}) end end @@ -297,7 +319,7 @@ expect(subject).to receive(:upload_screenshots).with(any_args) expect(app_screenshot).to receive(:delete!) - subject.retry_upload_screenshots_if_needed(iterator, states, 1, 1, [], {}) + subject.retry_upload_screenshots_if_needed(iterator, states, 1, 1, 0, [], {}) end end @@ -314,7 +336,7 @@ states = { 'FAILED' => 1, 'COMPLETE' => 0 } expect(subject).to receive(:upload_screenshots).with(any_args) - subject.retry_upload_screenshots_if_needed(iterator, states, 999, 1, [], {}) + subject.retry_upload_screenshots_if_needed(iterator, states, 999, 1, 0, [], {}) end end @@ -336,7 +358,7 @@ expect(subject).to_not(receive(:upload_screenshots).with(any_args)) expect(UI).to receive(:user_error!) - subject.retry_upload_screenshots_if_needed(iterator, states, 1, 0, [], {}) + subject.retry_upload_screenshots_if_needed(iterator, states, 1, 0, 0, [], {}) end end @@ -357,7 +379,30 @@ expect(subject).to_not(receive(:upload_screenshots).with(any_args)) expect(UI).to_not(receive(:user_error!)) - subject.retry_upload_screenshots_if_needed(iterator, states, number_of_screenshots, 0, [], screenshots_per_language) + subject.retry_upload_screenshots_if_needed(iterator, states, number_of_screenshots, 0, 0, [], screenshots_per_language) + end + end + + context 'when screenshots are in UPLOAD_COMPLETE state' do + it 'should trigger retry logic' do + app_screenshot = double('Spaceship::ConnectAPI::AppScreenshot', + 'complete?' => false, + 'error?' => false, + file_name: '5.5_1.jpg', + error_messages: ['error_message']) + app_screenshot_set = double('Spaceship::ConnectAPI::AppScreenshotSet', + screenshot_display_type: Spaceship::ConnectAPI::AppScreenshotSet::DisplayType::APP_IPHONE_55, + app_screenshots: [app_screenshot]) + localization = double('Spaceship::ConnectAPI::AppStoreVersionLocalization', + locale: 'en-US', + get_app_screenshot_sets: [app_screenshot_set]) + localizations = [localization] + iterator = Deliver::AppScreenshotIterator.new(localizations) + states = { 'UPLOAD_COMPLETE' => 1 } + + expect(subject).to receive(:upload_screenshots).with(any_args) + expect(app_screenshot).to receive(:delete!) + subject.retry_upload_screenshots_if_needed(iterator, states, 1, 1, 0, localizations, {}) end end end