Fix theme push --development --json to output the proper exit code (#…

…1769)

CHANGELOG.md

@@ -1,7 +1,8 @@
 From version 2.6.0, the sections in this file adhere to the [keep a changelog](https://keepachangelog.com/en/1.0.0/) specification.
 ## [Unreleased]
+### Fixed
+* [#1769](https://github.com/Shopify/shopify-cli/pull/1769): Fix `theme push --development --json` to output the proper exit code
 * [#1763](https://github.com/Shopify/shopify-cli/pull/1722): Fix: Tunnel --PORT parameter not working in Node.js app.
-
 ## Version 2.7.1
 ### Fixed
 * [#1722](https://github.com/Shopify/shopify-cli/pull/1763): Fix `theme serve` failing when the port is already being used

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

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

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

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

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)

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

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