diff --git a/CHANGELOG.md b/CHANGELOG.md index 40b860ad1..78967d54b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,7 +10,7 @@ _None_ ### New Features -_None_ +- Add an opt-in `fail_on_error` mode to translation download actions so GlotPress request and response errors fail CI jobs. [#771] ### Bug Fixes diff --git a/lib/fastlane/plugin/wpmreleasetoolkit/actions/android/android_download_translations_action.rb b/lib/fastlane/plugin/wpmreleasetoolkit/actions/android/android_download_translations_action.rb index 97be798f9..35957fbb9 100644 --- a/lib/fastlane/plugin/wpmreleasetoolkit/actions/android/android_download_translations_action.rb +++ b/lib/fastlane/plugin/wpmreleasetoolkit/actions/android/android_download_translations_action.rb @@ -1,5 +1,7 @@ # frozen_string_literal: true +require_relative '../../helper/config_item_helper' + # This action is the new version of android_update_metadata (AndroidUpdateMetadataAction) and should now be used instead of that one module Fastlane @@ -19,7 +21,8 @@ def self.run(params) res_dir: res_dir, glotpress_project_url: params[:glotpress_url], glotpress_filters: params[:status_filter].map { |s| { status: s } }, - locales_map: params[:locales] + locales_map: params[:locales], + fail_on_error: params[:fail_on_error] ) # Update submodules then lint translations @@ -99,6 +102,7 @@ def self.available_options type: Boolean, default_value: false ), + FastlaneCore::ConfigItem.new(**Fastlane::Helper::ConfigItemHelper::OPT_IN_FAIL_ON_ERROR_CONFIG_ITEM_OPTIONS), ] end diff --git a/lib/fastlane/plugin/wpmreleasetoolkit/actions/common/get_prs_between_tags.rb b/lib/fastlane/plugin/wpmreleasetoolkit/actions/common/get_prs_between_tags.rb index e68104c01..c988cf5e9 100644 --- a/lib/fastlane/plugin/wpmreleasetoolkit/actions/common/get_prs_between_tags.rb +++ b/lib/fastlane/plugin/wpmreleasetoolkit/actions/common/get_prs_between_tags.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true require 'fastlane/action' +require_relative '../../helper/config_item_helper' require_relative '../../helper/github_helper' module Fastlane @@ -104,15 +105,7 @@ def self.available_options 'See https://docs.github.com/en/repositories/releasing-projects-on-github/automatically-generated-release-notes#configuration-options', optional: true, type: String), - FastlaneCore::ConfigItem.new(key: :fail_on_error, - description: 'Whether to fail the lane if the changelog cannot be computed. ' \ - 'When `false` (the default), the error message is returned as the changelog itself, ' \ - 'so that it ends up visible in the GitHub Release body. ' \ - 'Set this to `true` if the caller publishes the release only after this action succeeds, ' \ - 'and would rather stop than publish a release whose notes are an error message', - optional: true, - default_value: false, - type: Boolean), + FastlaneCore::ConfigItem.new(**Fastlane::Helper::ConfigItemHelper::OPT_IN_FAIL_ON_ERROR_CONFIG_ITEM_OPTIONS), Fastlane::Helper::GithubHelper.github_token_config_item, ] end diff --git a/lib/fastlane/plugin/wpmreleasetoolkit/actions/common/gp_downloadmetadata_action.rb b/lib/fastlane/plugin/wpmreleasetoolkit/actions/common/gp_downloadmetadata_action.rb index ebadd8b41..a4cc4500f 100644 --- a/lib/fastlane/plugin/wpmreleasetoolkit/actions/common/gp_downloadmetadata_action.rb +++ b/lib/fastlane/plugin/wpmreleasetoolkit/actions/common/gp_downloadmetadata_action.rb @@ -2,6 +2,7 @@ require 'fastlane/action' require_relative '../../helper/metadata_download_helper' +require_relative '../../helper/config_item_helper' module Fastlane module Actions @@ -13,12 +14,13 @@ def self.run(params) UI.message "Source locale: #{params[:source_locale].nil? ? '-' : params[:source_locale]}" UI.message "Path: #{params[:download_path]}" UI.message "Auto-retry: #{params[:auto_retry]}" + UI.message "Fail on error: #{params[:fail_on_error]}" # Check download path FileUtils.mkdir_p(params[:download_path]) # Download - downloader = Fastlane::Helper::MetadataDownloader.new(params[:download_path], params[:target_files], params[:auto_retry]) + downloader = Fastlane::Helper::MetadataDownloader.new(params[:download_path], params[:target_files], params[:auto_retry], fail_on_error: params[:fail_on_error]) params[:locales].each do |loc| if loc.is_a?(Array) @@ -77,6 +79,7 @@ def self.available_options type: FastlaneCore::Boolean, optional: true, default_value: true), + FastlaneCore::ConfigItem.new(**Fastlane::Helper::ConfigItemHelper::OPT_IN_FAIL_ON_ERROR_CONFIG_ITEM_OPTIONS), ] end diff --git a/lib/fastlane/plugin/wpmreleasetoolkit/actions/ios/ios_download_strings_files_from_glotpress.rb b/lib/fastlane/plugin/wpmreleasetoolkit/actions/ios/ios_download_strings_files_from_glotpress.rb index ccbca6cdd..deef7e604 100644 --- a/lib/fastlane/plugin/wpmreleasetoolkit/actions/ios/ios_download_strings_files_from_glotpress.rb +++ b/lib/fastlane/plugin/wpmreleasetoolkit/actions/ios/ios_download_strings_files_from_glotpress.rb @@ -1,5 +1,8 @@ # frozen_string_literal: true +require 'tempfile' +require_relative '../../helper/config_item_helper' + module Fastlane module Actions class IosDownloadStringsFilesFromGlotpressAction < Action @@ -17,33 +20,91 @@ def self.run(params) destination = File.join(lproj_dir, "#{params[:table_basename]}.strings") FileUtils.mkdir_p(lproj_dir) - Fastlane::Helper::Ios::L10nHelper.download_glotpress_export_file( + download_and_replace_strings_file( project_url: params[:project_url], locale: glotpress_locale, filters: params[:filters], - destination: destination + destination: destination, + table_basename: params[:table_basename], + skip_file_validation: params[:skip_file_validation], + fail_on_error: params[:fail_on_error] ) - # Do a quick check of the downloaded `.strings` file to ensure it looks valid - validate_strings_file(destination) unless params[:skip_file_validation] end end - # Validate that a `.strings` file downloaded from GlotPress seems valid and does not contain empty translations - def self.validate_strings_file(destination) - return unless File.exist?(destination) # If the file failed to download, don't try to validate an non-existing file. We'd already have a separate error for the download failure anyway. + def self.download_and_replace_strings_file(project_url:, locale:, filters:, destination:, table_basename:, skip_file_validation:, fail_on_error:) + if File.exist?(destination) && !File.file?(destination) + report_error("The destination `#{destination}` exists but is not a regular file", fail_on_error: fail_on_error) + return false + end - translations = Fastlane::Helper::Ios::L10nHelper.read_strings_file_as_hash(path: destination) - empty_keys = translations.select { |_, value| value.nil? || value.empty? }.keys.sort - unless empty_keys.empty? - UI.error( - "Found empty translations in `#{destination}` for the following keys: #{empty_keys.inspect}.\n" \ - + "This is likely a GlotPress bug, and will lead to copies replaced by empty text in the UI.\n" \ - + 'Please report this to the GlotPress team, and fix the file locally before continuing.' + destination_mode = File.exist?(destination) ? File.stat(destination).mode & 0o7777 : 0o644 + Tempfile.create([table_basename, '.strings'], File.dirname(destination)) do |temporary_file| + downloaded = Fastlane::Helper::Ios::L10nHelper.download_glotpress_export_file( + project_url: project_url, + locale: locale, + filters: filters, + destination: temporary_file, + fail_on_error: fail_on_error ) + return false unless downloaded + + temporary_file.flush + # Do a quick check of the downloaded `.strings` file to ensure it looks valid + validate_strings_file(temporary_file.path, display_path: destination, fail_on_error: fail_on_error) unless skip_file_validation + File.chmod(destination_mode, temporary_file.path) + temporary_file.close + File.rename(temporary_file.path, destination) end + true + rescue FastlaneCore::Interface::FastlaneError + raise rescue StandardError => e - UI.error("Error while validating the file exported from GlotPress (`#{destination}`) - #{e.message.chomp}") + report_error("Error writing downloaded locale `#{locale}` — #{e.message} (#{destination})", fail_on_error: fail_on_error) + false + end + private_class_method :download_and_replace_strings_file + + # Validate that a `.strings` file downloaded from GlotPress seems valid and does not contain empty translations + def self.validate_strings_file(path, display_path: path, fail_on_error: false) + unless File.exist?(path) + report_error("The file exported from GlotPress was not created (`#{display_path}`)", fail_on_error: fail_on_error) if fail_on_error + return + end + + if File.empty?(path) + report_error("The file exported from GlotPress is empty (`#{display_path}`)", fail_on_error: fail_on_error) if fail_on_error + return + end + + translations = nil + begin + translations = Fastlane::Helper::Ios::L10nHelper.read_strings_file_as_hash(path: path) + rescue StandardError => e + report_error("Error while validating the file exported from GlotPress (`#{display_path}`) - #{e.message.chomp}", fail_on_error: fail_on_error) + return + end + + unless translations.is_a?(Hash) && translations.all? { |key, value| key.is_a?(String) && value.is_a?(String) } + report_error("The file exported from GlotPress is not a string-to-string dictionary (`#{display_path}`)", fail_on_error: fail_on_error) + return + end + + empty_keys = translations.select { |_, value| value.nil? || value.empty? }.keys.sort + return if empty_keys.empty? + + report_error( + "Found empty translations in `#{display_path}` for the following keys: #{empty_keys.inspect}.\n" \ + + "This is likely a GlotPress bug, and will lead to copies replaced by empty text in the UI.\n" \ + + 'Please report this to the GlotPress team, and fix the file locally before continuing.', + fail_on_error: fail_on_error + ) + end + + def self.report_error(message, fail_on_error:) + fail_on_error ? UI.user_error!(message) : UI.error(message) end + private_class_method :report_error ##################################################### # @!group Documentation @@ -92,6 +153,7 @@ def self.available_options type: Fastlane::Boolean, optional: true, default_value: false), + FastlaneCore::ConfigItem.new(**Fastlane::Helper::ConfigItemHelper::OPT_IN_FAIL_ON_ERROR_CONFIG_ITEM_OPTIONS), ] end diff --git a/lib/fastlane/plugin/wpmreleasetoolkit/helper/android/android_localize_helper.rb b/lib/fastlane/plugin/wpmreleasetoolkit/helper/android/android_localize_helper.rb index 05fd4f3f7..b2c615c35 100644 --- a/lib/fastlane/plugin/wpmreleasetoolkit/helper/android/android_localize_helper.rb +++ b/lib/fastlane/plugin/wpmreleasetoolkit/helper/android/android_localize_helper.rb @@ -243,9 +243,11 @@ def self.create_available_languages_file(res_dir:, locale_codes:) # @param [ArrayString}>] locales_map # An array of locales to download. Each item in the array must be a Hash # with keys `:glotpress` and `:android` containing the respective locale codes. + # @param [Boolean] fail_on_error Whether to fail on request errors or invalid downloaded XML. # - def self.download_from_glotpress(res_dir:, glotpress_project_url:, locales_map:, glotpress_filters: { status: 'current' }) + def self.download_from_glotpress(res_dir:, glotpress_project_url:, locales_map:, glotpress_filters: { status: 'current' }, fail_on_error: false) glotpress_filters = [glotpress_filters] unless glotpress_filters.is_a?(Array) + UI.user_error!('At least one GlotPress filter is required.') if fail_on_error && glotpress_filters.empty? orig_file = File.join(res_dir, 'values', 'strings.xml') orig_xml = File.open(orig_file) { |f| Nokogiri::XML(f, nil, Encoding::UTF_8.to_s) } @@ -253,7 +255,7 @@ def self.download_from_glotpress(res_dir:, glotpress_project_url:, locales_map:, locales_map.each do |lang_codes| all_xml_documents = glotpress_filters.map do |filters| UI.message "Downloading translations for '#{lang_codes[:android]}' from GlotPress (#{lang_codes[:glotpress]}) [#{filters}]..." - download_glotpress_export_file(project_url: glotpress_project_url, locale: lang_codes[:glotpress], filters: filters) + download_glotpress_export_file(project_url: glotpress_project_url, locale: lang_codes[:glotpress], filters: filters, fail_on_error: fail_on_error) end.compact next if all_xml_documents.empty? @@ -282,19 +284,35 @@ def self.download_from_glotpress(res_dir:, glotpress_project_url:, locales_map:, # @param [String] locale The GlotPress locale code to download strings for. # @param [Hash{Symbol=>String}] filters The hash of filters to apply when exporting from GlotPress. # Typical examples include `{ status: 'current' }` or `{ status: 'review' }`. + # @param [Boolean] fail_on_error Whether to fail on request errors or invalid downloaded XML. # @return [Nokogiri::XML::Document] the download XML document, parsed as a Nokogiri::XML object # - def self.download_glotpress_export_file(project_url:, locale:, filters:) + def self.download_glotpress_export_file(project_url:, locale:, filters:, fail_on_error:) query_params = filters.transform_keys { |k| "filters[#{k}]" }.merge(format: 'android') url = "#{project_url.chomp('/')}/#{locale}/default/export-translations/?#{URI.encode_www_form(query_params)}" Fastlane::Helper::GlotPressDownloader.download( url: url, locale: locale, - auto_retry: true + auto_retry: true, + fail_on_error: fail_on_error ) do |response_body| # Replace tabs with spaces (GlotPress uses tabs, but we prefer spaces) - Nokogiri::XML(response_body.gsub("\t", ' '), nil, Encoding::UTF_8.to_s) + if fail_on_error + begin + xml = Nokogiri::XML(response_body.gsub("\t", ' '), nil, Encoding::UTF_8.to_s, &:strict) + rescue Nokogiri::XML::SyntaxError => e + UI.user_error!("Invalid Android translation export for locale `#{locale}` — #{e.message} (#{url})") + end + + unless xml.root&.name == 'resources' + UI.user_error!("Invalid Android translation export for locale `#{locale}` — expected a `resources` root element (#{url})") + end + + xml + else + Nokogiri::XML(response_body.gsub("\t", ' '), nil, Encoding::UTF_8.to_s) + end end end private_class_method :download_glotpress_export_file diff --git a/lib/fastlane/plugin/wpmreleasetoolkit/helper/config_item_helper.rb b/lib/fastlane/plugin/wpmreleasetoolkit/helper/config_item_helper.rb new file mode 100644 index 000000000..6fe8392fa --- /dev/null +++ b/lib/fastlane/plugin/wpmreleasetoolkit/helper/config_item_helper.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +module Fastlane + module Helper + class ConfigItemHelper + OPT_IN_FAIL_ON_ERROR_CONFIG_ITEM_OPTIONS = { + key: :fail_on_error, + description: 'Whether handled errors should fail the lane instead of using the action-specific fallback behavior', + type: FastlaneCore::Boolean, + optional: true, + default_value: false + }.freeze + end + end +end diff --git a/lib/fastlane/plugin/wpmreleasetoolkit/helper/glotpress_downloader.rb b/lib/fastlane/plugin/wpmreleasetoolkit/helper/glotpress_downloader.rb index c7b09c118..27ffbc543 100644 --- a/lib/fastlane/plugin/wpmreleasetoolkit/helper/glotpress_downloader.rb +++ b/lib/fastlane/plugin/wpmreleasetoolkit/helper/glotpress_downloader.rb @@ -9,19 +9,22 @@ module Helper class GlotPressDownloader AUTO_RETRY_SLEEP_TIME = 20 MAX_AUTO_RETRY_ATTEMPTS = 30 + MAX_REDIRECTS = 10 - attr_reader :auto_retry, :auto_retry_attempt_counter, :url, :locale + attr_reader :auto_retry, :auto_retry_attempt_counter, :url, :locale, :fail_on_error # Initialize a new GlotPressDownloader # # @param [String] url The URL to download from # @param [String] locale The locale being downloaded (for logging purposes) # @param [Boolean] auto_retry Whether to automatically retry on rate limiting (429 errors) + # @param [Boolean] fail_on_error Whether to raise a Fastlane error after retry handling instead of returning false # - def initialize(url:, locale:, auto_retry: false) + def initialize(url:, locale:, auto_retry: false, fail_on_error: false) @url = url @locale = locale @auto_retry = auto_retry + @fail_on_error = fail_on_error @auto_retry_attempt_counter = 0 end @@ -30,34 +33,55 @@ def initialize(url:, locale:, auto_retry: false) # @param [String] url The URL to download from # @param [String] locale The locale being downloaded (for logging purposes) # @param [Boolean] auto_retry Whether to automatically retry on rate limiting (429 errors) - # @yield [String] The response body if the download was successful - # @return The result of the block if provided, or true/false indicating success if no block provided + # @param [Boolean] fail_on_error Whether to raise a Fastlane error after retry handling instead of returning false + # @yield [String] The response body if the download was successful. Return `false` to reject it and suppress the success message. + # @return The result of the block if provided, or true/false indicating success if no block is provided + # @raise [FastlaneCore::Interface::FastlaneError] If the download fails after retry handling and `fail_on_error` is true # - # - def self.download(url:, locale:, auto_retry: false, &) - new(url: url, locale: locale, auto_retry: auto_retry).download(&) + def self.download(url:, locale:, auto_retry: false, fail_on_error: false, &) + new(url: url, locale: locale, auto_retry: auto_retry, fail_on_error: fail_on_error).download(&) end # Downloads data from GlotPress # - # @yield [String] The response body if the download was successful - # @return The result of the block if provided, or true/false indicating success if no block provided + # @yield [String] The response body if the download was successful. Return `false` to reject it and suppress the success message. + # @return The result of the block if provided, or true/false indicating success if no block is provided + # @raise [FastlaneCore::Interface::FastlaneError] If the download fails after retry handling and `fail_on_error` is true # def download(&) @auto_retry_attempt_counter = 0 # Reset counter only at start of download - download_from_url(@url, &) + download_from_url(@url, redirect_count: 0, &) end private - def download_from_url(url, &) - uri = URI(url) + def download_from_url(url, redirect_count:, &) + uri = parse_uri(url) + return block_given? ? nil : false if uri.nil? + response = make_request(uri) + return block_given? ? nil : false if response.nil? + + unless block_given? + return handle_response(response: response, url: url, original_uri: uri, redirect_count: redirect_count) + end + result = nil - success = handle_response(response: response, url: url, original_uri: uri) do |body| - result = yield body if block_given? + handle_response(response: response, url: url, original_uri: uri, redirect_count: redirect_count) do |body| + result = yield(body) end - block_given? ? result : success + result + end + + def parse_uri(url) + uri = URI(url) + return uri if uri.is_a?(URI::HTTP) && uri.host + + handle_failure("Invalid URL for locale `#{@locale}` (#{url})") + nil + rescue URI::InvalidURIError, TypeError => e + handle_failure("Invalid URL for locale `#{@locale}` — #{e.message} (#{url})") + nil end def make_request(uri) @@ -68,60 +92,70 @@ def make_request(uri) http.request(request) rescue StandardError => e # Network errors, connection errors, etc. - UI.error("Error downloading locale `#{@locale}` — #{e.message} (#{uri})") + message = "Error downloading locale `#{@locale}` — #{e.message} (#{uri})" retry if UI.interactive? && UI.confirm("Retry downloading `#{@locale}`?") + handle_failure(message) nil end - def handle_response(response:, url:, original_uri:) - return false if response.nil? - + def handle_response(response:, url:, original_uri:, redirect_count:, &) case response.code when '200' + accepted = block_given? ? yield(response.body) : true + return false if accepted == false + UI.success("Successfully downloaded `#{@locale}`.") - yield response.body if block_given? true when '301', '302', '307', '308' # Follow the redirect UI.message("Received #{response.code} for `#{@locale}`. Following redirect...") redirect_url = response['location'] - if redirect_url.nil? - UI.error("Received #{response.code} but no location header found.") - false - else - # Follow redirect with the new URL - download_from_url(redirect_url) { |body| yield body if block_given? } - end + return handle_failure("Received #{response.code} for `#{@locale}` but no location header was found (#{original_uri}).") if redirect_url.nil? || redirect_url.empty? + return handle_failure("Too many redirects while downloading locale `#{@locale}` (#{original_uri}).") if redirect_count >= MAX_REDIRECTS + + resolved_url = resolve_redirect_url(original_uri, redirect_url) + return false if resolved_url.nil? + + download_from_url(resolved_url, redirect_count: redirect_count + 1, &) when '429' # Rate limited - handle_rate_limiting(url: url) do |body| - yield body if block_given? - end + handle_rate_limiting(url: url, response: response, redirect_count: redirect_count, &) else # Unexpected status code (including 404, 500, etc.) - status_line = "#{response.code} #{response.message}" - UI.error("Error downloading locale `#{@locale}` — #{status_line} (#{original_uri})") - if UI.interactive? && UI.confirm("Retry downloading `#{@locale}`?") - download_from_url(url) { |body| yield body if block_given? } - else - false - end + status_line = [response.code, response.message].compact.join(' ').strip + message = "Error downloading locale `#{@locale}` — #{status_line} (#{original_uri})" + return handle_failure(message) unless UI.interactive? && UI.confirm("Retry downloading `#{@locale}`?") + + download_from_url(url, redirect_count: redirect_count, &) end end - def handle_rate_limiting(url:) + def handle_rate_limiting(url:, response:, redirect_count:, &) if @auto_retry && @auto_retry_attempt_counter < MAX_AUTO_RETRY_ATTEMPTS UI.message("Received 429 for `#{@locale}`. Auto retrying in #{AUTO_RETRY_SLEEP_TIME} seconds... (attempt #{@auto_retry_attempt_counter + 1}/#{MAX_AUTO_RETRY_ATTEMPTS})") sleep(AUTO_RETRY_SLEEP_TIME) @auto_retry_attempt_counter += 1 - download_from_url(url) { |body| yield body if block_given? } + download_from_url(url, redirect_count: redirect_count, &) elsif UI.interactive? && UI.confirm("Retry downloading `#{@locale}` after receiving 429 from the API?") - download_from_url(url) { |body| yield body if block_given? } + download_from_url(url, redirect_count: redirect_count, &) else - UI.error("Abandoning `#{@locale}` download.") - false + status_line = [response.code, response.message].compact.join(' ').strip + handle_failure("Error downloading locale `#{@locale}` — #{status_line} (#{url})") end end + + def resolve_redirect_url(original_uri, redirect_url) + URI.join(original_uri.to_s, redirect_url).to_s + rescue URI::InvalidURIError => e + handle_failure("Invalid redirect URL for locale `#{@locale}` — #{e.message} (#{redirect_url})") + nil + end + + def handle_failure(message) + UI.error(message) + UI.user_error!(message) if @fail_on_error + false + end end end end diff --git a/lib/fastlane/plugin/wpmreleasetoolkit/helper/ios/ios_l10n_helper.rb b/lib/fastlane/plugin/wpmreleasetoolkit/helper/ios/ios_l10n_helper.rb index 5f6a63792..d6aeaab0d 100644 --- a/lib/fastlane/plugin/wpmreleasetoolkit/helper/ios/ios_l10n_helper.rb +++ b/lib/fastlane/plugin/wpmreleasetoolkit/helper/ios/ios_l10n_helper.rb @@ -191,26 +191,27 @@ def self.generate_strings_file_from_hash(translations:, output_path:) # @param [Hash{Symbol=>String}] filters The hash of filters to apply when exporting from GlotPress. # Typical examples include `{ status: 'current' }` or `{ status: 'review' }`. # @param [String, IO] destination The path or `IO`-like instance, where to write the downloaded file on disk. + # @param [Boolean] fail_on_error Whether to fail on request errors. # - def self.download_glotpress_export_file(project_url:, locale:, filters:, destination:) + def self.download_glotpress_export_file(project_url:, locale:, filters:, destination:, fail_on_error: false) query_params = (filters || {}).transform_keys { |k| "filters[#{k}]" }.merge(format: 'strings') url = "#{project_url.chomp('/')}/#{locale}/default/export-translations/?#{URI.encode_www_form(query_params)}" - begin - Fastlane::Helper::GlotPressDownloader.download( - url: url, - locale: locale, - auto_retry: true - ) do |response_body| - if destination.is_a?(String) - File.write(destination, response_body) - else - destination.write(response_body) - end + Fastlane::Helper::GlotPressDownloader.download( + url: url, + locale: locale, + auto_retry: true, + fail_on_error: fail_on_error + ) do |response_body| + if destination.is_a?(String) + File.write(destination, response_body) + else + destination.write(response_body) end rescue StandardError => e - UI.error "Error downloading locale `#{locale}` — #{e.message} (#{url})" - nil + message = "Error writing downloaded locale `#{locale}` — #{e.message} (#{url})" + fail_on_error ? UI.user_error!(message) : UI.error(message) + false end end end diff --git a/lib/fastlane/plugin/wpmreleasetoolkit/helper/metadata_download_helper.rb b/lib/fastlane/plugin/wpmreleasetoolkit/helper/metadata_download_helper.rb index f588cee3f..8a7b595e8 100644 --- a/lib/fastlane/plugin/wpmreleasetoolkit/helper/metadata_download_helper.rb +++ b/lib/fastlane/plugin/wpmreleasetoolkit/helper/metadata_download_helper.rb @@ -9,10 +9,11 @@ module Helper class MetadataDownloader attr_reader :target_folder, :target_files - def initialize(target_folder, target_files, auto_retry) + def initialize(target_folder, target_files, auto_retry, fail_on_error: false) @target_folder = target_folder @target_files = target_files @auto_retry = auto_retry + @fail_on_error = fail_on_error @alternates = {} end @@ -21,9 +22,10 @@ def download(target_locale, glotpress_url, is_source) GlotPressDownloader.download( url: glotpress_url, locale: target_locale, - auto_retry: @auto_retry + auto_retry: @auto_retry, + fail_on_error: @fail_on_error ) do |response_body| - handle_glotpress_response(response_body: response_body, locale: target_locale, is_source: is_source) + handle_glotpress_response(response_body: response_body, locale: target_locale, is_source: is_source, url: glotpress_url) end end @@ -106,17 +108,34 @@ def get_target_file_path(locale, file_name) private - def handle_glotpress_response(response_body:, locale:, is_source:) + def handle_glotpress_response(response_body:, locale:, is_source:, url:) # Parse the JSON response @alternates.clear - loc_data = begin - JSON.parse(response_body) - rescue StandardError - nil - end + loc_data = parse_metadata_response(response_body: response_body, locale: locale, url: url) + parse_data(locale, loc_data, is_source) reparse_alternates(locale, loc_data, is_source) unless @alternates.empty? end + + def parse_metadata_response(response_body:, locale:, url:) + return JSON.parse(response_body) unless @fail_on_error + + loc_data = JSON.parse(response_body) + UI.user_error!("Unexpected GlotPress metadata response for locale `#{locale}` (#{url})") unless valid_metadata_response?(loc_data) + loc_data + rescue JSON::ParserError, TypeError => e + UI.user_error!("Error parsing GlotPress response for locale `#{locale}` — #{e.message} (#{url})") if @fail_on_error + nil + end + + def valid_metadata_response?(loc_data) + return loc_data.empty? if loc_data.is_a?(Array) + return false unless loc_data.is_a?(Hash) && !loc_data.empty? + + loc_data.all? do |source, translations| + source.is_a?(String) && translations.is_a?(Array) && translations.all?(String) + end + end end end end diff --git a/spec/android_download_translations_action_spec.rb b/spec/android_download_translations_action_spec.rb new file mode 100644 index 000000000..b0604cd74 --- /dev/null +++ b/spec/android_download_translations_action_spec.rb @@ -0,0 +1,28 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Fastlane::Actions::AndroidDownloadTranslationsAction do + it 'passes fail_on_error to the download helper' do + allow(Fastlane::Helper::Android::LocalizeHelper).to receive(:create_available_languages_file) + expect(Fastlane::Helper::Android::LocalizeHelper).to receive(:download_from_glotpress).with(hash_including(fail_on_error: true)) + + run_described_fastlane_action( + res_dir: 'res', + glotpress_url: 'https://translate.example/projects/test/', + status_filter: ['current'], + source_locale: 'en_US', + locales: [{ glotpress: 'fr', android: 'fr' }], + lint_task: nil, + skip_commit: true, + fail_on_error: true + ) + end + + it 'keeps fail_on_error disabled by default' do + option = described_class.available_options.find { |item| item.key == :fail_on_error } + + expect(option.default_value).to be(false) + expect(option.env_name).to be_nil + end +end diff --git a/spec/android_localize_helper_spec.rb b/spec/android_localize_helper_spec.rb index 8add98f9c..4e249d4c1 100644 --- a/spec/android_localize_helper_spec.rb +++ b/spec/android_localize_helper_spec.rb @@ -295,7 +295,7 @@ def generated_file(code) "#{gp_fake_url.chomp('/')}/#{locale[:glotpress]}/default/export-translations/?filters%5Bstatus%5D=custom-status&filters%5Bwarnings%5D=yes&format=android" end custom_gp_urls.each do |url| - stub_request(:get, url) + stub_request(:get, url).to_return(status: 200, body: '') end # Act @@ -340,6 +340,63 @@ def generated_file(code) expect(File.exist?(generated_file_path)).to be(true) expect(File.read(generated_file_path)).to eq(expected_merged_content) end + + it 'raises instead of writing a partial export when one filter download fails' do + FileUtils.mkdir_p(File.dirname(generated_file(nil))) + FileUtils.cp(expected_file(nil), generated_file(nil)) + + stub_path = File.join(fixtures_dir, 'filters', 'current.xml') + stub_request(:get, "#{gp_fake_url.chomp('/')}/fakegploc/default/export-translations/?filters%5Bstatus%5D=current&format=android") + .to_return(status: 200, body: File.read(stub_path)) + stub_request(:get, "#{gp_fake_url.chomp('/')}/fakegploc/default/export-translations/?filters%5Bstatus%5D=waiting&format=android") + .to_return(status: 500) + allow(FastlaneCore::UI).to receive(:interactive?).and_return(false) + + expect do + described_class.download_from_glotpress( + res_dir: tmpdir, + glotpress_project_url: gp_fake_url, + glotpress_filters: [{ status: 'current' }, { status: 'waiting' }], + locales_map: [{ glotpress: 'fakegploc', android: 'fakeanloc' }], + fail_on_error: true + ) + end.to raise_error(FastlaneCore::Interface::FastlaneError, /500/) + + expect(File).not_to exist(generated_file('fakeanloc')) + end + + it 'raises when no export filters are provided' do + expect do + described_class.download_from_glotpress(res_dir: tmpdir, glotpress_project_url: gp_fake_url, glotpress_filters: [], locales_map: [{ glotpress: 'fakegploc', android: 'fakeanloc' }], fail_on_error: true) + end.to raise_error(FastlaneCore::Interface::FastlaneError, /At least one GlotPress filter is required/) + end + end + + ['', 'Service unavailable'].each do |response_body| + it "raises instead of writing an invalid successful export: #{response_body.inspect}" do + FileUtils.mkdir_p(File.dirname(generated_file(nil))) + FileUtils.cp(expected_file(nil), generated_file(nil)) + stub_request(:get, "#{gp_fake_url.chomp('/')}/fakegploc/default/export-translations/?filters%5Bstatus%5D=current&format=android") + .to_return(status: 200, body: response_body) + + expect do + described_class.download_from_glotpress(res_dir: tmpdir, glotpress_project_url: gp_fake_url, locales_map: [{ glotpress: 'fakegploc', android: 'fakeanloc' }], fail_on_error: true) + end.to raise_error(FastlaneCore::Interface::FastlaneError, /Invalid Android translation export.*fakegploc/) + + expect(File).not_to exist(generated_file('fakeanloc')) + end + end + + it 'retains permissive response handling by default' do + FileUtils.mkdir_p(File.dirname(generated_file(nil))) + FileUtils.cp(expected_file(nil), generated_file(nil)) + body = 'Service unavailable' + stub_request(:get, "#{gp_fake_url.chomp('/')}/fakegploc/default/export-translations/?filters%5Bstatus%5D=current&format=android").to_return(status: 200, body: body) + + expect do + described_class.download_from_glotpress(res_dir: tmpdir, glotpress_project_url: gp_fake_url, locales_map: [{ glotpress: 'fakegploc', android: 'fakeanloc' }]) + end.not_to raise_error + expect(File.read(generated_file('fakeanloc'))).to include(' 'Automattic App Release Automator; https://github.com/wordpress-mobile/release-toolkit/' } - ).to_return(status: 200, body: '') + ).to_return(status: 200, body: '') # Act described_class.download_from_glotpress( diff --git a/spec/glotpress_downloader_spec.rb b/spec/glotpress_downloader_spec.rb index 97121e1dd..4faeae51e 100644 --- a/spec/glotpress_downloader_spec.rb +++ b/spec/glotpress_downloader_spec.rb @@ -18,6 +18,37 @@ expect(result).to eq('test content') end + it 'returns true when downloading without a block' do + stub_request(:get, test_url) + .to_return(status: 200, body: 'test content') + + downloader = described_class.new(url: test_url, locale: locale, auto_retry: false) + + expect(downloader.download).to be(true) + end + + it 'does not report success until the downloaded body has been accepted' do + stub_request(:get, test_url) + .to_return(status: 200, body: 'invalid content') + expect(FastlaneCore::UI).not_to receive(:success) + + downloader = described_class.new(url: test_url, locale: locale, auto_retry: false) + + expect do + downloader.download { raise 'invalid downloaded body' } + end.to raise_error(RuntimeError, 'invalid downloaded body') + end + + it 'does not report success when the downloaded body is rejected' do + stub_request(:get, test_url) + .to_return(status: 200, body: 'rejected content') + expect(FastlaneCore::UI).not_to receive(:success) + + downloader = described_class.new(url: test_url, locale: locale, auto_retry: false) + + expect(downloader.download { false }).to be(false) + end + it 'resets retry counter at start of download' do # Counter should be reset to 0 at the start of download() call downloader = described_class.new(url: test_url, locale: locale, auto_retry: true) @@ -91,14 +122,16 @@ it 'stops retrying after max attempts' do stub_request(:get, test_url).to_return(status: 429) - downloader = described_class.new(url: test_url, locale: locale, auto_retry: true) + downloader = described_class.new(url: test_url, locale: locale, auto_retry: true, fail_on_error: true) allow(downloader).to receive(:sleep).with(20) # Mock non-interactive environment to avoid prompts allow(FastlaneCore::UI).to receive(:interactive?).and_return(false) - allow(FastlaneCore::UI).to receive(:error) + expect(FastlaneCore::UI).to receive(:error).with(/Error downloading locale `test-locale` — 429.*#{Regexp.escape(test_url)}/) - downloader.download { |body| body } + expect do + downloader.download { |body| body } + end.to raise_error(FastlaneCore::Interface::FastlaneError, /429/) # Should try: 1 initial + 30 retries = 31 total expect(a_request(:get, test_url)).to have_been_made.times(31) @@ -107,13 +140,15 @@ it 'does not auto-retry when auto_retry is disabled' do stub_request(:get, test_url).to_return(status: 429) - downloader = described_class.new(url: test_url, locale: locale, auto_retry: false) + downloader = described_class.new(url: test_url, locale: locale, auto_retry: false, fail_on_error: true) # Mock UI methods to avoid prompts allow(FastlaneCore::UI).to receive(:error) - allow(FastlaneCore::UI).to receive(:confirm).and_return(false) + allow(FastlaneCore::UI).to receive(:interactive?).and_return(false) - downloader.download { |body| body } + expect do + downloader.download { |body| body } + end.to raise_error(FastlaneCore::Interface::FastlaneError, /429/) # Should only try once (no auto-retry) expect(a_request(:get, test_url)).to have_been_made.once @@ -136,22 +171,87 @@ expect(a_request(:get, test_url)).to have_been_made.once expect(a_request(:get, redirect_url)).to have_been_made.once end + + it 'resolves relative redirect locations' do + redirect_url = 'https://translate.wordpress.org/redirected' + stub_request(:get, test_url) + .to_return(status: 302, headers: { 'Location' => '/redirected' }) + stub_request(:get, redirect_url) + .to_return(status: 200, body: 'redirected content') + + downloader = described_class.new(url: test_url, locale: locale, auto_retry: false) + + expect(downloader.download).to be(true) + expect(a_request(:get, redirect_url)).to have_been_made.once + end + + it 'raises when a redirect has no location header' do + stub_request(:get, test_url).to_return(status: 302) + + downloader = described_class.new(url: test_url, locale: locale, auto_retry: false, fail_on_error: true) + + expect do + downloader.download { |body| body } + end.to raise_error(FastlaneCore::Interface::FastlaneError, "Received 302 for `test-locale` but no location header was found (#{test_url}).") + end + + it 'raises when the maximum number of redirects is exceeded' do + stub_request(:get, test_url) + .to_return(status: 302, headers: { 'Location' => test_url }) + + downloader = described_class.new(url: test_url, locale: locale, auto_retry: false, fail_on_error: true) + + expect { downloader.download { |body| body } }.to raise_error(FastlaneCore::Interface::FastlaneError, /Too many redirects/) + expect(a_request(:get, test_url)).to have_been_made.times(described_class::MAX_REDIRECTS + 1) + end end describe 'error handling' do - it 'handles 404 errors gracefully in non-interactive mode' do + it 'returns falsey results by default' do stub_request(:get, test_url).to_return(status: 404, body: 'Not Found') + allow(FastlaneCore::UI).to receive(:interactive?).and_return(false) + allow(FastlaneCore::UI).to receive(:error) - downloader = described_class.new(url: test_url, locale: locale, auto_retry: false) + downloader = described_class.new(url: test_url, locale: locale) + + expect(downloader.download { |body| body }).to be_nil + expect(downloader.download).to be(false) + expect(a_request(:get, test_url)).to have_been_made.times(2) + end + + it 'raises on 404 errors in non-interactive mode' do + stub_request(:get, test_url).to_return(status: 404, body: 'Not Found') + + downloader = described_class.new(url: test_url, locale: locale, auto_retry: false, fail_on_error: true) # Mock non-interactive environment allow(FastlaneCore::UI).to receive(:interactive?).and_return(false) allow(FastlaneCore::UI).to receive(:error) - result = downloader.download { |body| body } + expect do + downloader.download { |body| body } + end.to raise_error(FastlaneCore::Interface::FastlaneError, /404/) + expect(a_request(:get, test_url)).to have_been_made.once + end - expect(result).to be_falsey + it 'raises on SSL errors in non-interactive mode' do + stub_request(:get, test_url).to_raise(OpenSSL::SSL::SSLError.new('certificate verify failed')) + + downloader = described_class.new(url: test_url, locale: locale, auto_retry: false, fail_on_error: true) + allow(FastlaneCore::UI).to receive(:interactive?).and_return(false) + allow(FastlaneCore::UI).to receive(:error) + + expect do + downloader.download { |body| body } + end.to raise_error(FastlaneCore::Interface::FastlaneError, /certificate verify failed/) expect(a_request(:get, test_url)).to have_been_made.once end + + it 'raises a clean user error for invalid URLs' do + invalid_url = 'https://translate.wordpress.org/an invalid path' + downloader = described_class.new(url: invalid_url, locale: locale, auto_retry: false, fail_on_error: true) + + expect { downloader.download { |body| body } }.to raise_error(FastlaneCore::Interface::FastlaneError, /Invalid URL for locale `test-locale`.*an invalid path/) + end end end diff --git a/spec/gp_downloadmetadata_action_spec.rb b/spec/gp_downloadmetadata_action_spec.rb new file mode 100644 index 000000000..febf07685 --- /dev/null +++ b/spec/gp_downloadmetadata_action_spec.rb @@ -0,0 +1,28 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Fastlane::Actions::GpDownloadmetadataAction do + it 'passes fail_on_error to the metadata downloader' do + in_tmp_dir do |tmpdir| + expect(Fastlane::Helper::MetadataDownloader).to receive(:new).with(tmpdir, {}, false, fail_on_error: true) + + run_described_fastlane_action( + project_url: 'https://translate.example/projects/test/', + target_files: {}, + locales: [], + source_locale: nil, + download_path: tmpdir, + auto_retry: false, + fail_on_error: true + ) + end + end + + it 'keeps fail_on_error disabled by default' do + option = described_class.available_options.find { |item| item.key == :fail_on_error } + + expect(option.default_value).to be(false) + expect(option.env_name).to be_nil + end +end diff --git a/spec/ios_download_strings_files_from_glotpress_spec.rb b/spec/ios_download_strings_files_from_glotpress_spec.rb index 573beeb6c..529d24293 100644 --- a/spec/ios_download_strings_files_from_glotpress_spec.rb +++ b/spec/ios_download_strings_files_from_glotpress_spec.rb @@ -12,6 +12,12 @@ def gp_stub(locale:, query:) stub_request(:get, "#{gp_fake_url}/#{locale}/default/export-translations/").with(query: query) end + it 'does not expose fail_on_error through an environment variable' do + option = described_class.available_options.find { |item| item.key == :fail_on_error } + + expect(option.env_name).to be_nil + end + describe 'downloading export files from GlotPress' do def test_gp_download(filters:, tablename:, expected_gp_params:) Dir.mktmpdir('a8c-release-toolkit-tests-') do |tmp_dir| @@ -66,7 +72,7 @@ def test_gp_download(filters:, tablename:, expected_gp_params:) end describe 'error handling' do - it 'shows an error if an invalid locale is provided (404)' do + it 'raises if an invalid locale is provided (404)' do Dir.mktmpdir('a8c-release-toolkit-tests-') do |tmp_dir| # Arrange stub = gp_stub(locale: 'unknown-locale', query: { 'filters[status]': 'current', format: 'strings' }).to_return(status: [404, 'Not Found']) @@ -75,13 +81,17 @@ def test_gp_download(filters:, tablename:, expected_gp_params:) allow(FastlaneCore::UI).to receive(:confirm).and_return(false) # as we will be asked if we want to retry when getting a network error # Act - run_described_fastlane_action( - project_url: gp_fake_url, - locales: { 'unknown-locale': 'Base' }, - download_dir: tmp_dir - ) + act = lambda do + run_described_fastlane_action( + project_url: gp_fake_url, + locales: { 'unknown-locale': 'Base' }, + download_dir: tmp_dir, + fail_on_error: true + ) + end # Assert + expect { act.call }.to raise_error(FastlaneCore::Interface::FastlaneError, /404 Not Found/) expect(stub).to have_been_made.once expect(File).not_to exist(File.join(tmp_dir, 'Base.lproj', 'Localizable.strings')) expect(error_messages).to eq(["Error downloading locale `unknown-locale` — 404 Not Found (#{gp_fake_url}/unknown-locale/default/export-translations/?filters%5Bstatus%5D=current&format=strings)"]) @@ -106,55 +116,134 @@ def test_gp_download(filters:, tablename:, expected_gp_params:) expect { act.call }.to raise_error(FastlaneCore::Interface::FastlaneError, "The parent directory `#{download_dir}` (which contains all the `*.lproj` subdirectories) must already exist") end - it 'reports if a downloaded file is not a valid `.strings` file' do + it 'does not replace an existing file when a permissive download fails' do Dir.mktmpdir('a8c-release-toolkit-tests-') do |tmp_dir| - # Arrange - stub = gp_stub(locale: 'fr-FR', query: { 'filters[status]': 'current', format: 'strings' }).to_return(body: 'some invalid strings file content') + stub = gp_stub(locale: 'fr-FR', query: { 'filters[status]': 'current', format: 'strings' }).to_return(status: [500, 'Internal Server Error']) + allow(FastlaneCore::UI).to receive(:confirm).and_return(false) + file = File.join(tmp_dir, 'fr.lproj', 'Localizable.strings') + FileUtils.mkdir_p(File.dirname(file)) + File.write(file, 'existing valid content') + + expect do + run_described_fastlane_action(project_url: gp_fake_url, locales: { 'fr-FR': 'fr' }, download_dir: tmp_dir) + end.not_to raise_error + + expect(stub).to have_been_made.once + expect(File.read(file)).to eq('existing valid content') + end + end + + it 'reports invalid files without failing by default' do + Dir.mktmpdir('a8c-release-toolkit-tests-') do |tmp_dir| + body = 'some invalid strings file content' + stub = gp_stub(locale: 'fr-FR', query: { 'filters[status]': 'current', format: 'strings' }).to_return(body: body) error_messages = [] allow(FastlaneCore::UI).to receive(:error) { |message| error_messages.append(message) } - # Act - run_described_fastlane_action( - project_url: gp_fake_url, - locales: { 'fr-FR': 'fr' }, - download_dir: tmp_dir - ) + expect do + run_described_fastlane_action(project_url: gp_fake_url, locales: { 'fr-FR': 'fr' }, download_dir: tmp_dir) + end.not_to raise_error - # Assert + file = File.join(tmp_dir, 'fr.lproj', 'Localizable.strings') expect(stub).to have_been_made.once + expect(File.read(file)).to eq(body) + expect(error_messages.first).to start_with('Error while validating the file exported from GlotPress') + end + end + + it 'reports non-dictionary files without failing by default' do + Dir.mktmpdir('a8c-release-toolkit-tests-') do |tmp_dir| + body = '("value")' + stub = gp_stub(locale: 'fr-FR', query: { 'filters[status]': 'current', format: 'strings' }).to_return(body: body) + error_messages = [] + allow(FastlaneCore::UI).to receive(:error) { |message| error_messages.append(message) } + + expect do + run_described_fastlane_action(project_url: gp_fake_url, locales: { 'fr-FR': 'fr' }, download_dir: tmp_dir) + end.not_to raise_error + file = File.join(tmp_dir, 'fr.lproj', 'Localizable.strings') - expect(File).to exist(file) - expected_error = 'Property List error: Unexpected character s at line 1 / JSON error: JSON text did not start with array or object and option to allow fragments not set.' - expect(error_messages.count).to eq(1) - expect(error_messages.first).to start_with("Error while validating the file exported from GlotPress (`#{file}`) - #{file}: #{expected_error}") # Different versions of `plutil` might append the line/column as well, but not all. + expect(stub).to have_been_made.once + expect(File.read(file)).to eq(body) + expect(error_messages).to contain_exactly(a_string_matching(/string-to-string dictionary.*#{Regexp.escape(file)}/)) + end + end + + it 'raises before downloading if the strict destination is not a regular file' do + Dir.mktmpdir('a8c-release-toolkit-tests-') do |tmp_dir| + destination = File.join(tmp_dir, 'fr.lproj', 'Localizable.strings') + FileUtils.mkdir_p(destination) + stub = gp_stub(locale: 'fr-FR', query: { 'filters[status]': 'current', format: 'strings' }).to_return(body: '"key" = "value";') + + expect do + run_described_fastlane_action(project_url: gp_fake_url, locales: { 'fr-FR': 'fr' }, download_dir: tmp_dir, fail_on_error: true) + end.to raise_error(FastlaneCore::Interface::FastlaneError, /destination.*not a regular file/i) + + expect(stub).not_to have_been_made + expect(Dir).to be_empty(destination) end end - it 'reports if a downloaded file has empty translations' do + it 'reports a non-regular destination without failing by default' do + Dir.mktmpdir('a8c-release-toolkit-tests-') do |tmp_dir| + destination = File.join(tmp_dir, 'fr.lproj', 'Localizable.strings') + FileUtils.mkdir_p(destination) + stub = gp_stub(locale: 'fr-FR', query: { 'filters[status]': 'current', format: 'strings' }).to_return(body: '"key" = "value";') + error_messages = [] + allow(FastlaneCore::UI).to receive(:error) { |message| error_messages.append(message) } + + expect do + run_described_fastlane_action(project_url: gp_fake_url, locales: { 'fr-FR': 'fr' }, download_dir: tmp_dir) + end.not_to raise_error + + expect(stub).not_to have_been_made + expect(error_messages).to eq(["The destination `#{destination}` exists but is not a regular file"]) + end + end + + [ + ['invalid', 'some invalid strings file content', /Error while validating the file exported from GlotPress.*Property List error/m], + ['empty', '', /file exported from GlotPress is empty/], + ['not a dictionary', '("value")', /string-to-string dictionary/], + ['not string-to-string', '"key" = ("value");', /string-to-string dictionary/], + ].each do |description, response_body, expected_error| + it "raises without replacing the existing file if a download is #{description}" do + Dir.mktmpdir('a8c-release-toolkit-tests-') do |tmp_dir| + stub = gp_stub(locale: 'fr-FR', query: { 'filters[status]': 'current', format: 'strings' }).to_return(body: response_body) + file = File.join(tmp_dir, 'fr.lproj', 'Localizable.strings') + FileUtils.mkdir_p(File.dirname(file)) + File.write(file, 'existing valid content') + + expect do + run_described_fastlane_action(project_url: gp_fake_url, locales: { 'fr-FR': 'fr' }, download_dir: tmp_dir, fail_on_error: true) + end.to raise_error(FastlaneCore::Interface::FastlaneError, expected_error) + + expect(stub).to have_been_made.once + expect(File.read(file)).to eq('existing valid content') + end + end + end + + it 'raises if a downloaded file has empty translations' do Dir.mktmpdir('a8c-release-toolkit-tests-') do |tmp_dir| # Arrange stub = gp_stub(locale: 'fr-FR', query: { 'filters[status]': 'current', format: 'strings' }) .to_return(body: ['"key1" = "value1";', '"key2" = "";', '"key3" = "";', '/* translators: use "" quotes please */', '"key4" = "value4";'].join("\n")) - error_messages = [] - allow(FastlaneCore::UI).to receive(:error) { |message| error_messages.append(message) } # Act - run_described_fastlane_action( - project_url: gp_fake_url, - locales: { 'fr-FR': 'fr' }, - download_dir: tmp_dir - ) + expect do + run_described_fastlane_action( + project_url: gp_fake_url, + locales: { 'fr-FR': 'fr' }, + download_dir: tmp_dir, + fail_on_error: true + ) + end.to raise_error(FastlaneCore::Interface::FastlaneError, /Found empty translations.*\["key2", "key3"\]/m) # Assert expect(stub).to have_been_made.once file = File.join(tmp_dir, 'fr.lproj', 'Localizable.strings') - expect(File).to exist(file) - expected_error = <<~MSG.chomp - Found empty translations in `#{file}` for the following keys: ["key2", "key3"]. - This is likely a GlotPress bug, and will lead to copies replaced by empty text in the UI. - Please report this to the GlotPress team, and fix the file locally before continuing. - MSG - expect(error_messages).to eq([expected_error]) + expect(File).not_to exist(file) end end diff --git a/spec/ios_l10n_helper_spec.rb b/spec/ios_l10n_helper_spec.rb index e7e492ebb..ab8bdbb96 100644 --- a/spec/ios_l10n_helper_spec.rb +++ b/spec/ios_l10n_helper_spec.rb @@ -409,13 +409,15 @@ def file_encoding(path) stub = stub_request(:get, "#{gp_fake_url}/fr/default/export-translations/").with(query: { format: 'strings' }).to_return(body: 'content') error_messages = [] allow(FastlaneCore::UI).to receive(:error) { |message| error_messages.append(message) } + expect(FastlaneCore::UI).not_to receive(:success) dest = '/these/are/not/the/droids/you/are/looking/for.strings' # Act - described_class.download_glotpress_export_file(project_url: gp_fake_url, locale: 'fr', filters: nil, destination: dest) + result = described_class.download_glotpress_export_file(project_url: gp_fake_url, locale: 'fr', filters: nil, destination: dest) # Assert expect(stub).to have_been_made.once expect(File).not_to exist(dest) - expect(error_messages).to eq(["Error downloading locale `fr` — No such file or directory @ rb_sysopen - #{dest} (#{gp_fake_url}/fr/default/export-translations/?format=strings)"]) + expect(result).to be(false) + expect(error_messages).to eq(["Error writing downloaded locale `fr` — No such file or directory @ rb_sysopen - #{dest} (#{gp_fake_url}/fr/default/export-translations/?format=strings)"]) end end end diff --git a/spec/metadata_download_helper_spec.rb b/spec/metadata_download_helper_spec.rb new file mode 100644 index 000000000..724821caa --- /dev/null +++ b/spec/metadata_download_helper_spec.rb @@ -0,0 +1,67 @@ +# frozen_string_literal: true + +require_relative 'spec_helper' + +describe Fastlane::Helper::MetadataDownloader do + let(:test_url) { 'https://translate.wordpress.org/projects/test/locale/default/export-translations/' } + let(:target_files) { { release_notes: { desc: 'release_notes.txt', max_size: 0 } } } + + def existing_metadata_file(tmpdir) + path = File.join(tmpdir, 'fr', 'release_notes.txt') + FileUtils.mkdir_p(File.dirname(path)) + File.write(path, 'Existing release notes') + path + end + + it 'propagates download failures' do + stub_request(:get, test_url).to_return(status: 500) + allow(FastlaneCore::UI).to receive(:interactive?).and_return(false) + + in_tmp_dir do |tmpdir| + downloader = described_class.new(tmpdir, target_files, false, fail_on_error: true) + + expect do + downloader.download('fr', test_url, false) + end.to raise_error(FastlaneCore::Interface::FastlaneError, /500/) + end + end + + ['{', 'null', '42', '{}', '["unexpected"]', '{"error":"maintenance"}'].each do |response_body| + it "raises without deleting existing metadata for an invalid response body: #{response_body}" do + stub_request(:get, test_url).to_return(status: 200, body: response_body) + + in_tmp_dir do |tmpdir| + existing_file = existing_metadata_file(tmpdir) + downloader = described_class.new(tmpdir, target_files, false, fail_on_error: true) + + expect { downloader.download('fr', test_url, false) }.to raise_error(FastlaneCore::Interface::FastlaneError, /GlotPress.*locale `fr`.*#{Regexp.escape(test_url)}/) + expect(File.read(existing_file)).to eq('Existing release notes') + end + end + end + + it 'accepts a legitimate empty export and removes stale metadata' do + stub_request(:get, test_url).to_return(status: 200, body: '[]') + + in_tmp_dir do |tmpdir| + existing_file = existing_metadata_file(tmpdir) + downloader = described_class.new(tmpdir, target_files, false, fail_on_error: true) + + downloader.download('fr', test_url, false) + + expect(File).not_to exist(existing_file) + end + end + + it 'retains legacy malformed-response handling by default' do + stub_request(:get, test_url).to_return(status: 200, body: '{') + + in_tmp_dir do |tmpdir| + existing_file = existing_metadata_file(tmpdir) + downloader = described_class.new(tmpdir, target_files, false) + + expect { downloader.download('fr', test_url, false) }.not_to raise_error + expect(File).not_to exist(existing_file) + end + end +end diff --git a/spec/update_apps_cdn_build_metadata_spec.rb b/spec/update_apps_cdn_build_metadata_spec.rb index 797ceaadb..b8531a8d2 100644 --- a/spec/update_apps_cdn_build_metadata_spec.rb +++ b/spec/update_apps_cdn_build_metadata_spec.rb @@ -21,10 +21,6 @@ }.to_json end - before do - WebMock.disable_net_connect! - end - describe 'updating visibility' do it 'successfully updates the visibility to external' do stub_request(:post, api_url) diff --git a/spec/upload_build_to_apps_cdn_spec.rb b/spec/upload_build_to_apps_cdn_spec.rb index 96788708f..d7c906bc1 100644 --- a/spec/upload_build_to_apps_cdn_spec.rb +++ b/spec/upload_build_to_apps_cdn_spec.rb @@ -39,14 +39,9 @@ end before do - WebMock.disable_net_connect! allow(SecureRandom).to receive(:hex).with(10).and_return('dabad0001234dabad000') end - after do - WebMock.allow_net_connect! - end - # Helper method to build the expected multipart form data part def expected_form_part(name:, value:, filename: nil) lines = ["--#{test_boundary}"]