Skip to content
  • Edouard CHIN's avatar
    b1589fa7
    Let minitest parse ARGV: · b1589fa7
    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).
    b1589fa7
    Let minitest parse ARGV:
    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