diff --git a/lib/project_types/theme/commands/push.rb b/lib/project_types/theme/commands/push.rb index f806fc99bb..2a01039dc7 100644 --- a/lib/project_types/theme/commands/push.rb +++ b/lib/project_types/theme/commands/push.rb @@ -72,6 +72,7 @@ def call(args, _name) @ctx.done(@ctx.message("theme.push.done", theme.preview_url, theme.editor_url)) end end + raise ShopifyCLI::AbortSilent if syncer.has_any_error? rescue ShopifyCLI::API::APIRequestNotFoundError @ctx.abort(@ctx.message("theme.push.theme_not_found", theme.id)) ensure diff --git a/lib/shopify_cli/theme/syncer.rb b/lib/shopify_cli/theme/syncer.rb index ab0547f0b8..8b697a0810 100644 --- a/lib/shopify_cli/theme/syncer.rb +++ b/lib/shopify_cli/theme/syncer.rb @@ -2,24 +2,28 @@ require "thread" require "json" require "base64" +require "forwardable" + +require_relative "syncer/error_reporter" +require_relative "syncer/operation" module ShopifyCLI module Theme class Syncer - class Operation < Struct.new(:method, :file) - def to_s - "#{method} #{file&.relative_path}" - end - end + extend Forwardable + API_VERSION = "unstable" attr_reader :checksums attr_accessor :ignore_filter + def_delegators :@error_reporter, :delay_errors!, :report_errors!, :has_any_error? + def initialize(ctx, theme:, ignore_filter: nil) @ctx = ctx @theme = theme @ignore_filter = ignore_filter + @error_reporter = ErrorReporter.new(ctx) # Queue of `Operation`s waiting to be picked up from a thread for processing. @queue = Queue.new @@ -30,10 +34,6 @@ def initialize(ctx, theme:, ignore_filter: nil) # Mutex used to pause all threads when backing-off when hitting API rate limits @backoff_mutex = Mutex.new - # Allows delaying log of errors, mainly to not break the progress bar. - @delay_errors = false - @delayed_errors = [] - # Latest theme assets checksums. Updated on each upload. @checksums = {} end @@ -103,7 +103,8 @@ def start_threads(count = 2) break if operation.nil? # shutdown was called perform(operation) rescue Exception => e - report_error( + + @error_reporter.report_error( "{{red:ERROR}} {{blue:#{operation}}}: #{e}" + (@ctx.debug? ? "\n\t#{e.backtrace.join("\n\t")}" : "") ) @@ -112,16 +113,6 @@ def start_threads(count = 2) end end - def delay_errors! - @delay_errors = true - end - - def report_errors! - @delay_errors = false - @delayed_errors.each { |error| report_error(error) } - @delayed_errors.clear - end - def upload_theme!(delay_low_priority_files: false, delete: true, &block) fetch_checksums! @@ -212,7 +203,7 @@ def perform(operation) backoff_if_near_limit!(used, total) end rescue ShopifyCLI::API::APIRequestError => e - report_error( + @error_reporter.report_error( "{{red:ERROR}} {{blue:#{operation}}}:\n " + parse_api_errors(e).join("\n ") ) @@ -295,14 +286,6 @@ def file_has_changed?(file) file.checksum != @checksums[file.relative_path.to_s] end - def report_error(error) - if @delay_errors - @delayed_errors << error - else - @ctx.puts(error) - end - end - def parse_api_errors(exception) parsed_body = JSON.parse(exception&.response&.body) message = parsed_body.dig("errors", "asset") || parsed_body["message"] || exception.message diff --git a/lib/shopify_cli/theme/syncer/error_reporter.rb b/lib/shopify_cli/theme/syncer/error_reporter.rb new file mode 100644 index 0000000000..d07d751907 --- /dev/null +++ b/lib/shopify_cli/theme/syncer/error_reporter.rb @@ -0,0 +1,45 @@ +# frozen_string_literal: true + +module ShopifyCLI + module Theme + class Syncer + ## + # ShopifyCLI::Theme::Syncer::ErrorReporter allows delaying log of errors, + # mainly to not break the progress bar. + # + class ErrorReporter + attr_reader :ctx, :delayed_errors + + def initialize(ctx) + @ctx = ctx + @has_any_error = false + @delay_errors = false + @delayed_errors = [] + end + + def delay_errors! + @delay_errors = true + end + + def report_errors! + @delay_errors = false + @delayed_errors.each { |error| report_error(error) } + @delayed_errors.clear + end + + def report_error(error_message) + if @delay_errors + @delayed_errors << error_message + else + @has_any_error = true + @ctx.puts(error_message) + end + end + + def has_any_error? + @has_any_error + end + end + end + end +end diff --git a/lib/shopify_cli/theme/syncer/operation.rb b/lib/shopify_cli/theme/syncer/operation.rb new file mode 100644 index 0000000000..a497b9697f --- /dev/null +++ b/lib/shopify_cli/theme/syncer/operation.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +module ShopifyCLI + module Theme + class Syncer + class Operation < Struct.new(:method, :file) + def to_s + "#{method} #{file&.relative_path}" + end + end + end + end +end diff --git a/test/project_types/theme/commands/push_test.rb b/test/project_types/theme/commands/push_test.rb index a0f7059133..a8db781951 100644 --- a/test/project_types/theme/commands/push_test.rb +++ b/test/project_types/theme/commands/push_test.rb @@ -21,7 +21,7 @@ def setup editor_url: "https://test.myshopify.io/", live?: false, ) - @syncer = stub("Syncer", delay_errors!: nil, report_errors!: nil) + @syncer = stub("Syncer", delay_errors!: nil, report_errors!: nil, has_any_error?: false) @ignore_filter = mock("IgnoreFilter") end @@ -123,6 +123,37 @@ def test_push_json @command.call([], "push") end + def test_push_when_syncer_has_an_error + syncer = stub("Syncer", delay_errors!: nil, report_errors!: nil, has_any_error?: true) + + ShopifyCLI::Theme::Theme.expects(:new) + .with(@ctx, root: ".", id: 1234) + .returns(@theme) + + @theme.expects(:to_h).returns({}) + + ShopifyCLI::Theme::IgnoreFilter.expects(:from_path).with(".").returns(@ignore_filter) + + ShopifyCLI::Theme::Syncer.expects(:new) + .with(@ctx, theme: @theme, ignore_filter: @ignore_filter) + .returns(syncer) + + syncer.expects(:start_threads) + syncer.expects(:shutdown) + + syncer.expects(:upload_theme!).with(delete: true) + @command.expects(:puts).with("{\"theme\":{}}") + + @ctx.expects(:puts).never + + @command.options.flags[:theme_id] = 1234 + @command.options.flags[:json] = 1234 + + assert_raises(ShopifyCLI::AbortSilent) do + @command.call([], "push") + end + end + def test_push_and_publish ShopifyCLI::Theme::Theme.expects(:new) .with(@ctx, root: ".", id: 1234) diff --git a/test/shopify-cli/theme/syncer/error_reporter_test.rb b/test/shopify-cli/theme/syncer/error_reporter_test.rb new file mode 100644 index 0000000000..6516ee28cb --- /dev/null +++ b/test/shopify-cli/theme/syncer/error_reporter_test.rb @@ -0,0 +1,59 @@ +# frozen_string_literal: true + +require "test_helper" +require "shopify_cli/theme/syncer" + +module ShopifyCLI + module Theme + class Syncer + class ErrorReporterTest < Minitest::Test + def setup + super + @error_reporter = ErrorReporter.new(@context) # stub(puts: nil)) + end + + def test_report_errors_with_standard_errors + io = capture_io do + @error_reporter.report_error("error 1") + @error_reporter.report_error("error 2") + end + + io_messages = io.join + + assert_match("error 1", io_messages) + assert_match("error 2", io_messages) + end + + def test_report_errors_with_delayed_errors + @error_reporter.delay_errors! + + before_report_errors = capture_io do + @error_reporter.report_error("error 1") + @error_reporter.report_error("error 2") + end + + after_report_errors = capture_io do + @error_reporter.report_errors! + end + + before_report_io = before_report_errors.join + after_report_io = after_report_errors.join + + assert_empty(before_report_io) + refute_empty(after_report_io) + assert_match("error 1", after_report_io) + assert_match("error 2", after_report_io) + end + + def test_has_any_error_when_no_error_was_reported + refute(@error_reporter.has_any_error?) + end + + def test_has_any_error_when_an_error_was_reported + @error_reporter.report_error("error 1") + assert(@error_reporter.has_any_error?) + end + end + end + end +end diff --git a/test/shopify-cli/theme/syncer/operation_test.rb b/test/shopify-cli/theme/syncer/operation_test.rb new file mode 100644 index 0000000000..d4c9532b26 --- /dev/null +++ b/test/shopify-cli/theme/syncer/operation_test.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +require "test_helper" +require "shopify_cli/theme/syncer" + +module ShopifyCLI + module Theme + class Syncer + class OperationTest < Minitest::Test + def test_to_s + file = stub(relative_path: "sections/apps.liquid") + operation = Operation.new("update", file) + + assert_equal("update sections/apps.liquid", operation.to_s) + end + + def test_to_s_when_file_is_nil + operation = Operation.new("update", nil) + + assert_equal("update ", operation.to_s) + end + end + end + end +end