Modify the Test Runner to allow running test at root: #54647
Merged
+59
−5
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- ### Problem I'd like to be able to do `bin/rails my_test.rb`. But I can't, as `my_test.rb` is not considered a path by the runner, so Rails ends up running the entire test suite. This is also confusing, when I run `bin/rails test unexisting.rb` I don't get a load error, and the whole suite runs. ### Solution Don't make the assumptions about the `/` character to consider the arg as a path. Instead, skip over any arg that is preceded by a flag, and consider all others to be a path. ### Caveat (Not really one, since you could never do that.) Running a file named `--file` or `-file` is not possible. But this isn't anything new, Minitest will trip over when running the OptionParser and see that this "flag" isn't defined. ### Bonus point You can now run a test that has a slash in its name. `bin/rails test -n foo\/bar`
Edouard-chin
force-pushed
the
ec-test-root
branch
from
1e4c91f to
f4408dd
Compare
4 tasks
byroot
reviewed
Mar 8, 2025
Comment on lines
+97
to
+103
| current_arg_is_a_flag = /^-{1,2}[a-zA-Z0-9\-_.]+=?.*\Z/.match?(path) | ||
|
|
||
| if previous_arg_was_a_flag && !current_arg_is_a_flag | ||
| # Handle the case where a flag is followed by another flag (e.g. --fail-fast --seed ...) | ||
| previous_arg_was_a_flag = false | ||
| next | ||
| end |
There was a problem hiding this comment.
I get the need, but this code seems a bit fragile to me. Rather than all this extra logic, have you tried to improve path_argument? ?
I think it could ultimately use File.exist? to figure out that something is a path.
There was a problem hiding this comment.
I dug a bit more to check what less brittle alternative we can have, but I couldn't find any that would fix all current use cases and the missing ones I described in this PR.
I think it could ultimately use File.exist? to figure out that something is a path.
If we were to do this, we still would need to determine whether we need to require the file (even in the event that File.exist? returns false). If we don't, we'd just skip over any un-existing files and running bin/rails test test/unexisting_file.rb would just run the entire test suite (instead of raising a LoadError).
So ultimately we'd have the same logic from this patch where we'd have to check whether the arg is an option from a flag.
Also take in consideration a random minitest plugin that accepts an option, we'd have no way to determine whether the arg is part of an option:
bin/rails test test/test_file.rb --load-fixtures test/fixtures/foo.rb
puts ARGV #=> ['test/test_file.rb', '--load-fixtures', 'test/fixtures/foo.rb']
I think the ideal solution would be to use the OptionParser, parse all options and return only the arguments. Rafael had a similar ish use case in ruby/optparse#38, but the implemented solution doesn't fully work (it doesn't proxy short flags such as -t, and stops the parsing at the first unknown long flag).
This was referenced Mar 11, 2025
Edouard-chin
added a commit
to Edouard-chin/rails
that referenced
this pull request
Mar 12, 2025
- ### 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).
Edouard-chin
added a commit
to Edouard-chin/rails
that referenced
this pull request
Mar 12, 2025
- ### 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).
Edouard-chin
added a commit
to Edouard-chin/rails
that referenced
this pull request
Mar 12, 2025
- ### 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).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation / Background
I'd like to be able to do
bin/rails my_test.rb. But I can't, asmy_test.rbis not considered a path by the runner, so Rails ends up running the entire test suite.This is also confusing, when I run
bin/rails test unexisting.rbI don't get a load error, and the whole suite runs.Solution
Don't make the assumptions about the
/character to consider the arg as a path. Instead, skip over any arg that is preceded by a flag, and consider all others to be a path.Caveat
(Not really one, since you could never do that.)
Running a file named
--fileor-fileis not possible.But this isn't anything new, Minitest will trip over when running the OptionParser and see that this "flag" isn't defined.
Bonus point
You can now run a test that has a slash in its name.
bin/rails test -n foo\/barChecklist
Before submitting the PR make sure the following are checked:
[Fix #issue-number]