Add `rails test` command to run the test suite #9080

Merged
merged 7 commits into from Mar 11, 2013

Projects

None yet
@sikachu
Member
sikachu commented Jan 25, 2013

To run the whole test suite:

$ rails test

To run the test file(s):

$ rails test test/unit/foo_test.rb [test/unit/bar_test.rb ...]

To run the test suite

$ rails test [models,helpers,units,controllers,mailers,...]

For more information, see rails test --help.

This command will eventually replacing rake test:*, and rake test
command will actually invoking rails test instead.


After this got merged, there'll be several things that need to be done:

  • Update guides to cover this new feature.
  • Decide whether we should show a deprecation warning if user run rake test:* (but if you run rake or rake test, there will no deprecation warning.
  • Add a notification for test:prepare event, so that we can drop Rake::Task['test:prepare'].invoke line.
  • Add back uncommitted and recent suites.
  • Add support for -n to specify the test name.
@sikachu
Member
sikachu commented Jan 25, 2013

/cc @dhh

@rafaelfranca
Member

Seems good. I think we should update the guides in the scope of this pull request. Unless you think it will need a lot of work.

@sikachu
Member
sikachu commented Jan 25, 2013

Yeah, I'm working on the guide right now. Going to submit that as a separate commit, just in case if someone want to merge this in first.

@sikachu
Member
sikachu commented Jan 25, 2013

Documentation updated.

Also, add some more stuff to my todo list.

@carlosantoniodasilva carlosantoniodasilva and 1 other commented on an outdated diff Jan 26, 2013
guides/source/testing.md
@@ -755,29 +758,34 @@ end
Rake Tasks for Running your Tests
---------------------------------
-You don't need to set up and run your tests by hand on a test-by-test basis. Rails comes with a number of rake tasks to help in testing. The table below lists all rake tasks that come along in the default Rakefile when you initiate a Rails project.
+You don't need to set up and run your tests by hand on a test-by-test basis. Rails comes with a number of commands to help in testing. The table below lists all commands that come along in the default Rakefile when you initiate a Rails project.
+
+| Tasks | Description |
+| ------------------------------- | ----------- |
@carlosantoniodasilva
carlosantoniodasilva Jan 26, 2013 Member

Different table indent.

@sikachu
sikachu Jan 26, 2013 Member

nice catch. fixed!

@carlosantoniodasilva carlosantoniodasilva and 1 other commented on an outdated diff Jan 26, 2013
railties/lib/rails/commands/test_runner.rb
+ puts "-------------------------------------------------------------"
+ end
+ end
+
+ # Create a new +TestRunner+ object with a list of test file paths.
+ def initialize(files)
+ @files = files
+ Rake::Task['test:prepare'].invoke
+ MiniTest::Unit.output = SilentUntilSyncStream.new(MiniTest::Unit.output)
+ end
+
+ # Run the test files by evaluate each of them.
+ def run
+ @files.each do |filename|
+ $0 = filename
+ eval(File.read(filename), nil, filename)
@carlosantoniodasilva
carlosantoniodasilva Jan 26, 2013 Member

Wouldn't load work?

@sikachu
sikachu Jan 26, 2013 Member

Yeah, it does, so I'm going to fix it. I totally forgot about Kernel.load since I was copying the code from rails runner 😄

@carlosantoniodasilva

Cool stuff @sikachu 👍

@mdespuits
Contributor

@sikachu Awesome!

@sikachu
Member
sikachu commented Jan 26, 2013

Code updated based on @carlosantoniodasilva's review. Thanks!

@xiejava
xiejava commented Jan 26, 2013

good job

@calebthompson
Contributor

Is this going to be smart enough to run tests/specs for the appropriate test framework for those of us who don't use MiniTest?

@sikachu
Member
sikachu commented Jan 26, 2013

@calebthompson no, not for now. We might provide a hook later, but it's not currently in our radar.

Actually, we were thinking about making ./bin/test to run the test (which would be closely to ./bin/rspec) but then it would conflict with /usr/bin/test.

@calebthompson
Contributor

I feel like this is just going to be cumbersome unless/until rspec and cucumber specifically pick it up. You'll end up with two ways of running tests, and frankly the gems that people prefer to test with are going to win out over Rails conventions here I think.

@calebthompson
Contributor

With rake, rspec, or cucumber commands being the way people run their tests, over anything rails.

@sikachu
Member
sikachu commented Jan 26, 2013

Right. I understand what you mean, and I like that.

However, the goal of this ticket was to fix the slow rake test runner. I think we achieve that. From now on, though, only the sky is the limit. :)

