Skip to content

Commit b1589fa

Browse files
committed
Let minitest parse ARGV:
- ### Problem See rails#54647 for the original reason this needs to be fixed. ### Context rails#54647 was reverted as we thought it broke running `bin/rails test test:system` where in fact this command was never a valid command, but it was previously silently ignoring the `test:system` arg (anything not considered a path is discarded). rails#54647 was in some way a better behaviour as it was at least failing loudly. Whether `bin/rails test test:system` should be supported (as discussed [here](rails#54736 (comment))), is a different problem that is out of scope of this patch. Ultimately, rails#54647 implementation was not broken but the solution wasn't elegant and didn't reach any consensus, so I figured let's try a new approach. ### Solution Basically the issue is that it's impossible for the Rails test command to know which arguments should be parsed and which should be proxied to minitest. And what it tries to achieve is providing some sort of CLI that minitest lacks. This implementation relies on letting minitest parse all options **and then** we load the files. Previously it was the opposite, where the rails runner would try to guess what files needs to be required, and then let minitest parse all options. This is done using a "catch-all non-flag arguments" to minitest option parser. -------------------- Now that we delegate the parsing of ARGV to minitest, I had to workaround two issues: 1) Running any test subcommand such as `bin/rails test:system` is the equivalent of `bin/rails test test/system`, but in the former, ARGV is empty and the "test/system" string is passed as a ruby parameter to the main test command. So we need to mutate ARGV. *But* mutating argv can only be done inside a `at_exit` hook, because any mutation would be rolled back when the command exists. This happens before minitest has run any test (at process exit). https://github.com/rails/rails/blob/f575fca24a72cf0631c59ed797c575392fbbc527/railties/lib/rails/command.rb#L140-L146 2) Running a test **without** the rails command: `ruby test/my_test.rb` would end up loading the `my_test.rb` file twice. First, because of `ruby`, and the because of our minitest plugin. So we need to let our plugin know whether loading file is required (e.g. did the user used the CLI).
1 parent f575fca commit b1589fa

File tree

3 files changed

+80
-5
lines changed

3 files changed

+80
-5
lines changed

railties/lib/minitest/rails_plugin.rb

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -127,6 +127,11 @@ def self.plugin_rails_options(opts, options)
127127
options[:profile] = count
128128
end
129129

130+
opts.on(/^[^-]/) do |test_file|
131+
options[:test_files] ||= []
132+
options[:test_files] << test_file
133+
end
134+
130135
options[:color] = true
131136
options[:output_inline] = true
132137
end
@@ -137,6 +142,10 @@ def self.plugin_rails_init(options)
137142
# Don't mess with Minitest unless RAILS_ENV is set
138143
return unless ENV["RAILS_ENV"] || ENV["RAILS_MINITEST_PLUGIN"]
139144

145+
if ::Rails::TestUnit::Runner.load_test_files
146+
::Rails::TestUnit::Runner.load_tests(options.fetch(:test_files, []))
147+
end
148+
140149
unless options[:full_backtrace]
141150
# Plugin can run without Rails loaded, check before filtering.
142151
if ::Rails.respond_to?(:backtrace_cleaner)

railties/lib/rails/test_unit/runner.rb

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ class Runner
2626
TEST_FOLDERS = [:models, :helpers, :channels, :controllers, :mailers, :integration, :jobs, :mailboxes]
2727
PATH_ARGUMENT_PATTERN = %r"^(?!/.+/$)[.\w]*[/\\]"
2828
mattr_reader :filters, default: []
29+
mattr_reader :load_test_files, default: false
2930

3031
class << self
3132
def attach_before_load_options(opts)
@@ -52,10 +53,14 @@ def run_from_rake(test_command, argv = [])
5253
success || exit(false)
5354
end
5455

55-
def run(argv = [])
56-
load_tests(argv)
57-
56+
def run(args = [])
5857
require "active_support/testing/autorun"
58+
59+
@@load_test_files = true
60+
61+
at_exit do
62+
ARGV.replace(args)
63+
end
5964
end
6065

6166
def load_tests(argv)
@@ -93,8 +98,6 @@ def compose_filter(runnable, filter)
9398
def extract_filters(argv)
9499
# Extract absolute and relative paths but skip -n /.*/ regexp filters.
95100
argv.filter_map do |path|
96-
next unless path_argument?(path)
97-
98101
path = path.tr("\\", "/")
99102
case
100103
when /(:\d+(-\d+)?)+$/.match?(path)

railties/test/application/test_runner_test.rb

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -222,6 +222,69 @@ def test_sanae
222222
end
223223
end
224224

225+
def test_run_test_at_root
226+
app_file "my_test.rb", <<-RUBY
227+
require "test_helper"
228+
229+
class MyTest < ActiveSupport::TestCase
230+
def test_rikka
231+
puts 'Rikka'
232+
end
233+
end
234+
RUBY
235+
236+
run_test_command("my_test.rb").tap do |output|
237+
assert_match "Rikka", output
238+
end
239+
end
240+
241+
def test_run_test_having_a_slash_in_its_name
242+
app_file "my_test.rb", <<-RUBY
243+
require "test_helper"
244+
245+
class MyTest < ActiveSupport::TestCase
246+
test "foo/foo" do
247+
puts 'Rikka'
248+
end
249+
end
250+
RUBY
251+
252+
run_test_command("my_test.rb -n foo\/foo").tap do |output|
253+
assert_match "Rikka", output
254+
end
255+
end
256+
257+
def test_run_test_with_flags_unordered
258+
app_file "my_test.rb", <<-RUBY
259+
require "test_helper"
260+
261+
class MyTest < ActiveSupport::TestCase
262+
test "foo/foo" do
263+
puts 'Rikka'
264+
end
265+
end
266+
RUBY
267+
268+
run_test_command("--seed 344 my_test.rb --fail-fast -n foo\/foo").tap do |output|
269+
assert_match "Rikka", output
270+
end
271+
end
272+
273+
def test_run_test_after_a_flag_without_argument
274+
app_file "my_test.rb", <<-RUBY
275+
require "test_helper"
276+
class MyTest < ActiveSupport::TestCase
277+
test "foo/foo" do
278+
puts 'Rikka'
279+
end
280+
end
281+
RUBY
282+
283+
run_test_command("--fail-fast my_test.rb -n foo\/foo").tap do |output|
284+
assert_match "Rikka", output
285+
end
286+
end
287+
225288
def test_run_matched_test
226289
app_file "test/unit/chu_2_koi_test.rb", <<-RUBY
227290
require "test_helper"

0 commit comments

Comments
 (0)