Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve Test Runner's Minitest integration. #19571

Merged
merged 1 commit into from Jun 8, 2015

Conversation

@kaspth
Copy link
Member

kaspth commented Mar 28, 2015

I've tried to improve the Rails new test running system to take better advantage of Minitest's extension system.

The build will break as I haven't ported over the Runner tests yet. But the tests running the tests commands should pass.

@zenspider, was this more inline with your comments on the original pull request?

cc @senny @arthurnn

@kaspth kaspth added the railties label Mar 28, 2015
@kaspth kaspth added this to the 5.0.0 milestone Mar 28, 2015
@senny
Copy link
Member

senny commented Apr 1, 2015

@kaspth Thank you for working on this 💛

Can we make it work without Minitest.autorun? Part of the idea of the original runner was to have a simple interface to run a bunch of tests:

Rails::TestRunner.run(["test/controllers", "test/mailers", "test/functional"])

When relying on Minitest.autorun the API feels unclear and relies on the require side-effects. There is a related issue #17708 and PR #19435.


opts.on("-n", "--name [FILENAME:LINE]",
"Run a single test by appending the line number to filename:\n\n" \
"bin/rails test test/models/user_test.rb:27") do |name|

This comment has been minimized.

Copy link
@senny

senny Apr 1, 2015

Member

I'm a bit confused by this option. Isn't -n supposed to name the method to run? Also the example does not use the flag at all. This seems to be more an explanation of the [file or directory] argument at the top.

@arthurnn
Copy link
Member

arthurnn commented Apr 1, 2015

I still think we should have a TestRunner class. maybe that class wont parse the options anymore. But it would be good to have the test runner logic in one class.

@kaspth kaspth force-pushed the kaspth:improve-runner-integration branch from c428d3e to 6f209b4 Apr 6, 2015
@kaspth
Copy link
Member Author

kaspth commented Apr 6, 2015

Alright, updated here @senny @arthurnn

@@ -14,7 +15,7 @@ def teardown
teardown_app
end

def test_run_in_test_environment
def test_run_in_differing_environments

This comment has been minimized.

Copy link
@senny

senny Apr 6, 2015

Member

@kaspth do you think it's worth keep the other test around to be sure it runs in test env by default?

This comment has been minimized.

Copy link
@kaspth

kaspth Apr 11, 2015

Author Member

I must admit the setup line length got me to consolidate the tests. I do think you're right about keeping the old method, so we get a better test name, 👍

end
end

def test_run_multiple_files_and_run_one_file_by_line_only_runs_filter

This comment has been minimized.

Copy link
@senny

senny Apr 6, 2015

Member

Is this the behavior we want or a limitation by our current integration with minitest?

This comment has been minimized.

Copy link
@arthurnn

arthurnn Apr 7, 2015

Member

this is a limitation of Minitest atm, that only allows you to pass one filter.
I have a semi-patch for Minitest to solve that. Until we send the PR and gets merged, thats the behaviour we can have.

This comment has been minimized.

Copy link
@arthurnn

arthurnn Apr 7, 2015

Member

Sorry, what I said is not true.
We can implement a filter that accept files and lines, or just files. see that Minitest uses the filter ===.
https://github.com/seattlerb/minitest/blob/master/lib/minitest.rb#L289

So our filter can hold a chain of files:line and we accept multiple results.

This comment has been minimized.

Copy link
@kaspth

kaspth Apr 11, 2015

Author Member

@arthurnn Right, I was thinking about adding a chain. Do we want to support passing several line filters?

This comment has been minimized.

Copy link
@senny

senny Apr 11, 2015

Member

@kaspth I'm not sure we need to support that. What got my attention is that these tests specify behavior that is caused by the current implementation not because we designed it like that. I think the behavior is surprising, you can pass arguments that are not at all considered. It would be better to either warn the user when that happens or get it working.

This comment has been minimized.

Copy link
@kaspth

kaspth Apr 11, 2015

Author Member

It was actually easier to just implement a FilterChain to let us run more line filters and augment Minitest's -n option.

This comment has been minimized.

Copy link
@kaspth

kaspth Apr 11, 2015

Author Member

But I just realized that doesn't solve your original concern, @senny. I'll try working on it some more.

@senny
Copy link
Member

senny commented Apr 6, 2015

@kaspth really liking this patch 💛 Can you post the help output when running with -h?

