Use rails_command :inline option #38429

jonathanhefner · 2020-02-10T23:55:19Z

Follow-up to #37516.

Running locally (on a not-very-powerful machine), I saw the time for railties/test/generators/app_generator_test.rb reduced from ~260 seconds to ~232 seconds. However, all that time came from a single call site (see my review comments below).

Follow-up to rails#37516.

jonathanhefner · 2020-02-11T00:04:14Z

railties/lib/rails/generators/rails/app/app_generator.rb

@@ -316,7 +316,7 @@ def update_bin_files

      def update_active_storage
        unless skip_active_storage?
-          rails_command "active_storage:update"
+          rails_command "active_storage:update", inline: true


This line had a noticeable effect on test run time (when run locally).

jonathanhefner · 2020-02-11T00:05:48Z

railties/lib/rails/generators/app_base.rb

+          rails_command "webpacker:install", inline: true
+          if options[:webpack] && options[:webpack] != "webpack"
+            rails_command "webpacker:install:#{options[:webpack]}", inline: true
+          end


These lines were hit by the tests I ran (I added logging to verify), but made no difference in test run time.

...Because these specifc calls to rails_command are stubbed when tested:

rails/railties/test/generators/app_generator_test.rb

Lines 903 to 924 in 7745146

def test_webpack_option_with_js_framework

command_check = -> command, *_ do

case command

when "webpacker:install"

@webpacker ||= 0

@webpacker += 1

assert_equal 1, @webpacker, "webpacker:install expected to be called once, but was called #{@webpacker} times."

when "webpacker:install:react"

@react ||= 0

@react += 1

assert_equal 1, @react, "webpacker:install:react expected to be called once, but was called #{@react} times."

end

end

generator([destination_root], webpack: "react").stub(:rails_command, command_check) do

generator.stub :bundle_command, nil do

quietly { generator.invoke_all }

end

end

assert_gem "webpacker"

end

That being the case, perhaps it's not safe to add inline: true here, since it isn't really being exercised?

jonathanhefner · 2020-02-11T00:07:08Z

actiontext/lib/generators/action_text/install/install_generator.rb

@@ -49,7 +49,7 @@ def create_actiontext_files
      end

      def create_migrations
-        rails_command "railties:install:migrations FROM=active_storage,action_text"
+        rails_command "railties:install:migrations FROM=active_storage,action_text", inline: true


This line was not hit by the tests I ran locally, so I am currently unsure of its impact.

jonathanhefner · 2020-02-11T00:07:15Z

actiontext/lib/generators/action_text/install/install_generator.rb

@@ -9,7 +9,7 @@ class InstallGenerator < ::Rails::Generators::Base
      source_root File.expand_path("templates", __dir__)

      def install_javascript_dependencies
-        rails_command "app:update:bin"
+        rails_command "app:update:bin", inline: true


This line was not hit by the tests I ran locally, so I am currently unsure of its impact.

jonathanhefner · 2020-02-11T00:37:06Z

All the tests pass, but I'm unable to see a reduction in build time on Buildkite. That's not entirely unexpected though, since Buildkite run times are highly variable.

Overall, I'm not sure if this patch is worth it. Perhaps others can chime in.

kaspth · 2020-02-11T03:59:57Z

I think these are fine, despite no noticeable runtime gain. I'd like to exercise the inline option and generally find that easier to reason about than shelling out. So the potential win with the active storage update one is just a nice bonus. Thanks for working on this 🙌

jonathanhefner · 2020-02-11T04:34:19Z

@kaspth Very well then! Thank you for your reviews and guidance! 💛

Follow-up to rails#38429. `Rails::Command.invoke` passes arguments through to the appropriate command class. However, some command classes were ignoring those arguments, and instead relying on the contents of ARGV. In particular, RakeCommand expected ARGV to contain the arguments necessary to the Rake task, and no other arguments. This caused the `webpacker:install` task to fail when the `--dev` option from `rails new --dev` remained in ARGV. This commit changes the relevant command classes to not rely on the previous contents of ARGV. This commit also adds a missing `require` for use of `Kernel#silence_warnings`. Fixes rails#38459.

Use rails_command :inline option

f6f5163

Follow-up to rails#37516.

rails-bot bot added actiontext railties labels Feb 10, 2020

jonathanhefner commented Feb 11, 2020

View reviewed changes

jonathanhefner mentioned this pull request Feb 11, 2020

Avoid shelling out for generator generate action #37516

Merged

kaspth merged commit 5d7f151 into rails:master Feb 11, 2020

jonathanhefner mentioned this pull request Feb 14, 2020

Fix rails new --dev #38463

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Use rails_command :inline option #38429

Use rails_command :inline option #38429

Uh oh!

jonathanhefner commented Feb 10, 2020 •

edited

Loading

Uh oh!

jonathanhefner Feb 11, 2020 •

edited

Loading

Uh oh!

jonathanhefner Feb 11, 2020

Uh oh!

jonathanhefner Feb 11, 2020

Uh oh!

jonathanhefner Feb 11, 2020

Uh oh!

jonathanhefner Feb 11, 2020

Uh oh!

jonathanhefner commented Feb 11, 2020

Uh oh!

kaspth commented Feb 11, 2020

Uh oh!

jonathanhefner commented Feb 11, 2020

Uh oh!

Uh oh!

	def test_webpack_option_with_js_framework
	command_check = -> command, *_ do
	case command
	when "webpacker:install"
	@webpacker \|\|= 0
	@webpacker += 1
	assert_equal 1, @webpacker, "webpacker:install expected to be called once, but was called #{@webpacker} times."
	when "webpacker:install:react"
	@react \|\|= 0
	@react += 1
	assert_equal 1, @react, "webpacker:install:react expected to be called once, but was called #{@react} times."
	end
	end

	generator([destination_root], webpack: "react").stub(:rails_command, command_check) do
	generator.stub :bundle_command, nil do
	quietly { generator.invoke_all }
	end
	end

	assert_gem "webpacker"
	end

Use rails_command :inline option #38429

Use rails_command :inline option #38429

Uh oh!

Conversation

jonathanhefner commented Feb 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonathanhefner Feb 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonathanhefner Feb 11, 2020

Choose a reason for hiding this comment

Uh oh!

jonathanhefner Feb 11, 2020

Choose a reason for hiding this comment

Uh oh!

jonathanhefner Feb 11, 2020

Choose a reason for hiding this comment

Uh oh!

jonathanhefner Feb 11, 2020

Choose a reason for hiding this comment

Uh oh!

jonathanhefner commented Feb 11, 2020

Uh oh!

kaspth commented Feb 11, 2020

Uh oh!

jonathanhefner commented Feb 11, 2020

Uh oh!

Uh oh!

jonathanhefner commented Feb 10, 2020 •

edited

Loading

jonathanhefner Feb 11, 2020 •

edited

Loading