@alindeman
Contributor

I feel like we do need some way to hook into this. On many projects, I use different frameworks for unit tests, acceptance tests, and JavaScript tests (e.g., rspec/cucumber/jasmine or minitest/cucumber/jasmine). rake test can run them all, and it's a command that everyone expects to run all the tests. If rails test becomes the new standard, but can't be used for the same situations, I expect things to be more confusing for newcomers and experienced folks alike.

@dhh
Member
dhh commented Jan 26, 2013

Please Do Investigate.

On Jan 26, 2013, at 19:31, Andy Lindeman notifications@github.com wrote:

I feel like we do need some way to hook into this. On many projects, I use different frameworks for unit tests, acceptance tests, and JavaScript tests (e.g., rspec/cucumber/jasmine or minitest/cucumber/jasmine). rake test can run them all, and it's a command that everyone expects to run all the tests. If rails test becomes the new standard, but can't be used for the same situations, I expect things to be more confusing for newcomers and experienced folks alike.


Reply to this email directly or view it on GitHub.

@josevalim
Member

I really don't see the motivation for having rails test running cucumber, rspec and what not. How would it even pick which one to run when I am using both rspec and cucumber? Who uses rake test to run specs today anyway?

Also, given that rspec accepts command line options completely different than rails test would, I just see this bringing more confusion.

@josevalim
Member

For last, why would you bind your test framework runner to Rails when there isn't a need in the first place? If you guys are having extra free-time, we still have ~400 issues open that would love some attention. 😉 😉

@drbrain
drbrain commented Jan 26, 2013

the goal of this ticket was to fix the slow rake test runner

What work was done to make the rake test runner faster? This appears to be a complete replacement that doesn't use or touch rake at all.

Since this is a replacement for existing working (if slow) code I would like to see evidence of why it is pointless to try to make rake or the rails test running tasks faster.

Could it be something I can improve in rake?

@sikachu
Member
sikachu commented Jan 26, 2013

@drbrain the original problem was that running test via rake had to load the Rails environment twice. (One for rake's environment, then rake would call ruby -Itest ... which is the second.) We kinda want to eliminate that by just load the test files into the runner; hence the usage of load command. On a rainy day, that could mean the test took twice the load time than running the test directly via some command.

@steveklabnik
Member

@drbrain @sikachu

@myronmarston seems to think it's a '(mis?)-use' of Rake that's causing the problems:

https://twitter.com/myronmarston/status/295296519203065856

https://twitter.com/myronmarston/status/295296827350200320

Myron, how should we be improving this?

@myronmarston
Contributor

Myron, how should we be improving this?

Let me preface my suggestions with two quick comments:

  • Calling rails' use of rake a "misuse" has more to do with twitter-imposed terseness than with the actual way I would categorize it. I do think rails' use of rake is suboptimal and has room for improvement, but that's more nuanced than fits in a tweet.
  • I haven't worked on a rails app in a long time (I've never used rails 3.1 or 3.2, for example, and barely used rails 3.0). The apps I've been working on are very, very far from rails-style CRUD apps, and, as @dhh has stated elsewhere, the more your app is dissimilar from Basecamp, the worse fit rails is for your application. I'm not sure if/when I'll be working on another rails app, so I'm not going to invest lots of time in an extended discussion here.

