Use rails_command :inline option#38429
Conversation
Follow-up to rails#37516.
| def update_active_storage | ||
| unless skip_active_storage? | ||
| rails_command "active_storage:update" | ||
| rails_command "active_storage:update", inline: true |
There was a problem hiding this comment.
This line had a noticeable effect on test run time (when run locally).
| rails_command "webpacker:install", inline: true | ||
| if options[:webpack] && options[:webpack] != "webpack" | ||
| rails_command "webpacker:install:#{options[:webpack]}", inline: true | ||
| end |
There was a problem hiding this comment.
These lines were hit by the tests I ran (I added logging to verify), but made no difference in test run time.
There was a problem hiding this comment.
...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
That being the case, perhaps it's not safe to add inline: true here, since it isn't really being exercised?
|
|
||
| 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 |
There was a problem hiding this comment.
This line was not hit by the tests I ran locally, so I am currently unsure of its impact.
|
|
||
| def install_javascript_dependencies | ||
| rails_command "app:update:bin" | ||
| rails_command "app:update:bin", inline: true |
There was a problem hiding this comment.
This line was not hit by the tests I ran locally, so I am currently unsure of its impact.
|
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. |
|
I think these are fine, despite no noticeable runtime gain. I'd like to exercise the |
|
@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.
Follow-up to #37516.
Running locally (on a not-very-powerful machine), I saw the time for
railties/test/generators/app_generator_test.rbreduced from ~260 seconds to ~232 seconds. However, all that time came from a single call site (see my review comments below).