Collect all file patterns when running multiple rake test tasks #27802

domcleal · 2017-01-25T16:03:02Z

rails-bot · 2017-01-25T16:03:06Z

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @chancancode (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

arthurnn · 2017-01-31T20:14:57Z

Thanks @domcleal for the bug fix.. Do you mind adding a changelog entry for this fix, and rebasing your PR against latest master?

kaspth · 2017-01-31T20:27:23Z

railties/lib/rails/test_unit/minitest_plugin.rb

@@ -60,7 +60,7 @@ def self.plugin_rails_options(opts, options)
  # Running several Rake tasks in a single command would trip up the runner,
  # as the patterns would also contain the other Rake tasks.
  def self.rake_run(patterns) # :nodoc:
-    @rake_patterns = patterns
+    (@rake_patterns ||= []).push(*patterns)


Won't this negate what the comment says above? In other words, do we need rake_run anymore? It might have become obsolete after moving to autorun.

The comment is still valid with @rake_patterns - it's targeted at fixing rake db:migrate test rather than two test tasks. Without it, the plugin would run tests listed in the argument list, which is ['db:migrate' ,'test'] - instead rake_run stores the paths from test:* tasks so running test + non-test tasks works.

This was originally added in df744b5, but missing a test to cover it and the conditional above, so I added one in b5a9428.

I think you're right that rake_run (or rather @rake_patterns) is a bit obsolete now that autorun is used. The test files can be required immediately and autorun will handle it. I've removed @rake_patterns in a12803d, but kept rake_run as it makes the tasks in railties/lib/rails/test_unit/testing.rake a little less repetitive.

domcleal · 2017-02-01T10:10:21Z

@arthurnn thanks, I've rebased the PR, added two commits in response to @kaspth's comment (improve test coverage, refactor away this instance variable) and added the CHANGELOG entry.

Edit: I think the actioncable test failure is unrelated, possibly a transient or intermittent issue.

arthurnn · 2017-02-01T18:48:42Z

Aren't we going to require_files twice now? https://github.com/domcleal/rails/blob/a12803dc17417437ddbbe93bc798d6b779363331/railties/lib/rails/test_unit/minitest_plugin.rb#L90

domcleal · 2017-02-02T10:57:18Z

@arthurnn thanks, you're right that it was called twice. options[:pattern] would be empty though for rake runs, but I've updated the last commit to skip the step when called via rake.

kaspth · 2017-02-02T19:28:35Z

railties/test/application/test_runner_test.rb

+
+    def test_rake_db_and_test_tasks_parses_args_correctly
+      create_test_file :models, "account"
+      output = Dir.chdir(app_path) { `bin/rake db:migrate test:models TESTOPTS='-v' && echo ".tables" | rails dbconsole` }


Won't the piping into dbconsole make the test stall?

The test doesn't hang if that's what you mean, but it may take a bit longer to run. I wanted to check that the db:migrate task ran too, and this is the method used by previous tests.

kaspth · 2017-02-02T19:30:06Z

railties/test/application/test_runner_test.rb

+      create_test_file :models, "account"
+      output = Dir.chdir(app_path) { `bin/rake db:migrate test:models TESTOPTS='-v' && echo ".tables" | rails dbconsole` }
+      assert_match "AccountTest#test_truth", output, "passing test:models should run test/models/"
+      assert_match "ar_internal_metadata", output, "tables should be dumped"


Let's kill the custom messages on assert_match here and above. Think the intention is sufficiently clear in the test code itself.

kaspth · 2017-02-02T19:30:46Z

railties/lib/rails/test_unit/minitest_plugin.rb

-    # If run via `ruby` we've been passed the files to run directly.
-    unless run_via[:ruby]
+    # If run via `ruby` we've been passed the files to run directly, or if run
+    # via `rake` then they have already been included.


included is a new term, let's say eagerly required to stick with the established terminology.

kaspth · 2017-02-02T19:31:45Z

railties/lib/rails/test_unit/minitest_plugin.rb

  def self.rake_run(patterns) # :nodoc:
-    @rake_patterns = patterns
+    ::Rails::TestRequirer.require_files patterns
+    run_via[:rake] = true


Think it makes more sense to assign run_via as the first step and then require the files.

kaspth · 2017-02-02T19:32:07Z

railties/lib/rails/test_unit/minitest_plugin.rb

  def self.rake_run(patterns) # :nodoc:
-    @rake_patterns = patterns
+    ::Rails::TestRequirer.require_files patterns


Add parens around here: ::Rails::TestRequirer.require_files(patterns)

kaspth · 2017-02-02T19:33:56Z

railties/CHANGELOG.md

@@ -1,3 +1,7 @@
+*   Fix running multiple test\* tasks specified in a single `rake` command


Don't understand what test\* is supposed to mean.

I'd write:

* Fix running multiple tests in one `rake` command. E.g. `bin/rake test:models test:controllers` # …

kaspth · 2017-02-02T19:38:20Z

railties/test/application/test_runner_test.rb

+    def test_rake_runs_multiple_test_tasks
+      create_test_file :models, "account"
+      create_test_file :controllers, "accountcontroller"
+      output = Dir.chdir(app_path) { `bin/rake test:models test:controllers TESTOPTS='-v'` }


Why do we pass in TESTOPTS here and below?

It prints the names of the tests, so it can assert the expected tests ran using output.

kaspth · 2017-02-02T19:38:58Z

railties/test/application/test_runner_test.rb

@@ -536,6 +536,21 @@ def test_rake_passes_multiple_TESTOPTS_to_minitest
      assert_match "seed=1234", output, "passing TEST= should run selected test"
    end

+    def test_rake_runs_multiple_test_tasks
+      create_test_file :models, "account"
+      create_test_file :controllers, "accountcontroller"


Let's follow Rails' naming conventions for good measure: "accounts_controller" :)