@zenspider
Copy link
Contributor

zenspider commented Apr 7, 2015

@senny it already does.


ARGV.shift if ARGV[0] == "test"

Rails::TestRunner.run

This comment has been minimized.

Copy link
@arthurnn

arthurnn Apr 7, 2015

Member

why the runner dont receive the args?
I remember I initially had the ARGV.shift too, but I think, it didnt work when you do: bundle exec rake test db:migrate , dont fully remember why.

This comment has been minimized.

Copy link
@zenspider

zenspider Apr 7, 2015

Contributor

By the time a rake task is run, ARGV is out of the picture. That's how that task got run in the first place.

This comment has been minimized.

Copy link
@kaspth

kaspth Apr 11, 2015

Author Member

@zenspider Right, so as long as rake is the funnel people will have to keep using TEST and TESTOPTS.

This comment has been minimized.

Copy link
@kaspth

kaspth Apr 11, 2015

Author Member

I guess we could modify a Rake::TestTask to run something like ruby -Ilib:test -r"rails/test_unit/minitest_plugin" whatever.rb

@@ -284,6 +354,18 @@ def test_fixture
RUBY
end

def create_backtrace_test

This comment has been minimized.

Copy link
@arthurnn

arthurnn Apr 7, 2015

Member

the output of the tests that check backtrace look the same when backtrace is enable or disabled. That looks weird IMO. We should have different outputs I think.

This comment has been minimized.

Copy link
@kaspth

kaspth Apr 11, 2015

Author Member

Right, I can add a failure message to the assert_kind_of and match on that.

end
end
end

private

This comment has been minimized.

Copy link
@arthurnn

arthurnn Apr 7, 2015

Member

should we also test the cases where tests fail, and the right output is there?

This comment has been minimized.

Copy link
@kaspth

kaspth Apr 11, 2015

Author Member

The way I see it, the happy path testing is to make sure we're correctly funneling to Minitest. Thus adding tests for more than our extensions would be testing Minitest.

@@ -45,6 +45,41 @@ def test_order

test_order
end

def run(reporter, options = {})
if filter = line_filter_for(options[:patterns] || [])

This comment has been minimized.

Copy link
@arthurnn

arthurnn Apr 7, 2015

Member

why do we need this method here, and not in the Runner?
If we had this method in the runner, we could called it from the rails_minitest_plugin.
And tests that only extend Minitest::Test would also work.

This comment has been minimized.

Copy link
@kaspth

kaspth Apr 11, 2015

Author Member

We need to check which runnable implements a test - we don't have access to the runnable in rails_minitest_plugin.

Also adding it to Runner would make Active Support depend on Railties - this way stays clear of that.

@@ -2,4 +2,4 @@

$: << File.expand_path("../../test", APP_PATH)

Rails::TestRunner.run(ARGV)
Minitest.autorun

This comment has been minimized.

Copy link
@arthurnn

arthurnn Apr 7, 2015

Member

why do we call the autorun here and not the Rails runner?

This comment has been minimized.

Copy link
@senny

senny Apr 22, 2015

Member

regarding the naming discussion TestRequirer and TestRunner. I'd rather opt for something, which does not rely on Minitest.autorun.

@kaspth do you see any reasons not to go down the path of explicitly calling Minitest.run when we want it to run? Also clearing in between runs, making it possible to run different sets of tests in the same process?

ENV["RAILS_ENV"] = options[:environment] || "test"

options[:patterns] = OptionParser.new(options[:args]).order!
Rails::TestRunner.require_files options[:patterns]

This comment has been minimized.

Copy link
@arthurnn

arthurnn Apr 7, 2015

Member

I just realized we have this require call here, so do we need the TestRunner.run method at all?

@arthurnn
Copy link
Member

arthurnn commented Apr 7, 2015

I like where this is going too.. Thanks @kaspth

@senny
Copy link
Member

senny commented Apr 7, 2015

@zenspider I meant to post the output here to GitHub.

@zenspider
Copy link
Contributor

zenspider commented Apr 7, 2015

"Runner" == manager class. That's what MT4 had and why I explicitly did a rewrite for MT5. Manager classes are bad design and should be avoided where possible. I explicitly designed test classes (runnables, really) to be able to run themselves on purpose. This allows for different types of runnables (eg Test vs Benchmark). Having a test class that knows how to run its own tests makes sense. It's what tests do: run, so why externalize the functionality of it?