With that said, I think there's an optimal way to use rake that's quite different from how rails uses it. Specifically, it leverages the fact that rake, like most ruby programs, has two run-time phases:

  • Phase 1 is "task definition time" -- during this phase, rake is evaluating your Rakefile (which involves loading other files required or loaded by your Rakefile).
  • Phase 2 is "task execution time" -- during this phase, rake is executing the specified task(s).

My preferred way to use rake is to make phase 1 as lightweight and minimal as possible. My rule of thumb is to only require files that define rake tasks during this phase, and to defer all other requires to phase 2. Each task becomes responsible (either through the code directly in the task, or via a prerequisite task) for loading the files and libraries it needs to run. This goes hand-in-hand with the way I use bundler, and fits right in with my preferred approach of explicitly loading dependencies at runtime...but I know that DHH prefers the convenience of not having to manually manage dependencies at runtime. (As usual, this is about trade offs. The particular things I value most differ from the things DHH values most and that's fine).

Rails has, for as long as I know, chosen to load the entire rails environment at task definition time, which essentially causes the environment to be double-booted when running a task that spawns a new process that boots the environment. Changing to my preferred approach would be a large change, but if there's desire to move in that direction, a major release (like rails 4) is the right time to do it. As a point of comparison, here's the kind of speed I get out of rake. This is on a large app that I (and an entire team at @seomoz) have been working on full time for 9+ months:

$ time bin/rake -T > /dev/null
bin/rake -T > /dev/null  0.21s user 0.04s system 98% cpu 0.255 total

That's 255 ms wall clock time, which is barely noticeable.

As for the idea of having a more full-featured test runner...I heartily support such a thing (one of my favorite parts of rspec is the super flexible test runner, complete with the binary that supports a plethora of options), but I wonder if it's better suited as an external gem that can be used by any T::U/minitest project? Is there anything about this that needs to be coupled to rails?

@steveklabnik
Member

Calling rails use of rake a "misuse" has more to do with twitter-imposed terseness than with the actual way I would categorize it. I do think rails' use of rake is suboptimal and has room for improvement, but that's more nuanced than fits in a tweet.

Yes, that's why I wanted to make sure you came here, and why I made sure to copy the tweet and the (mis?) part of it. The ? says to me that you weren't trying to make a super big statement, just Twitter. Since you are on the rspec team, I figured that your insight here is important, along with @alindeman's.

@myronmarston
Contributor

Yes, that's why I wanted to make sure you came here, and why I made sure to copy the tweet and the (mis?) part of it. The ? says to me that you weren't trying to make a super big statement, just Twitter.

Thanks...it's nice to know my tweet was interpreted in the spirit it was intended :).

@dhh
Member
dhh commented Jan 28, 2013

Prem, before we apply this, please get -n and fixture loading implemented. I don't think it's a full replacement before that happens.

@sikachu
Member
sikachu commented Feb 1, 2013

@dhh -n get added to this feature. I'm not sure what you mean by "fixture loading", since I thought Rails doesn't load fixture by default unless you run rake db:fixtures:load ?

@dhh
Member
dhh commented Feb 2, 2013

Rails loads fixtures by default when you run the rake suite tasks. So "rake test" and "rake test:units" will load fixtures. This should do the same. But don't load fixtures when running one specific file or test within that file. In that case, we should have an option like --load-fixtures/-f that does it.

On Feb 1, 2013, at 23:37, Prem Sichanugrist notifications@github.com wrote:

@dhh -n get added to this feature. I'm not sure what you mean by "fixture loading", since I thought Rails doesn't load fixture by default unless you run rake db:fixtures:load ?


Reply to this email directly or view it on GitHub.

@sikachu
Member
sikachu commented Feb 6, 2013

Code update with not loading fixture by default. @dhh can you give it another look?

@dhh
Member
dhh commented Feb 6, 2013

Looks good to me. I think this is good enough to merge and start using.

I say we deprecate the "rake test" tasks. No reason to have two ways to do this.

Also, we should pull in the test:prepare stuff so it doesn't go through Rake.

@dalibor dalibor commented on an outdated diff Feb 7, 2013
railties/CHANGELOG.md
@@ -1,11 +1,40 @@
## Rails 4.0.0 (unreleased) ##
+* Rails now generate a `test/test_helper.rb` file with `fixtures :all` commented out by default,
+ since we don't want to force loading all fixtures for user when a single test is run. However,
+ fixtures are still going to be loaded automatically for test suites.
+
+ To force all fixtures to be create in your database, use `rails test -f` to run your test.
@dalibor
dalibor Feb 7, 2013 Contributor

%s/create/created/g

@sikachu
Member
sikachu commented Feb 15, 2013

Hmm, I just realized that 0ba5229 breaks backward compatibility. I need to fix that before adding the deprecation warning.

@jonleighton jonleighton referenced this pull request in rails/spring Feb 22, 2013
Closed

Consider renaming the `spring test` command #64

@sikachu
Member
sikachu commented Feb 25, 2013

Code updated. Backward compatibility fixed. Now the only thing I need is to use AS::Notification ...

@rwjblue rwjblue referenced this pull request in rails/spring Feb 26, 2013
Closed

Issues with testing gems #83

@sikachu
Member
sikachu commented Mar 8, 2013

After I've been trying to convert database preparing step to use AS::Notification, it seems like it's not worth doing it anymore. I think it's fine that we do Rake::Task['db:test:prepare'] for now.

The reason is that all the tasks in https://github.com/rails/rails/blob/master/activerecord/lib/active_record/railties/databases.rake has to be converted into a new class, with tests, so that it can be called by a new subscriber and rake task without having to duplicate the code.

All it's left now is that @rubys is seeing some failing test case after using rails test. I'm looking into that right now.

Prem Sichanu... and others added some commits Jan 25, 2013
@sikachu Prem Sichanugrist and Chris Toomey Add `rails test` command to run the test suite
To run the whole test suite:

    $ rails test

To run the test file(s):

    $ rails test test/unit/foo_test.rb [test/unit/bar_test.rb ...]

To run the test suite

    $ rails test [models,helpers,units,controllers,mailers,...]

For more information, see `rails test --help`.

This command will eventually replacing `rake test:*`, and `rake test`
command will actually invoking `rails test` instead.
b4df253
@sikachu sikachu Update testing documentation
* Update test invocation to use `rails test` instead.
* Update all the test command previews (since we're now using MiniTest.)
* Mentioning MiniTest instead of Test::Unit.
* Update list of test suites.
9f75f77
@sikachu sikachu Add support for MiniTest flags in TestRunner
Any flags that got set will be passed through to MiniTest::Unit.runner,
such as `-n`, `-s-, and `-v`.
176b57c
@sikachu sikachu Load fixtures only when running suites, or `-f`
* `rails test -f` will run the test suites with all fixtures loaded
* New application will now generated without `fixtures :all` line
  enabled by default.
1a0c58b
@dalibor @sikachu dalibor Improve wording for rails test command df85dfa
@sikachu
Member
sikachu commented Mar 9, 2013

Ok, this should be ready to go. @dhh, and @rubys can you give it another look?

@rubys
Contributor
rubys commented Mar 9, 2013

I immediately get a failure with a new project. Reproduction instructions:

$ rails new depot
$ cd depot
$ rails generate scaffold Product title
$ rake db:migrate
$ rails test
You have 1 pending migrations:
  20130309000009 CreateUsers
Run `rake db:migrate` to update your database then try again.

Needless to say, running rake db:migrate again doesn't clear up the problem.

@sikachu
Member
sikachu commented Mar 9, 2013

@rubys found it. It seems like I was trying to run db:abort_if_pending_migrations in the test environment, which failed terribly. Can you give it another try?

@rubys
Contributor
rubys commented Mar 9, 2013

Looks good. I'll have to update all of my tests as the output of the test command changed -- not an issue you need to worry about.

One think I did note is that commands like "rake test:controllers" will inform you that these commands are deprecated, but will not actually run the correct command.

Timings of rake test before:

real    0m11.942s
user    0m10.645s
sys 0m0.648s

Timings of rails test after:

real    0m5.279s
user    0m4.404s
sys 0m0.240s
sikachu added some commits Feb 25, 2013
@sikachu sikachu Update Rake tasks to call `rails test` instead
Also, print out deprecation warning for other rake tasks except
`rake test` and `rake` (default)
b51673f
@sikachu sikachu Make sure that `rails test` load test in test env 3ed41e5
@sikachu
Member
sikachu commented Mar 9, 2013

@rubys good catch. Thank you. It was actually running your test, but it was swallowing the output (I was doing backticks instead of exec())

Code updated (again) to fix that issue.

@rubys
Contributor
rubys commented Mar 10, 2013

I noticed two more things.

What appears to be going on is that I used to be able to run individual tests, but now when I try to do so, the helper methods that provide access to fixture data no longer is available. For example, in my cart_test.rb, I make reference to products(:one). This works from "rails test", but not from "rails test test/models/cart_test.rb".

The other thing I noticed is that "rails test " only supports one filename. Ideally it would support multiple. At a minimum it should warn when additional arguments are passed.

@rubys
Contributor
rubys commented Mar 10, 2013

I tracked down the problem to sikachu@1a0c58b#L2L9

In the process, I discovered the -f parameter. Either uncommenting that one line or adding the -f parameter makes this work. So the question becomes: why is -f required? And why is that fixtures :all line commented out? Is there ever a case where people will want to uncomment that line?

@sikachu
Member
sikachu commented Mar 11, 2013

@rubys see comment above from @dhh. We decided not to load all the fixtures by default if you run single test. I think the intention is that you will want to load only fixtures you want explicitly in the test file.

However, if you run the whole suite, rails test will load the fixtures for you like it used to be.

Regarding the support for only one file name: is that really the case? I think I have a test case to cover running multiple files. Can you re-confirming me on that one?

@rubys
Contributor
rubys commented Mar 11, 2013

My bad. When I specified two files, I neglected to notice that the second file had zero tests in it.

:shipit:

@rafaelfranca rafaelfranca merged commit 90a9715 into rails:master Mar 11, 2013
@sikachu sikachu deleted the sikachu:master-rails-test branch Mar 11, 2013
@spastorino

@sikachu why are you using bundle exec? I think there's no need to do so

Member

because I thought I have to use it? haha ..

Sure, I'll send in another pull request. Maybe change it to bin/rails ?

Member

👍

@tenderlove
Member

Why?

@tenderlove
Member

Rails loads fixtures by default when you run the rake suite tasks. So "rake test" and "rake test:units" will load fixtures. This should do the same. But don't load fixtures when running one specific file or test within that file. In that case, we should have an option like --load-fixtures/-f that does it.

@dhh why?

This change

  • breaks our usage of autotest
  • prevents us from running one test file via ruby
  • eliminates the possibility of focusing on one failing test
  • eliminates the Rake task hooks we use for environment specific setup

Providing rails test seems fine for people that want to use it, but why do we need to remove these hooks?

Being able to run one test with just plain old ruby is an extremely important use case for me. I would like to know the essence of this feature so that I can add support for my usecase again.

@tenderlove
Member

Just a couple more thoughts on this. When you do something like this:

$ rails generate scaffold LineItems product:references cart:belongs_to

Rails generates a controller test that depends on fixtures, but that file cannot stand on it's own. It depends on fixtures, but does not load them. Someone writing their own controller test can choose whether or not to load fixtures, but as long as we're generating a controller test that depends on fixtures, shouldn't that file be able to run on it's own?

@dhh
Member
dhh commented Mar 21, 2013

Aaron, today fixtures are not loaded when you do "ruby -Itest test/unit/some_test.rb". "rails test test/unit/some_test.rb" would mimic that. The reason is that running a single test requires a quick feedback loop and loading all fixtures is slow, so reloading all the fixtures every time you run an isolated test where the fixtures are unlikely to change would be a poor trade-off. We'd still allow you to specify that fixtures should be reloaded, like "rails test test/unit/some_test.rb --reload-fixtures".

"rails test models" would work just like "rake test:models" do today, which includes reloading the fixtures. The reason here is that the trade-off is different. Running an entire battery of tests take a fair while in most cases, so it's not an undue tax to reload fixtures before doing so.

In other words, "rails test" operates just like rake + one-off ruby running does today. It mimics those behaviors to fit the trade-offs that are inherent in either operation.

I don't see how any of these changes break existing code. Could you explain further?

On Mar 20, 2013, at 6:20 PM, Aaron Patterson notifications@github.com wrote:

Just a couple more thoughts on this. When you do something like this:

$ rails generate scaffold LineItems product:references cart:belongs_to

Rails generates a controller test that depends on fixtures, but that file cannot stand on it's own. It depends on fixtures, but does not load them. Someone writing their own controller test can choose whether or not to load fixtures, but as long as we're generating a controller test that depends on fixtures, shouldn't that file be able to run on it's own?


Reply to this email directly or view it on GitHub.

@rubys
Contributor
rubys commented Mar 21, 2013

today fixtures are not loaded when you do "ruby -Itest test/unit/some_test.rb"

Actually, they do with Rails 3.2.x and Rails 4.0.0.beta1. And did up to this change, apparently in response to your request: 1a0c58b#L2L9

@dhh
Member
dhh commented Mar 21, 2013

That's just loading the fixtures. What's I'm talking about is setting them up in the database, that's what's taking time. rake db:fixtures:load.

On Mar 21, 2013, at 9:01 AM, Sam Ruby notifications@github.com wrote:

today fixtures are not loaded when you do "ruby -Itest test/unit/some_test.rb"

Actually, they do with Rails 3.2.x and Rails 4.0.0.beta1. And did up to this change, apparently in response to your request: 1a0c58b#L2L9


Reply to this email directly or view it on GitHub.

@sikachu
Member
sikachu commented Mar 21, 2013

Wait. Let me double-check. I might interpret what @dhh wanted wrong in 1a0c58b#L2L9

We don't want to create records in the database when you start your test, but we definitely want you to be able to use users(:david) in your test.

@rubys
Contributor
rubys commented Mar 21, 2013

today fixtures are not loaded

Actually, they do with Rails 3.2.x and Rails 4.0.0.beta1.

That's just loading the fixtures.

Ok. Now I'm thoroughly confused :-)

In any case, the problem that Aaron is seeing, and would like reversed, is as follows:

$ ruby -I lib:test test/controllers/line_items_controller_test.rb 
Run options: --seed 26717

# Running tests:

EEEEEEE

Finished tests in 0.020279s, 345.1847 tests/s, 0.0000 assertions/s.

  1) Error:
LineItemsControllerTest#test_should_create_line_item:
NoMethodError: undefined method `line_items' for #<LineItemsControllerTest:0x007f8d5c520d28>
    test/controllers/line_items_controller_test.rb:5:in `block in <class:LineItemsControllerTest>'
@dhh
Member
dhh commented Mar 21, 2013

That's a bug then. Prem must have misunderstood. The way it should work is:

  1. "rails test models/single_test" should NOT reload the db fixtures, but MUST provide access to the existing fixtures through the accessors, like line_items(:one).

  2. "rails test models/single_test --reload-fixtures" MUST reload the db fixtures AND provide access through accessors.

  3. "rails test models" MUST reload the db fixtures AND provide access through accessors.

  4. "rails test" MUST reload the db fixtures AND provide access through accessors.

If any of these points are not currently true, it's a bug.

On Mar 21, 2013, at 9:29 AM, Sam Ruby notifications@github.com wrote:

today fixtures are not loaded

Actually, they do with Rails 3.2.x and Rails 4.0.0.beta1.

That's just loading the fixtures.

Ok. Now I'm thoroughly confused :-)

In any case, the problem that Aaron is seeing, and would like reversed, is as follows:

$ ruby -I lib:test test/controllers/line_items_controller_test.rb
Run options: --seed 26717

Running tests:

EEEEEEE

Finished tests in 0.020279s, 345.1847 tests/s, 0.0000 assertions/s.

  1. Error:
    LineItemsControllerTest#test_should_create_line_item:
    NoMethodError: undefined method line_items' for #<LineItemsControllerTest:0x007f8d5c520d28> test/controllers/line_items_controller_test.rb:5:inblock in class:LineItemsControllerTest'


Reply to this email directly or view it on GitHub.

@sikachu
Member
sikachu commented Mar 21, 2013

@rubys I think I was the one who got it wrong. Load fixture != put them in the database.

So it seems like we didn't even create records in the database when we ran rake test though. Let me fix it.

Thanks @dhh @rubys and @tenderlove, and I'm sorry that I got it wrong.

@dhh
Member
dhh commented Mar 21, 2013

No worries, Prem. It is a little confusing that loading fixtures and providing access to them are two separate tasks. Glad we got it cleared up. Can you ping this thread again when it's all fixed up and Aaron/Sam can retest? Thanks!

On Mar 21, 2013, at 9:35 AM, Prem Sichanugrist notifications@github.com wrote:

@rubys I think I was the one who got it wrong. Load fixture != put them in the database.

So it seems like we didn't even create records in the database when we ran rake test though. Let me fix it.

Thanks @dhh @rubys and @tenderlove, and I'm sorry that I got it wrong.


Reply to this email directly or view it on GitHub.

@sikachu
Member
sikachu commented Mar 21, 2013

I've created #9854 to track the progress and put it on 4.0.0 milestone!

@rubys
Contributor
rubys commented Mar 21, 2013

I've created #9854 to track the progress and put it on 4.0.0 milestone!

👍

@tenderlove
Member

I don't see how any of these changes break existing code. Could you explain further?

@dhh the rake tasks call exec which kills the existing process. Any code that was executing after the rake tasks will no longer execute. Also, it looks like the test tasks have been changed to run bundle exec .... Booting the app runs bundle setup, so running "bundle exec" just adds more overhead.

The actual problem behind the slow tests is that we load the environment twice. @wangjohn (and team MIT) are working to remove the requirement that we have one app per process. Once this requirement is gone, we will have the ability to switch environments in the same process. That means we can fix the rake tasks to have the same speed as rails test but deprecate nothing and break no existing code.

I would like to revert the changes to the rake tasks. There is no reason to deprecate them, so we should not.

TL;DR we can eventually make the rake tasks as fast as rails test so there is no reason to deprecate them

@dhh
Member
dhh commented Mar 25, 2013

I guess I don't see why we need two ways to run the same thing. I'm fine that we have a transition period, but at some point we should pick that "rails test" is the way to run tests in Rails, and deprecate/extract the rake tasks that do the same time.

@tenderlove
Member

I guess I don't see why we need two ways to run the same thing. I'm fine that we have a transition period, but at some point we should pick that "rails test" is the way to run tests in Rails, and deprecate/extract the rake tasks that do the same time.

Why rails test? The rake tasks give people a standard place and way to hook pre / post execution code. Is it just the command line interface you want to change?

@dhh
Member
dhh commented Mar 25, 2013

I actually don't care much whether it's one or the other. What I care about is:

  1. "X test" is super duper fast and can be paired with something like spring to be even faster.
  2. That Rails has one way to test things.

I find the argument form for rake test a little awkward, so from a CLI perspective, "rails test" seems nicer. But that's not the major point.

@tenderlove
Member

@dhh these seem like fine requirements. We can eventually make rake test speed equal rails test speed and the rake tasks should work fine with things like spring. The important thing for me is that we have a place to hook setup / teardown code.

rails test could be implemented in terms of Rake, which would give us the same standard hooks.

Quick proof that rake test does not have to be slow:

$ time bin/rails test
.......

Finished tests in 0.202827s, 34.5122 tests/s, 64.0940 assertions/s.

7 tests, 13 assertions, 0 failures, 0 errors, 0 skips

real    0m2.586s
user    0m2.231s
sys 0m0.325s
$ time RAILS_ENV=test rake aarontest
Run options: --seed 11

# Running tests:

.......

Finished tests in 0.177000s, 39.5480 tests/s, 73.4463 assertions/s.

7 tests, 13 assertions, 0 failures, 0 errors, 0 skips

real    0m1.960s
user    0m1.723s
sys 0m0.224s
$

Here's the definition of aarontest:

task :aarontest do
  require 'minitest/autorun'
  $: << '.'
  $: << 'test'
  Dir['test/**/*_test.rb'].each { |x| require x }
end
@dhh
Member
dhh commented Mar 25, 2013

I buy all that. So the next question is timing. How quickly can we speed up rake test? In time for 4.0? If not, is it worth shipping "rails test" in the mean time?

I would really like to have fast tests for 4.0.

On Mar 25, 2013, at 10:44 AM, Aaron Patterson notifications@github.com wrote:

@dhh these seem like fine requirements. We can eventually make rake test speed equal rails test speed and the rake tasks should work fine with things like spring. The important thing for me is that we have a place to hook setup / teardown code.

rails test could be implemented in terms of Rake, which would give us the same standard hooks.

Quick proof that rake test does not have to be slow:

$ time bin/rails test
.......

Finished tests in 0.202827s, 34.5122 tests/s, 64.0940 assertions/s.

7 tests, 13 assertions, 0 failures, 0 errors, 0 skips

real 0m2.586s
user 0m2.231s
sys 0m0.325s
$ time RAILS_ENV=test rake aarontest
Run options: --seed 11

Running tests:

.......

Finished tests in 0.177000s, 39.5480 tests/s, 73.4463 assertions/s.

7 tests, 13 assertions, 0 failures, 0 errors, 0 skips

real 0m1.960s
user 0m1.723s
sys 0m0.224s
$

Here's the definition of aarontest:

task :aarontest do

require 'minitest/autorun'

$: << '.'

$: << 'test'

Dir['test/*_/__test.rb'].each { |x| require x }
end

Reply to this email directly or view it on GitHub.

@tenderlove

Why load and not request? Do we support loading the same file twice?

Member

I think you mean require?

I was using require before, then someone suggest me to use load instead. I don't think we would want to support loading the same file twice anyway.

@tenderlove
Member

Alright. I don't feel comfortable shipping the singleton removal in 4.0. I think our deadline is too close.

Instead, it is possible to detect that we are inside a rake test task before the application gets loaded. I've added a diff here which reverts the rake tasks but makes them as fast as rails test. I had to add a hack to rails/all in this commit, but it allows us to switch the environment to test before the app is ever loaded.

If we're OK with that hack, I can modify all the test tasks to no longer shell out. People can have fast tests without using rails test.

Here's output from a test app (using @rubys data):

$ time rake test:controllers
Run options: --seed 10096

# Running tests:

.......

Finished tests in 0.125721s, 55.6788 tests/s, 103.4036 assertions/s.

7 tests, 13 assertions, 0 failures, 0 errors, 0 skips

real    0m2.034s
user    0m1.785s
sys 0m0.234s
$
@dhh
Member
dhh commented Mar 25, 2013

I'd be OK with a hack if we have a clean path to a solid solution. Then we don't have to divert to "rails test" on the API side. So I say let's roll with your speed up for "rake test" and thank prem for his working that provoked this, even if we don't end up keeping it. The goal all along was fast tests, so that's the big deal!

@sikachu
Member
sikachu commented Mar 25, 2013

Yeah, as long as we have a fast test runner, I don't mind if at the end we decide to not including rails test at all.

@tenderlove
Member

@sikachu thank you for the work you've done on this. I really appreciate the effort you put in! ❤️

@dhh ok. Seems good. I will makeitso.

@alindeman
Contributor

@tenderlove, et al, is there a way that rspec-rails's spec tasks can be invoked in RAILS_ENV='test' without booting the app twice too?

I think most rspec-rails users use the rspec command instead of rake (which has the same performance benefits as the changes here), but I think it would be nice to bring this speedup to those who do use rake spec.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment