Address PR #1907 code review feedback (#1917)

justin808 · claude · web-flow · commit ca6366eb9f5e · 2025-11-04T19:22:21.000-10:00
* Fix critical error handling bug and improve test suite This commit addresses critical issues identified in code review: 1. **Fix error handling in silent mode** (CRITICAL) - Changed from warn/\$stderr to STDERR.puts constant - Bypasses capture_output redirection to ensure errors always appear - Added RuboCop disable pragmas with detailed comments explaining why 2. **Refactor test suite to mock at boundaries** - Removed mocking of private methods (run_rake_task_directly) - Now mocks Rake::Task and system calls directly - Tests verify actual behavior, not implementation details - More resilient to refactoring 3. **Add comprehensive error handling tests** - Test that errors appear in stderr even in silent mode - Test backtrace inclusion when DEBUG env is set - Test stdout suppression in silent mode - Test Rails fallback when not available 4. **Improve test coverage and quality** - 10 examples, all passing - Tests now verify error output reaches STDERR correctly - Added context for Rails not being available All tests pass (10 examples, 0 failures) RuboCop passes with no offenses 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * Fix critical missing StringIO require and test stubbing issues This commit addresses critical issues identified in code review: 1. **Add missing StringIO require** (BLOCKER) - Added `require "stringio"` at line 4 - Required for capture_output method at line 91 - Without this, code fails at runtime in silent mode with: NameError: uninitialized constant StringIO 2. **Fix inconsistent system call stubbing in tests** - Updated "when not in Bundler context" tests to match actual implementation - Changed from string form: `system("bundle exec rake...")` - To array form: `system("bundle", "exec", "rake", ...)` - Silent mode now correctly stubs with File::NULL options - This matches the actual implementation at lines 116-121 3. **Fix Rails fallback test stubbing** - Updated "when Rails is not available" test - Now uses correct array form system call signature All tests pass (10 examples, 0 failures) RuboCop passes with no offenses 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: Claude <noreply@anthropic.com>
diff --git a/lib/react_on_rails/dev/pack_generator.rb b/lib/react_on_rails/dev/pack_generator.rb
@@ -1,6 +1,7 @@
 # frozen_string_literal: true
 
 require "English"
+require "stringio"
 
 module ReactOnRails
   module Dev
@@ -105,7 +106,10 @@ def handle_rake_error(error, _silent)
           error_msg += "\n#{error.backtrace.join("\n")}" if ENV["DEBUG"]
 
           # Always write to stderr, even in silent mode
-          warn error_msg
+          # Use STDERR constant instead of warn/$stderr to bypass capture_output redirection
+          # rubocop:disable Style/StderrPuts, Style/GlobalStdStream
+          STDERR.puts error_msg
+          # rubocop:enable Style/StderrPuts, Style/GlobalStdStream
         end
 
         def run_via_bundle_exec(silent: false)
diff --git a/spec/react_on_rails/dev/pack_generator_spec.rb b/spec/react_on_rails/dev/pack_generator_spec.rb
@@ -5,30 +5,167 @@
 
 RSpec.describe ReactOnRails::Dev::PackGenerator do
   describe ".generate" do
-    it "runs pack generation successfully in verbose mode" do
-      allow(described_class).to receive(:system)
-        .with("bundle", "exec", "rake", "react_on_rails:generate_packs")
-        .and_return(true)
+    context "when in Bundler context with Rails available" do
+      let(:mock_task) { instance_double(Rake::Task) }
+      let(:mock_rails_app) do
+        # rubocop:disable RSpec/VerifiedDoubles
+        double("Rails.application").tap do |app|
+          allow(app).to receive(:load_tasks)
+          allow(app).to receive(:respond_to?).with(:load_tasks).and_return(true)
+        end
+        # rubocop:enable RSpec/VerifiedDoubles
+      end
 