Avoid a "Runner" type class. You'll be better off in the long run.

@zenspider
Copy link
Contributor

zenspider commented Apr 7, 2015

@kaspth Sorry, I missed your original question: you've removed 75 lines, of course I'm a fan.

@senny
Copy link
Member

senny commented Apr 7, 2015

@zenspider I can see your Runner-argument for Minitest itself but this runner does nothing else than requiring the files. Someone needs to require the tests before they can run themselves. I'd like to have an interface in Rails to run a set of files and folders. The rest boils down to wether Runner is a good name for that.

@arthurnn arthurnn self-assigned this Apr 8, 2015
@kaspth
Copy link
Member Author

kaspth commented Apr 11, 2015

@zenspider Great to hear and, yes, I agree on removing the need for a runner and just use Minitest's autorun or run where appropriate.

@senny I'd be happy to go back to the TestRequirer name instead of TestRunner.

@kaspth kaspth force-pushed the kaspth:improve-runner-integration branch from 6f209b4 to 71aa510 Apr 11, 2015

private
def derive_regexp(filter)
filter =~ %r%/(.*)/% ? Regexp.new($1) : filter

This comment has been minimized.

Copy link
@kaspth

kaspth Apr 11, 2015

Author Member

If we didn't duplicate some logic from Minitest here, passing a -n /hello/ filter won't filter correctly.

@kaspth
Copy link
Member Author

kaspth commented Apr 11, 2015

@zenspider Writing the FilterChain is a bit cumbersome because we kind of have to hack Minitest. Would you consider letting others extend Minitest's filter in the same way reporters can?

@kaspth
Copy link
Member Author

kaspth commented Apr 12, 2015

@senny I forgot to post the help output. Here it is:

minitest options:
    -h, --help                        Display this help.
    -s, --seed SEED                   Sets random seed
    -v, --verbose                     Verbose. Show progress processing files.
    -n, --name PATTERN                Filter run on /pattern/ or string.

Known extensions: rails
Usage: bin/rails test [options] [file or directory]
You can run a single test by appending the line number to filename:

bin/rails test test/models/user_test.rb:27
    -e, --environment [ENV]           Run tests in the ENV environment
    -b, --backtrace                   Show the complete backtrace

We should probably add more separators - currently it looks a bit cramped.

@senny
Copy link
Member

senny commented Apr 13, 2015

@kaspth I agree, the output is pretty dense at the moment. Also, the usage (Usage: bin/rails test [options] [file or directory]) needs adjusting that it's possible to pass multiple files or directories:

@kaspth
Copy link
Member Author

kaspth commented Apr 13, 2015

@senny I've tweaked the output to this:

minitest options:
    -h, --help                       Display this help.
    -s, --seed SEED                  Sets random seed
    -v, --verbose                    Verbose. Show progress processing files.
    -n, --name PATTERN               Filter run on /pattern/ or string.

Known extensions: rails

Usage: bin/rails test [options] [files or directories]
You can run a single test in file by appending the line number:

    bin/rails test test/models/user_test.rb:27

Rails options:
    -e, --environment [ENV]          Run tests in the ENV environment
    -b, --backtrace                  Show the complete backtrace
@senny
Copy link
Member

senny commented Jun 2, 2015

@kaspth if you haven't already, can you add a test-case with a command that results in no runnables? Just to make sure we don't run into this problem again.

@kaspth
Copy link
Member Author

kaspth commented Jun 2, 2015

@senny Definitely. I'll add one now 👍

@kaspth
Copy link
Member Author

kaspth commented Jun 2, 2015

@senny I can't figure out how to write a failing test for this. Rails is loaded at this point so it'll respond to backtrace_cleaner just fine.

def test_run_app_without_tests
  Dir.chdir(app_path) do
    `rm -rf test`
    `bin/rails test`.tap do |output|
      assert_match '0 runs, 0 assertions', output
    end
  end
end

I guess the test name really should be something like: "run app without rails loaded".

@kaspth kaspth force-pushed the kaspth:improve-runner-integration branch from be32121 to 1836dc6 Jun 2, 2015
@zenspider
Copy link
Contributor

zenspider commented Jun 2, 2015

@kaspth That's because it is hard to follow an idiot making an idiotic comment. :) I've deleted my comment since I entirely missed the point. I'm blaming a lack of caffeine.

