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

Prevent multiple values being set to run_via #27941

Merged

Conversation

y-yagi
Copy link
Member

@y-yagi y-yagi commented Feb 8, 2017

When executing the test via rake, since rake is set for run_via, ruby should not be set.
Related 2cb6c27

@rails-bot
Copy link

r? @arthurnn

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

@kaspth
Copy link
Contributor

kaspth commented Feb 8, 2017

Think this means we can just remove the run_via[:rails] = true lines. And that storing the run_via as a hash was a poor decision on my part. How about:

def run_via
  @@run_via
end

def run_via=(runner)
  if @@run_via
    raise ArgumentError, 'run_via already assigned'
  else
    @@run_via = RunVia.new(runner)
  end
end

class RunVia < Struct.new(:runner)
  def ruby?
    runner == :ruby
  end

  def rake?
    runner == :rake
  end
end

(Could be cleaned up with an ActiveSupport::SymbolInquirer but that's for another time).

@rafaelfranca rafaelfranca assigned kaspth and unassigned arthurnn Feb 8, 2017
@y-yagi y-yagi force-pushed the prevent_multiple_values_being_set_to_run_via branch 3 times, most recently from bd44b86 to 4f2c3b7 Compare February 9, 2017 07:08
@y-yagi
Copy link
Member Author

y-yagi commented Feb 9, 2017

Looks great. I updated.

I have one concern. There was using Minitest.run_via in test script for rails plugin.
For that reason, I made it possible to set it by [] = for compatibility. What do you think?
(By the way, it was not good to call the method directly in test script, I will modify it to use a wrapper file.)

Copy link
Contributor

@kaspth kaspth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice call on the backwardscompatibility! Agreed a wrapper file would be nicer, as right now we're exposing a private API in a public file, but you know that 😊

end

def self.rake_run(patterns) # :nodoc:
run_via[:rake] = true
self.run_via = :rake if self.run_via.nil?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this guard?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is because rake_run is called multiple times when running multiple tests in one rake command(e.g. bin/rake test:models test:controllers).

@@run_via = nil unless defined? @@run_via

# For backward compatibility, makes it possible to set via `[]=`.
def @@run_via.[]=(runner, _)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't this add the []= method to nil? I don't see how it can provide backwardscompatibility reliably?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about revising the surface to:

mattr_reader(:run_via) { RunVia.new }

def self.run_via=(runner)
  if run_via.set?
    raise # …
  else
    run_via.runner = runner
  end
end

class RunVia
  attr_accessor :runner
  alias set? runner

  # Backwardscompatibility with Rails 5.0 generated plugin test scripts.
  alias []= runner=

  def ruby?
    @runner == :ruby
  end

  def rake?
    @runner == :rake
  end
end

Would also prevent NoMethodError's in cases where run_via.rake? is called without first having something assigned (albeit an unlikely scenario).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Great! I updated.

@@ -5,6 +5,6 @@ require 'rails/test_unit/minitest_plugin'

Rails::TestUnitReporter.executable = 'bin/test'

Minitest.run_via[:rails] = true
Minitest.run_via = :rails
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have any tasks akin to rails app:update for plugins that'll help updating this file and let us avoid the backwardscompatibility line?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently there is no update task for plugin. Also, plugin currently does not load the task provided by rails. Therefore, if provide a task similar to rails app:update for plugin, first need to update the plugin's Rakefile.
However, it is inconvenient that there is no task for updating, so I will add it with another PR.

When executing the test via rake, since `rake` is set for `run_via`, `ruby` should not be set.
Related 2cb6c27
@y-yagi y-yagi force-pushed the prevent_multiple_values_being_set_to_run_via branch from 4f2c3b7 to f38a660 Compare February 18, 2017 00:53
@kaspth kaspth merged commit ff326e7 into rails:master Feb 20, 2017
@kaspth
Copy link
Contributor

kaspth commented Feb 20, 2017

Sweet! ❤️

kaspth added a commit that referenced this pull request Feb 20, 2017
…et_to_run_via

Prevent multiple values being set to `run_via`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants