Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move `dev:cache` rake task to use Rails::Command #33559

Merged
merged 3 commits into from Aug 15, 2018
Merged

Conversation

@anniecodes
Copy link
Contributor

anniecodes commented Aug 8, 2018

This PR:

  1. Moves the dev:cache rake task to be using Rails::Command instead without changing the API
  2. It adds deprecation around calling that command with rake instead of rails
@rails-bot
Copy link

rails-bot commented Aug 8, 2018

r? @sgrif

(@rails-bot has picked a reviewer for you, use r? to override)

railties/test/commands/dev_test.rb Outdated

test "`rails dev:cache` creates both caching and restart file when restart file doesn't exist and dev caching is currently off" do
Dir.chdir(app_path) do
refute File.exist?("tmp/caching-dev.txt")

This comment has been minimized.

@anniecodes

anniecodes Aug 8, 2018 Author Contributor

This might be too verbose but I thought it was making the before/after state clearer. I can get rid of those extra assertions if it feels too overkill to the reviewers :)

@guilleiguaran guilleiguaran requested a review from kaspth Aug 8, 2018
Copy link
Contributor

bogdanvlviv left a comment

Thanks for the PR!
Could you please fix rubocop issues https://codeclimate.com/github/rails/rails/pull/33559

railties/lib/rails/tasks/dev.rake Outdated
@@ -5,6 +5,7 @@ require "rails/dev_caching"
namespace :dev do
desc "Toggle development mode caching on/off"
task :cache do
Rails::DevCaching.enable_by_file
ActiveSupport::Deprecation.warn("Using `bin/rake dev:cache` is deprecated and will be removed in Rails 6.1. Use `bin/rails dev:cache` instead.\n")
Rails::Command.invoke 'dev:cache'

This comment has been minimized.

@bogdanvlviv

bogdanvlviv Aug 8, 2018 Contributor

We can remove require "rails/dev_caching" above in this file.

railties/test/commands/dev_test.rb Outdated

private
def run_dev_cache_command(args = [])
rails "dev:cache"

This comment has been minimized.

@bogdanvlviv

bogdanvlviv Aug 8, 2018 Contributor

rails "dev:cache", args

This comment has been minimized.

@anniecodes

anniecodes Aug 8, 2018 Author Contributor

I'm actually not passing args anywhere 🤦‍♀️ , I'll modify the signature.

railties/test/application/rake/dev_test.rb Outdated
assert File.exist?("tmp/restart.txt")

prev_mtime = File.mtime("tmp/restart.txt")
sleep(1)
rails "dev:cache"
`bin/rake dev:cache`

This comment has been minimized.

@bogdanvlviv

bogdanvlviv Aug 8, 2018 Contributor

Would be good to assert deprecation warning in each test of this file

This comment has been minimized.

@anniecodes

anniecodes Aug 8, 2018 Author Contributor

I tried to do that but it looks like the deprecation warning is not outputted in the same process and therefore is outputted to the console instead of caught by the output buffer.. Any suggestions?

This comment has been minimized.

@bogdanvlviv

bogdanvlviv Aug 8, 2018 Contributor

I think we can use

stderr = capture(:stderr) do
  # rake ...
end

assert_match(/DEPRECATION WARNING: .../, stderr)

I had similar case in a6ebafc

This comment has been minimized.

@anniecodes

anniecodes Aug 8, 2018 Author Contributor

Oh thanks for complementing my change to NotesCommand 😄 . I'll capture stdout. Thanks for the tip

railties/test/commands/dev_test.rb Outdated

require "isolation/abstract_unit"
require "rails/command"
require "rails/commands/dev/dev_command"

This comment has been minimized.

@bogdanvlviv

bogdanvlviv Aug 8, 2018 Contributor

Do we really need to require file rails/commands/dev/dev_command?

This comment has been minimized.

@anniecodes

anniecodes Aug 8, 2018 Author Contributor

I followed the pattern of other commands test but you are right, I do not need it. I'll get rid of it.

@anniecodes anniecodes force-pushed the anniecodes:dev-task branch Aug 8, 2018
# frozen_string_literal: true

require "isolation/abstract_unit"
require "rails/command"

This comment has been minimized.

@bogdanvlviv

bogdanvlviv Aug 8, 2018 Contributor

This "require" also seems extra in this file since we don't use Rails::Command.
But I think we should add require "rails/command" to railties/lib/rails/tasks/dev.rake file

This comment has been minimized.

@bogdanvlviv

bogdanvlviv Aug 8, 2018 Contributor

I double checked it, we need this "require"
since we use class Rails::Command::DevTest < ActiveSupport::TestCase
and removing it causes uninitialized constant Rails::Command (NameError)

This comment has been minimized.

@anniecodes

anniecodes Aug 8, 2018 Author Contributor

Yes this one is necessary. I had double checked that one when checking the rails/commands/dev/dev_command.rb as well. I'll update the dev.rake with the require.

@anniecodes anniecodes force-pushed the anniecodes:dev-task branch Aug 8, 2018
@anniecodes anniecodes force-pushed the anniecodes:dev-task branch Aug 8, 2018
railties/test/commands/dev_test.rb Outdated

assert_not File.exist?("tmp/caching-dev.txt")
restart_file_time_after = File.mtime("tmp/restart.txt")
assert restart_file_time_before < restart_file_time_after

This comment has been minimized.

@Edouard-chin

Edouard-chin Aug 8, 2018 Member

you can use assert_operator, gives a nice message

assert_operator restart_file_time_before, :<, restart_file_time_after

This comment has been minimized.

@anniecodes

anniecodes Aug 9, 2018 Author Contributor

Haven't use Minitest in a while. TIL :) I'll update.

railties/test/application/rake/dev_test.rb Outdated
assert File.exist?("tmp/restart.txt")

prev_mtime = File.mtime("tmp/restart.txt")
sleep(1)

This comment has been minimized.

@Edouard-chin

Edouard-chin Aug 8, 2018 Member

Do we still have to sleep here since the Time are going to be compared (including milliseconds)

This comment has been minimized.

@anniecodes

anniecodes Aug 9, 2018 Author Contributor

I didn't touch this test since it was the rake task test that was already present and will be deleted in 6.1. But I don't think that sleep is necessary to start with. I'll get rid of it here too.

@anniecodes
Copy link
Contributor Author

anniecodes commented Aug 9, 2018

Test failures are unrelated to this PR. I'm going to rebase after the fix for these i18n related failures is on master.

@anniecodes anniecodes force-pushed the anniecodes:dev-task branch 2 times, most recently Aug 9, 2018
Copy link
Member

kaspth left a comment

Needs a commit squash as well, then 👍

@@ -1,10 +1,11 @@
# frozen_string_literal: true

require "rails/dev_caching"
require "rails/command"

This comment has been minimized.

@kaspth

kaspth Aug 11, 2018 Member

We should probably do require "active_support/deprecation" here as well.

This comment has been minimized.

@anniecodes

anniecodes Aug 13, 2018 Author Contributor

Done :)

@anniecodes anniecodes force-pushed the anniecodes:dev-task branch Aug 13, 2018
anniecodes added 3 commits Aug 8, 2018
* Call the Rails::Command::DevCommand in the rake task for dev:cache
* Add deprecation for using `bin/rake` in favor of `bin/rails`
@anniecodes anniecodes force-pushed the anniecodes:dev-task branch to 35a70f8 Aug 13, 2018
@kaspth kaspth merged commit efaa719 into rails:master Aug 15, 2018
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
codeclimate All good!
Details
@kaspth
Copy link
Member

kaspth commented Aug 15, 2018

Thanks, @anniecodes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.