@kaspth
Copy link
Member Author

kaspth commented Jun 3, 2015

@zenspider haha, alright then 😄

@kaspth
Copy link
Member Author

kaspth commented Jun 3, 2015

@senny I found out why the test doesn't reproduce the issue. Here's boot.rb from the generated app in the test:

require '/Users/kasperhansen/Documents/code/rails/load_paths'
require 'rails/all'

And a real app:

ENV['BUNDLE_GEMFILE'] ||= File.expand_path('../../Gemfile', __FILE__)

require 'bundler/setup' # Set up gems listed in the Gemfile.

Not sure if we should just override that file or if there's a better way to simulate no runnables where Rails isn't loaded.

@senny
Copy link
Member

senny commented Jun 4, 2015

@kaspth let's start by overwriting the file. If someone has a more elegant idea we can change it later on.

/cc @rafaelfranca

@rafaelfranca
Copy link
Member

rafaelfranca commented Jun 4, 2015

Yeah, I think overwriting is good.

@kaspth kaspth force-pushed the kaspth:improve-runner-integration branch from 1836dc6 to e8ad8e1 Jun 4, 2015
@kaspth
Copy link
Member Author

kaspth commented Jun 4, 2015

@senny @rafaelfranca I've pushed the change overwriting the file.

def test_env
puts Rails.env
class PostTest < ActiveSupport::TestCase
test 'declarative syntax works' do

This comment has been minimized.

Copy link
@kaspth

kaspth Jun 4, 2015

Author Member

This test would always pass because we were using def test_the_truth, which didn't cause the bug @arthurnn experienced with the RuntimeError saying "already defined in PostTest."

I'm not sure how to make this clearer.

end
end
RUBY

assert_match env, run_test_command("-e #{env} test/unit/env_test.rb")
Dir.chdir(app_path) do
`ruby -Itest test/models/post_test.rb`.tap do |output|

This comment has been minimized.

Copy link
@kaspth

kaspth Jun 4, 2015

Author Member

Also if we ever regress here RuntimeError just goes straight through and is spit out in the log. I've tried catching it, but I guess the fact that we're in another process(?) throws us off. The tests will be marked as a failure, but we'll never hit the assertions below.

This also adds free mix and matching of directories, files and lines filters.
Like so:

bin/rails test models/post_test.rb test/integration models/person_test.rb:26

You can also mix in a traditional Minitest filter:

bin/rails test test/integration -n /check_it_out/
@kaspth kaspth force-pushed the kaspth:improve-runner-integration branch from e8ad8e1 to b6fc8e2 Jun 4, 2015
@senny
Copy link
Member

senny commented Jun 8, 2015

Just gave the latest version of this PR a spin and everything worked. (Even did some spring testing). Help text is descriptive and the new integration is superb. Thank you @kaspth for your work! 💛 💛 💛

@senny senny merged commit b6fc8e2 into rails:master Jun 8, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
senny added a commit that referenced this pull request Jun 8, 2015
Improve Test Runner's Minitest integration.
@kaspth kaspth deleted the kaspth:improve-runner-integration branch Jun 8, 2015
@kaspth
Copy link
Member Author

kaspth commented Jun 8, 2015

Woot! So glad to have this merged ❤️💛💜💚💙

@arthurnn
Copy link
Member

arthurnn commented Jun 8, 2015

NICE JOB!!!!

@deivid-rodriguez
Copy link
Contributor

deivid-rodriguez commented Jun 8, 2015

@kaspth Thanks a lot for this, it's a great improvement! 😄

senny added a commit to senny/rails that referenced this pull request Jun 8, 2015
This adds a script `bin/test` to most Rails framework components. The
script uses the rails minitest plugin to augment the runner.
See rails#19571 for details about the
plugin.

I did not yet add `bin/test` for activerecord, activejob and railties.
These components rely on specific setup performed in the rake-tasks.
senny added a commit to senny/rails that referenced this pull request Jun 11, 2015
This adds a script `bin/test` to most Rails framework components. The
script uses the rails minitest plugin to augment the runner.
See rails#19571 for details about the
plugin.

I did not yet add `bin/test` for activerecord, activejob and railties.
These components rely on specific setup performed in the rake-tasks.
@rafaelfranca rafaelfranca modified the milestones: 5.0.0 [temp], 5.0.0 Dec 30, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

9 participants
You can’t perform that action at this time.