-
Edouard CHIN authored
- ### Problem See #54647 for the original reason this needs to be fixed. ### Context #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). #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](https://github.com/rails/rails/pull/54736#issuecomment-2716156252)), is a different problem that is out of scope of this patch. Ultimately, #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 authored- ### Problem See #54647 for the original reason this needs to be fixed. ### Context #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). #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](https://github.com/rails/rails/pull/54736#issuecomment-2716156252)), is a different problem that is out of scope of this patch. Ultimately, #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).
Loading