From 8ac5a44133c1e1c613e2ec0dd3c7e5ca79037a2c Mon Sep 17 00:00:00 2001 From: Jonathan Hefner Date: Sun, 20 Oct 2019 18:17:26 -0500 Subject: [PATCH 1/2] Avoid shelling out for generator `generate` action This change addresses a few issues: First, shelling out to the `rails` command requires the destination directory to contain certain files that the command uses to initialize itself. While this is not an issue when running generators normally, it is troublesome when testing generator-invoking generators which output to ephemeral destination directories. Second, shelling out to the `rails` command is very slow. This also is not a particular concern when running generators normally, but it makes test suites for generator-invoking generators painfully slow. Third, shelling out to the `rails` command fails silently by default. Such silent failures can be surprising, and can lead to confusing downstream failures. --- railties/lib/rails/generators/actions.rb | 15 ++++++---- railties/test/generators/actions_test.rb | 37 ++++++++++++++++++------ 2 files changed, 38 insertions(+), 14 deletions(-) diff --git a/railties/lib/rails/generators/actions.rb b/railties/lib/rails/generators/actions.rb index c92fbb4c93..9c8302a710 100644 --- a/railties/lib/rails/generators/actions.rb +++ b/railties/lib/rails/generators/actions.rb @@ -1,5 +1,6 @@ # frozen_string_literal: true +require "shellwords" require "active_support/core_ext/string/strip" module Rails @@ -221,13 +222,17 @@ module Rails # the generator or an Array that is joined. # # generate(:authenticated, "user session") - def generate(what, *args) - log :generate, what - + def generate(*args) options = args.extract_options! - argument = args.flat_map(&:to_s).join(" ") + args = Shellwords.split(args.join(" ")) - execute_command :rails, "generate #{what} #{argument}", options + log :generate, args.first + + in_root do + silence_warnings do + ::Rails::Command.invoke("generate", args, options) + end + end end # Runs the supplied rake task (invoked with 'rake ...') diff --git a/railties/test/generators/actions_test.rb b/railties/test/generators/actions_test.rb index 60a32635d0..6159b9b8e9 100644 --- a/railties/test/generators/actions_test.rb +++ b/railties/test/generators/actions_test.rb @@ -378,22 +378,24 @@ class ActionsTest < Rails::Generators::TestCase assert_file "config/initializers/constants.rb", code.strip_heredoc end - def test_generate_should_run_script_generate_with_argument_and_options + test "generate" do run_generator action :generate, "model", "MyModel" assert_file "app/models/my_model.rb", /MyModel/ end - def test_generate_aborts_when_subprocess_fails_if_requested + test "generate with concatenated arguments" do run_generator - content = capture(:stderr) do - assert_raises SystemExit do - action :generate, "model", "MyModel:ADsad", abort_on_failure: true - action :generate, "model", "MyModel" - end + action :generate, "model MyModel name:string" + assert_file "app/models/my_model.rb", /MyModel/ + end + + test "generate should raise on failure" do + run_generator + error = assert_raises do + action :generate, "model", "1234567890" end - assert_match(/wrong constant name MyModel:aDsad/, content) - assert_no_file "app/models/my_model.rb" + assert_match(/1234567890/, error.message) end test "rake should run rake with the default environment" do @@ -438,6 +440,14 @@ class ActionsTest < Rails::Generators::TestCase end end + test "rake with abort_on_failure option should raise on failure" do + capture(:stderr) do + assert_raises SystemExit do + action :rake, "invalid", abort_on_failure: true + end + end + end + test "rails_command should run rails with the default environment" do assert_runs "rails log:clear", env: { "RAILS_ENV" => "development" } do with_rails_env nil do @@ -484,6 +494,15 @@ class ActionsTest < Rails::Generators::TestCase end end + test "rails_command with abort_on_failure option should raise on failure" do + run_generator + capture(:stderr) do + assert_raises SystemExit do + action :rails_command, "invalid", abort_on_failure: true + end + end + end + test "route should add route" do run_generator route_commands = ["get 'foo'", "get 'bar'", "get 'baz'"] From dcc3c85c2f78a065d5ecad1b146afee94e24b279 Mon Sep 17 00:00:00 2001 From: Jonathan Hefner Date: Thu, 9 Jan 2020 15:49:58 -0600 Subject: [PATCH 2/2] WIP inline option --- railties/lib/rails/generators/actions.rb | 26 ++++++++++------- railties/test/generators/actions_test.rb | 36 +++++++++++++++++++++--- 2 files changed, 48 insertions(+), 14 deletions(-) diff --git a/railties/lib/rails/generators/actions.rb b/railties/lib/rails/generators/actions.rb index 9c8302a710..9685995afd 100644 --- a/railties/lib/rails/generators/actions.rb +++ b/railties/lib/rails/generators/actions.rb @@ -222,17 +222,13 @@ module Rails # the generator or an Array that is joined. # # generate(:authenticated, "user session") - def generate(*args) + def generate(what, *args) + log :generate, what + options = args.extract_options! - args = Shellwords.split(args.join(" ")) + options[:abort_on_failure] = !options[:inline] - log :generate, args.first - - in_root do - silence_warnings do - ::Rails::Command.invoke("generate", args, options) - end - end + rails_command "generate #{what} #{args.join(" ")}", options end # Runs the supplied rake task (invoked with 'rake ...') @@ -252,7 +248,17 @@ module Rails # rails_command("gems:install", sudo: true) # rails_command("gems:install", capture: true) def rails_command(command, options = {}) - execute_command :rails, command, options + if options[:inline] + log :rails, command + command, *args = Shellwords.split(command) + in_root do + silence_warnings do + ::Rails::Command.invoke(command, args, options) + end + end + else + execute_command :rails, command, options + end end # Make an entry in Rails routing file config/routes.rb diff --git a/railties/test/generators/actions_test.rb b/railties/test/generators/actions_test.rb index 6159b9b8e9..8227a1310a 100644 --- a/railties/test/generators/actions_test.rb +++ b/railties/test/generators/actions_test.rb @@ -384,16 +384,28 @@ class ActionsTest < Rails::Generators::TestCase assert_file "app/models/my_model.rb", /MyModel/ end - test "generate with concatenated arguments" do + test "generate should raise on failure" do run_generator - action :generate, "model MyModel name:string" + message = capture(:stderr) do + assert_raises SystemExit do + action :generate, "model", "1234567890" + end + end + assert_match(/1234567890/, message) + end + + test "generate with inline option" do + run_generator + assert_not_called(generator, :run) do + action :generate, "model", "MyModel", inline: true + end assert_file "app/models/my_model.rb", /MyModel/ end - test "generate should raise on failure" do + test "generate with inline option should raise on failure" do run_generator error = assert_raises do - action :generate, "model", "1234567890" + action :generate, "model", "1234567890", inline: true end assert_match(/1234567890/, error.message) end @@ -503,6 +515,22 @@ class ActionsTest < Rails::Generators::TestCase end end + test "rails_command with inline option" do + run_generator + assert_not_called(generator, :run) do + action :rails_command, "generate model MyModel", inline: true + end + assert_file "app/models/my_model.rb", /MyModel/ + end + + test "rails_command with inline option should raise on failure" do + run_generator + error = assert_raises do + action :rails_command, "generate model 1234567890", inline: true + end + assert_match(/1234567890/, error.message) + end + test "route should add route" do run_generator route_commands = ["get 'foo'", "get 'bar'", "get 'baz'"]