-      expect { described_class.generate(verbose: true) }
-        .to output(/📦 Generating React on Rails packs.../).to_stdout_from_any_process
+      before do
+        # Setup Bundler context
+        stub_const("Bundler", Module.new)
+        allow(ENV).to receive(:[]).and_call_original
+        allow(ENV).to receive(:[]).with("BUNDLE_GEMFILE").and_return("/path/to/Gemfile")
+
+        # Setup Rails availability
+        app = mock_rails_app
+        rails_module = Module.new do
+          define_singleton_method(:application) { app }
+          define_singleton_method(:respond_to?) { |method, *| method == :application }
+        end
+        stub_const("Rails", rails_module)
+
+        # Mock Rake::Task at the boundary
+        allow(Rake::Task).to receive(:task_defined?).with("react_on_rails:generate_packs").and_return(false)
+        allow(Rake::Task).to receive(:[]).with("react_on_rails:generate_packs").and_return(mock_task)
+        allow(mock_task).to receive(:reenable)
+        allow(mock_task).to receive(:invoke)
+      end
+
+      it "runs pack generation successfully in verbose mode using direct rake execution" do
+        expect { described_class.generate(verbose: true) }
+          .to output(/📦 Generating React on Rails packs.../).to_stdout_from_any_process
+
+        expect(mock_task).to have_received(:invoke)
+        expect(mock_rails_app).to have_received(:load_tasks)
+      end
+
+      it "runs pack generation successfully in quiet mode using direct rake execution" do
+        expect { described_class.generate(verbose: false) }
+          .to output(/📦 Generating packs\.\.\. ✅/).to_stdout_from_any_process
+
+        expect(mock_task).to have_received(:invoke)
+      end
+
+      it "exits with error when pack generation fails" do
+        allow(mock_task).to receive(:invoke).and_raise(StandardError.new("Task failed"))
+
+        # Mock STDERR.puts to capture output
+        error_output = []
+        # rubocop:disable Style/GlobalStdStream
+        allow(STDERR).to receive(:puts) { |msg| error_output << msg }
+        # rubocop:enable Style/GlobalStdStream
+
+        expect { described_class.generate(verbose: false) }.to raise_error(SystemExit)
+        expect(error_output.join("\n")).to match(/Error generating packs: Task failed/)
+      end
+
+      it "outputs errors to stderr even in silent mode" do
+        allow(mock_task).to receive(:invoke).and_raise(StandardError.new("Silent mode error"))
+
+        # Mock STDERR.puts to capture output
+        error_output = []
+        # rubocop:disable Style/GlobalStdStream
+        allow(STDERR).to receive(:puts) { |msg| error_output << msg }
+        # rubocop:enable Style/GlobalStdStream
+
+        expect { described_class.generate(verbose: false) }.to raise_error(SystemExit)
+        expect(error_output.join("\n")).to match(/Error generating packs: Silent mode error/)
+      end
+
+      it "includes backtrace in error output when DEBUG env is set" do
+        allow(ENV).to receive(:[]).with("DEBUG").and_return("true")
+        allow(mock_task).to receive(:invoke).and_raise(StandardError.new("Debug error"))
+
+        # Mock STDERR.puts to capture output
+        error_output = []
+        # rubocop:disable Style/GlobalStdStream
+        allow(STDERR).to receive(:puts) { |msg| error_output << msg }
+        # rubocop:enable Style/GlobalStdStream
+
+        expect { described_class.generate(verbose: false) }.to raise_error(SystemExit)
+        expect(error_output.join("\n")).to match(/Error generating packs: Debug error.*pack_generator_spec\.rb/m)
+      end
+
+      it "suppresses stdout in silent mode" do
+        # Mock task to produce output
+        allow(mock_task).to receive(:invoke) do
+          puts "This should be suppressed"
+        end
+
+        expect { described_class.generate(verbose: false) }
+          .not_to output(/This should be suppressed/).to_stdout_from_any_process
+      end
     end
 
-    it "runs pack generation successfully in quiet mode" do
-      allow(described_class).to receive(:system)
-        .with("bundle", "exec", "rake", "react_on_rails:generate_packs", out: File::NULL, err: File::NULL)
-        .and_return(true)
+    context "when not in Bundler context" do
+      before do
+        # Ensure we're not in Bundler context
+        hide_const("Bundler") if defined?(Bundler)
+      end
+
+      it "runs pack generation successfully in verbose mode using bundle exec" do
+        allow(described_class).to receive(:system)
+          .with("bundle", "exec", "rake", "react_on_rails:generate_packs")
+          .and_return(true)
+
+        expect { described_class.generate(verbose: true) }
+          .to output(/📦 Generating React on Rails packs.../).to_stdout_from_any_process
+
+        expect(described_class).to have_received(:system)
+          .with("bundle", "exec", "rake", "react_on_rails:generate_packs")
+      end
+
+      it "runs pack generation successfully in quiet mode using bundle exec" do
+        allow(described_class).to receive(:system)
+          .with("bundle", "exec", "rake", "react_on_rails:generate_packs",
+                out: File::NULL, err: File::NULL)
+          .and_return(true)
 
-      expect { described_class.generate(verbose: false) }
-        .to output(/📦 Generating packs\.\.\. ✅/).to_stdout_from_any_process
+        expect { described_class.generate(verbose: false) }
+          .to output(/📦 Generating packs\.\.\. ✅/).to_stdout_from_any_process
+
+        expect(described_class).to have_received(:system)
+          .with("bundle", "exec", "rake", "react_on_rails:generate_packs",
+                out: File::NULL, err: File::NULL)
+      end
+
+      it "exits with error when pack generation fails" do
+        allow(described_class).to receive(:system)
+          .with("bundle", "exec", "rake", "react_on_rails:generate_packs",
+                out: File::NULL, err: File::NULL)
+          .and_return(false)
+
+        expect { described_class.generate(verbose: false) }.to raise_error(SystemExit)
+      end
     end
 
-    it "exits with error when pack generation fails" do
-      allow(described_class).to receive(:system)
-        .with("bundle", "exec", "rake", "react_on_rails:generate_packs", out: File::NULL, err: File::NULL)
-        .and_return(false)
+    context "when Rails is not available" do
+      before do
+        stub_const("Bundler", Module.new)
+        allow(ENV).to receive(:[]).and_call_original
+        allow(ENV).to receive(:[]).with("BUNDLE_GEMFILE").and_return("/path/to/Gemfile")
+
+        # Rails not available
+        hide_const("Rails") if defined?(Rails)
+      end
+
+      it "falls back to bundle exec when Rails is not defined" do
+        allow(described_class).to receive(:system)
+          .with("bundle", "exec", "rake", "react_on_rails:generate_packs")
+          .and_return(true)
+
+        expect { described_class.generate(verbose: true) }
+          .to output(/📦 Generating React on Rails packs.../).to_stdout_from_any_process
 
-      expect { described_class.generate(verbose: false) }.to raise_error(SystemExit)
+        expect(described_class).to have_received(:system)
+          .with("bundle", "exec", "rake", "react_on_rails:generate_packs")
+      end
     end
   end
 end