kaspth · 2017-02-02T19:40:07Z

Squash your commits down to 1 once the latest review has been addressed. Thanks for fixing this, digging it! 🤘

Replaces the rake_patterns instance variable with simple require, as `autorun` will run tests from all eagerly required test files. Fixes rails#27801

domcleal · 2017-02-04T13:00:02Z

Thank you for the detailed review and comments @kaspth. I've fixed the style comments and squashed the commits.

kaspth · 2017-02-05T16:03:04Z

Awesome, thanks!

domcleal · 2017-02-08T14:29:46Z

Thanks @kaspth. Would you mind backporting it to 5-0-stable, since it's a regression vs 4.2?

Collect all file patterns when running multiple rake test tasks

kaspth · 2017-02-20T21:33:22Z

Apologies for not keeping you in the loop, I needed #27941 to be merged first.

Backported to 5-0-stable at e79ffaf...88b335e

domcleal · 2017-02-21T08:06:29Z

No worries, thanks @kaspth.

kaspth · 2017-02-21T08:46:22Z

Sounds good 👍

rails-bot assigned chancancode Jan 25, 2017

maclover7 added the railties label Jan 26, 2017

domcleal force-pushed the 27801-multiple-rake-tests branch from f42effb to 065b5ca Compare January 26, 2017 12:10

arthurnn assigned arthurnn and unassigned chancancode Jan 31, 2017

kaspth reviewed Jan 31, 2017

View reviewed changes

maclover7 added the needs work label Jan 31, 2017

domcleal force-pushed the 27801-multiple-rake-tests branch from 065b5ca to a12803d Compare February 1, 2017 09:50

domcleal force-pushed the 27801-multiple-rake-tests branch from a12803d to d936d9f Compare February 2, 2017 10:56

kaspth reviewed Feb 2, 2017

View reviewed changes

Collect all file patterns when running multiple rake test tasks

1c8a4cd

Replaces the rake_patterns instance variable with simple require, as `autorun` will run tests from all eagerly required test files. Fixes rails#27801

domcleal force-pushed the 27801-multiple-rake-tests branch from d936d9f to 1c8a4cd Compare February 4, 2017 12:56

kaspth merged commit 2cb6c27 into rails:master Feb 5, 2017

maclover7 removed the needs work label Feb 5, 2017

domcleal mentioned this pull request Feb 17, 2017

Collect all file patterns when running multiple rake test tasks #28048

Closed

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

Merge pull request #27802 from domcleal/27801-multiple-rake-tests

9b5952c

Collect all file patterns when running multiple rake test tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Collect all file patterns when running multiple rake test tasks #27802

Collect all file patterns when running multiple rake test tasks #27802

domcleal commented Jan 25, 2017

rails-bot commented Jan 25, 2017

arthurnn commented Jan 31, 2017

kaspth Jan 31, 2017

domcleal Feb 1, 2017

domcleal commented Feb 1, 2017 •

edited

arthurnn commented Feb 1, 2017

domcleal commented Feb 2, 2017

kaspth Feb 2, 2017

domcleal Feb 4, 2017

kaspth Feb 2, 2017

kaspth Feb 2, 2017

kaspth Feb 2, 2017

kaspth Feb 2, 2017

kaspth Feb 2, 2017

kaspth Feb 2, 2017

domcleal Feb 4, 2017

kaspth Feb 2, 2017

kaspth commented Feb 2, 2017

domcleal commented Feb 4, 2017

kaspth commented Feb 5, 2017

domcleal commented Feb 8, 2017

kaspth commented Feb 20, 2017

domcleal commented Feb 21, 2017

kaspth commented Feb 21, 2017

		@@ -1,3 +1,7 @@
		* Fix running multiple test\* tasks specified in a single `rake` command

Collect all file patterns when running multiple rake test tasks #27802

Collect all file patterns when running multiple rake test tasks #27802

Conversation

domcleal commented Jan 25, 2017

rails-bot commented Jan 25, 2017

arthurnn commented Jan 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domcleal commented Feb 1, 2017 • edited

arthurnn commented Feb 1, 2017

domcleal commented Feb 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaspth commented Feb 2, 2017

domcleal commented Feb 4, 2017

kaspth commented Feb 5, 2017

domcleal commented Feb 8, 2017

kaspth commented Feb 20, 2017

domcleal commented Feb 21, 2017

kaspth commented Feb 21, 2017

domcleal commented Feb 1, 2017 •

edited