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

`bin/rails test` runner (rerun snippets, run tests by line, option documentation) #19216

Merged
merged 25 commits into from Mar 19, 2015

Conversation

Projects
None yet
@senny
Member

senny commented Mar 5, 2015

Successor of #18596.

This is the first stab at implementing a test runner for Rails. Moving forward we want to achieve the following goals with our runner:

  • Have an executable with proper arguments and documentation
  • Provide features not part of minitest. (running by line, rerun snippets, color?)
  • Get rid of some hacks we had to pull when using rake as a test runner (mainly around Rails.env setup)

Follow-up work (can happen after this PR has been merged):

  • revisit how plugins can hook into the testing process (rake test:prepare db:test:prepare)
  • expand the runner (color?, fail fast?)
  • ...
Next Steps

Get feedback and merge this foundation into rails/rails. Then everyone gets the chance to help us improve the runner.

Thank you @arthurnn for helping me out ❤️

This work was inspired by maxitest.

@senny

This comment has been minimized.

Show comment
Hide comment
@senny
Member

senny commented Mar 5, 2015

@senny senny referenced this pull request Mar 5, 2015

Closed

WIP: First commit of improving test runner on rails #18596

0 of 4 tasks complete
@arthurnn

This comment has been minimized.

Show comment
Hide comment
@arthurnn

arthurnn Mar 5, 2015

Member

After this is merged, a failed test would be shown like:

[arthurnn@ralph myapp]$ bin/rails test /Users/arthurnn/dev/railstest/myapp/test/models/comment_test.rb:8
Run options: --seed 28435

# Running:

F

Finished in 0.007840s, 127.5510 runs/s, 255.1020 assertions/s.

  1) Failure:
CommentTest#test_failling_one [/Users/arthurnn/dev/railstest/myapp/test/models/comment_test.rb:10]:
Failed refutation, no message given

1 runs, 2 assertions, 1 failures, 0 errors, 0 skips

Failed tests:

bin/rails test /Users/arthurnn/dev/railstest/myapp/test/models/comment_test.rb:8

Also things like this would work:

bin/rails test
bin/rails test test/models
bin/rails test test/models/comment_test.rb

And all the rake equivalents:

bin/rake test
bin/rake test test/models
bin/rake test test/models/comment_test.rb

cc @dhh / @tenderlove

Member

arthurnn commented Mar 5, 2015

After this is merged, a failed test would be shown like:

[arthurnn@ralph myapp]$ bin/rails test /Users/arthurnn/dev/railstest/myapp/test/models/comment_test.rb:8
Run options: --seed 28435

# Running:

F

Finished in 0.007840s, 127.5510 runs/s, 255.1020 assertions/s.

  1) Failure:
CommentTest#test_failling_one [/Users/arthurnn/dev/railstest/myapp/test/models/comment_test.rb:10]:
Failed refutation, no message given

1 runs, 2 assertions, 1 failures, 0 errors, 0 skips

Failed tests:

bin/rails test /Users/arthurnn/dev/railstest/myapp/test/models/comment_test.rb:8

Also things like this would work:

bin/rails test
bin/rails test test/models
bin/rails test test/models/comment_test.rb

And all the rake equivalents:

bin/rake test
bin/rake test test/models
bin/rake test test/models/comment_test.rb

cc @dhh / @tenderlove

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Mar 5, 2015

Member

❤️ 💚 💙 💛 💜

Member

rafaelfranca commented Mar 5, 2015

❤️ 💚 💙 💛 💜

@kaspth

This comment has been minimized.

Show comment
Hide comment
@kaspth

kaspth Mar 5, 2015

Member

Should we make a convention out of a test pattern, say test/**/*_test.rb, and support syntax such as:

bin/rails test models
bin/rails test models/comment  

which, respectively, would be the same as:

bin/rails test test/models
bin/rails test test/models/comment_test.rb
Member

kaspth commented Mar 5, 2015

Should we make a convention out of a test pattern, say test/**/*_test.rb, and support syntax such as:

bin/rails test models
bin/rails test models/comment  

which, respectively, would be the same as:

bin/rails test test/models
bin/rails test test/models/comment_test.rb
@senny

This comment has been minimized.

Show comment
Hide comment
@senny

senny Mar 5, 2015

Member

@kaspth That is what we had with rake test. In my opinion this is not necessary. When using the rspec runner I was always fine having to write the directory. I also find it much easier to discover than more convention case.

Member

senny commented Mar 5, 2015

@kaspth That is what we had with rake test. In my opinion this is not necessary. When using the rspec runner I was always fine having to write the directory. I also find it much easier to discover than more convention case.

@kaspth

This comment has been minimized.

Show comment
Hide comment
@kaspth

kaspth Mar 5, 2015

Member

@senny I'm having trouble following you about the discoverability part, can you elaborate?

Member

kaspth commented Mar 5, 2015

@senny I'm having trouble following you about the discoverability part, can you elaborate?

@senny

This comment has been minimized.

Show comment
Hide comment
@senny

senny Mar 5, 2015

Member

I myself find it easy to understand what the possibilities are when the first argument is called file or directory. The convention pattern approach relies on documentation for people to find it. When working on this I looked closer at the bin/rake test task and there was tons of stuff I didn't know it could do. Like
bin/rake test app/user for example.

I'm not really opposed to adding it but I have the feeling that doesn't add much to the table.

Member

senny commented Mar 5, 2015

I myself find it easy to understand what the possibilities are when the first argument is called file or directory. The convention pattern approach relies on documentation for people to find it. When working on this I looked closer at the bin/rake test task and there was tons of stuff I didn't know it could do. Like
bin/rake test app/user for example.

I'm not really opposed to adding it but I have the feeling that doesn't add much to the table.

@senny

This comment has been minimized.

Show comment
Hide comment
@senny

senny Mar 5, 2015

Member

Also, the directories are autocompletable while the patterns are not.

Member

senny commented Mar 5, 2015

Also, the directories are autocompletable while the patterns are not.

@kaspth

This comment has been minimized.

Show comment
Hide comment
@kaspth

kaspth Mar 5, 2015

Member

@senny The shipping test task does a lot of stuff that's hard to figure out. I'm 👍 on reigning it in.

Member

kaspth commented Mar 5, 2015

@senny The shipping test task does a lot of stuff that's hard to figure out. I'm 👍 on reigning it in.

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Mar 5, 2015

Member

👍 to only using paths.

On Mar 5, 2015, at 08:26, Yves Senn notifications@github.com wrote:

Also, the directories are autocompletable while the patterns are not.


Reply to this email directly or view it on GitHub.

Member

dhh commented Mar 5, 2015

👍 to only using paths.

On Mar 5, 2015, at 08:26, Yves Senn notifications@github.com wrote:

Also, the directories are autocompletable while the patterns are not.


Reply to this email directly or view it on GitHub.

def Minitest.plugin_rails_init(options)
self.reporter << Rails::TestUnitReporter.new(options[:io], options)
if $rails_test_runner && (method = $rails_test_runner.find_method)

This comment has been minimized.

@tenderlove

tenderlove Mar 5, 2015

Member

What is this plugin trying to do? I don't understand this code.

@tenderlove

tenderlove Mar 5, 2015

Member

What is this plugin trying to do? I don't understand this code.

This comment has been minimized.

@arthurnn

arthurnn Mar 5, 2015

Member

This plugin is here for two things:

  • Add the TestUnitReporter , which outputs:
Failed tests:

bin/rails test /Users/arthurnn/dev/railstest/myapp/test/models/comment_test.rb:8
  • Add a filter option to minitest. So we can find the method that we want to run if the runner had a line argument specified, and only run that individual test.
@arthurnn

arthurnn Mar 5, 2015

Member

This plugin is here for two things:

  • Add the TestUnitReporter , which outputs:
Failed tests:

bin/rails test /Users/arthurnn/dev/railstest/myapp/test/models/comment_test.rb:8
  • Add a filter option to minitest. So we can find the method that we want to run if the runner had a line argument specified, and only run that individual test.

This comment has been minimized.

@tenderlove

tenderlove Mar 6, 2015

Member

I see. Since we control the superclass (everything inherits from AS::TestCase) we can implement the runnables method, so maybe we don't need these globals.

@tenderlove

tenderlove Mar 6, 2015

Member

I see. Since we control the superclass (everything inherits from AS::TestCase) we can implement the runnables method, so maybe we don't need these globals.

This comment has been minimized.

@arthurnn

arthurnn Mar 6, 2015

Member

I am not 100% if I follow.
We still need to pass to Minitest runner which method we need to run, so we need the plugin, and need someway to know if we are running with the rails_runner or not, thats why $rails_test_runner

@arthurnn

arthurnn Mar 6, 2015

Member

I am not 100% if I follow.
We still need to pass to Minitest runner which method we need to run, so we need the plugin, and need someway to know if we are running with the rails_runner or not, thats why $rails_test_runner

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Mar 7, 2015

Member

Does this report failures as it's running, or only at the end?

Member

dhh commented Mar 7, 2015

Does this report failures as it's running, or only at the end?

@arthurnn

This comment has been minimized.

Show comment
Hide comment
@arthurnn

arthurnn Mar 7, 2015

Member

Does this report failures as it's running, or only at the end?

Only at the end for now. Now that we have our reporter and a rails minitest plugin we will definitely do that too.

Member

arthurnn commented Mar 7, 2015

Does this report failures as it's running, or only at the end?

Only at the end for now. Now that we have our reporter and a rails minitest plugin we will definitely do that too.

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Mar 8, 2015

Member

👍

On Mar 7, 2015, at 15:36, Arthur Nogueira Neves notifications@github.com wrote:

Does this report failures as it's running, or only at the end?

Only at the end for now. Now that we have our reporter and a rails minitest plugin we will definitely do that too.


Reply to this email directly or view it on GitHub.

Member

dhh commented Mar 8, 2015

👍

On Mar 7, 2015, at 15:36, Arthur Nogueira Neves notifications@github.com wrote:

Does this report failures as it's running, or only at the end?

Only at the end for now. Now that we have our reporter and a rails minitest plugin we will definitely do that too.


Reply to this email directly or view it on GitHub.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Mar 10, 2015

Member

Seems good to me but I don't know anything about the implementation. Maybe it is good to ask @zenspider's opinion about the implementation before merging.

Member

rafaelfranca commented Mar 10, 2015

Seems good to me but I don't know anything about the implementation. Maybe it is good to ask @zenspider's opinion about the implementation before merging.

@zenspider

This comment has been minimized.

Show comment
Hide comment
@zenspider

zenspider Mar 10, 2015

Contributor

I've just taken a quick glance at this and I guess I don't get it. I can see that you're using the plugin system to put in a custom reporter (almost the same as the reporters provided in seattlerb/minitest-sprint btw) and that's perfect. But, I don't understand why Rails::TestRunner exists as a whole. Can you explain why it exists? It looks like it is a reinvented wheel.

Also, it looks like run_test_file tests the test mechanism by shelling out. That's heavy. Is there some reason for that?

Contributor

zenspider commented Mar 10, 2015

I've just taken a quick glance at this and I guess I don't get it. I can see that you're using the plugin system to put in a custom reporter (almost the same as the reporters provided in seattlerb/minitest-sprint btw) and that's perfect. But, I don't understand why Rails::TestRunner exists as a whole. Can you explain why it exists? It looks like it is a reinvented wheel.

Also, it looks like run_test_file tests the test mechanism by shelling out. That's heavy. Is there some reason for that?

@arthurnn

This comment has been minimized.

Show comment
Hide comment
@arthurnn

arthurnn Mar 10, 2015

Member

@zenspider the Rails::TestRunner exists for a few reasons:

  • encapsulate the options we can receive on the runner i.e. rails test file.rb, file.rb is a parameter.
  • wrapper to find_method, so we can find the test method if you pass a line number to the runner.
  • Setup Rails configs, for instance ENV["RAILS_ENV"]

Pretty much, its main responsibility, is to encapsulate the test options, so we can use it on the minitest plugin, but also on the rails command https://github.com/rails/rails/pull/19216/files#diff-d372de5c2ec1bba7b73aa34bb2d2a798R5 https://github.com/rails/rails/pull/19216/files#diff-47b9fc20bec6924b9946969a002d33c4R9

Member

arthurnn commented Mar 10, 2015

@zenspider the Rails::TestRunner exists for a few reasons:

  • encapsulate the options we can receive on the runner i.e. rails test file.rb, file.rb is a parameter.
  • wrapper to find_method, so we can find the test method if you pass a line number to the runner.
  • Setup Rails configs, for instance ENV["RAILS_ENV"]

Pretty much, its main responsibility, is to encapsulate the test options, so we can use it on the minitest plugin, but also on the rails command https://github.com/rails/rails/pull/19216/files#diff-d372de5c2ec1bba7b73aa34bb2d2a798R5 https://github.com/rails/rails/pull/19216/files#diff-47b9fc20bec6924b9946969a002d33c4R9

@tenderlove

This comment has been minimized.

Show comment
Hide comment
@tenderlove

tenderlove Mar 10, 2015

Member

We can do the file name and line number thing without the global. Here is a patch against master that will do it. Adding a runner file seems fine. Maybe we could get access to the option parser on minitest so we don't have to duplicate the settings.

Member

tenderlove commented Mar 10, 2015

We can do the file name and line number thing without the global. Here is a patch against master that will do it. Adding a runner file seems fine. Maybe we could get access to the option parser on minitest so we don't have to duplicate the settings.

@arthurnn

This comment has been minimized.

Show comment
Hide comment
@arthurnn

arthurnn Mar 11, 2015

Member

@tenderlove A few concerns of you gist:

  • This will only work if you pass the exactly line number that the tests starts at.
  • You will need to pass the file:line on the filter option of minitest(-n), which is probably not what we want.

I agree that the global is not ideal, I am sure after we merge this people will come-up with a better solution to fix this problem. But to be honest I think this is kinda a implementation detail.

@senny thoughts?

Member

arthurnn commented Mar 11, 2015

@tenderlove A few concerns of you gist:

  • This will only work if you pass the exactly line number that the tests starts at.
  • You will need to pass the file:line on the filter option of minitest(-n), which is probably not what we want.

I agree that the global is not ideal, I am sure after we merge this people will come-up with a better solution to fix this problem. But to be honest I think this is kinda a implementation detail.

@senny thoughts?

@tenderlove

This comment has been minimized.

Show comment
Hide comment
@tenderlove

tenderlove Mar 11, 2015

Member

This will only work if you pass the exactly line number that the tests starts at.

Yes, I did this as a proof of concept to show that the implementation in this PR may not be the ideal place. I'm trying to give a working demo that doesn't require so much code.

Member

tenderlove commented Mar 11, 2015

This will only work if you pass the exactly line number that the tests starts at.

Yes, I did this as a proof of concept to show that the implementation in this PR may not be the ideal place. I'm trying to give a working demo that doesn't require so much code.

senny added a commit that referenced this pull request Mar 19, 2015

Merge pull request #19216 from senny/bin_test_runner
`bin/rails test` runner (rerun snippets, run tests by line, option documentation)

@senny senny merged commit 9959e95 into rails:master Mar 19, 2015

@senny senny deleted the senny:bin_test_runner branch Mar 19, 2015

@senny

This comment has been minimized.

Show comment
Hide comment
@senny

senny Mar 19, 2015

Member

While the implementation is not perfect, I'm merging this to create a foundation we can collaborate on. Feel free to send pull requests improving the runner 💛

Member

senny commented Mar 19, 2015

While the implementation is not perfect, I'm merging this to create a foundation we can collaborate on. Feel free to send pull requests improving the runner 💛

@arthurnn

This comment has been minimized.

Show comment
Hide comment
@arthurnn

arthurnn Mar 19, 2015

Member

❤️ ❤️ ❤️ ❤️ ❤️

Member

arthurnn commented Mar 19, 2015

❤️ ❤️ ❤️ ❤️ ❤️

arthurnn added a commit that referenced this pull request Mar 19, 2015

Merge branch 'bin_test_runner'. #19216
3 commits were missing when we merged the PR, probably they were lost
when that branch was rebased against latest master.
This merge, contains those 3 commits.
@zenspider

This comment has been minimized.

Show comment
Hide comment
@zenspider

zenspider Mar 19, 2015

Contributor

This still seems like a metric fuckton of code for something that really only needs to hook into the plugin system that minitest already provides. Having an entire runner, adding ~400 lines but only removing ~60? Seems entirely unnecessary. This can be done in 20-30 lines using facilities that minitest already provides.

  • encapsulate the options we can receive on the runner i.e. rails test file.rb, file.rb is a parameter.

Minitest already has a system for plugins to extend option processing.

  • wrapper to find_method, so we can find the test method if you pass a line number to the runner.

Doesn't need a wrapper. You just need to process and manipulate options

  • Setup Rails configs, for instance ENV["RAILS_ENV"]

That certainly doesn't need a runner. Again, the plugin system handles this entirely.

Contributor

zenspider commented Mar 19, 2015

This still seems like a metric fuckton of code for something that really only needs to hook into the plugin system that minitest already provides. Having an entire runner, adding ~400 lines but only removing ~60? Seems entirely unnecessary. This can be done in 20-30 lines using facilities that minitest already provides.

  • encapsulate the options we can receive on the runner i.e. rails test file.rb, file.rb is a parameter.

Minitest already has a system for plugins to extend option processing.

  • wrapper to find_method, so we can find the test method if you pass a line number to the runner.

Doesn't need a wrapper. You just need to process and manipulate options

  • Setup Rails configs, for instance ENV["RAILS_ENV"]

That certainly doesn't need a runner. Again, the plugin system handles this entirely.

@arthurnn

This comment has been minimized.

Show comment
Hide comment
@arthurnn

arthurnn Mar 19, 2015

Member

thanks @zenspider . I will give it a shot at your suggestions, and cc you in the PR. ❤️

Member

arthurnn commented Mar 19, 2015

thanks @zenspider . I will give it a shot at your suggestions, and cc you in the PR. ❤️

@senny

This comment has been minimized.

Show comment
Hide comment
@senny

senny Mar 20, 2015

Member

@zenspider I agree that we can make the implementation more focus. One thing I really want to have with the new runner is to advertise it's functionality. Similar to the rspec command I'd like to print a "Usage" for every option we have to make the features more discoverable.

Member

senny commented Mar 20, 2015

@zenspider I agree that we can make the implementation more focus. One thing I really want to have with the new runner is to advertise it's functionality. Similar to the rspec command I'd like to print a "Usage" for every option we have to make the features more discoverable.

@zenspider

This comment has been minimized.

Show comment
Hide comment
@zenspider

zenspider Mar 20, 2015

Contributor

That's handled too.

Contributor

zenspider commented Mar 20, 2015

That's handled too.

@senny

This comment has been minimized.

Show comment
Hide comment
@senny

senny Mar 20, 2015

Member

@zenspider thanks for the heads up. We'll look into a better integration with minitest to reduce the code on our end.

Member

senny commented Mar 20, 2015

@zenspider thanks for the heads up. We'll look into a better integration with minitest to reduce the code on our end.

@matthuhiggins

This comment has been minimized.

Show comment
Hide comment
@matthuhiggins

matthuhiggins Mar 21, 2015

It is worth mentioning that this integrates spring with single test runs.

matthuhiggins commented Mar 21, 2015

It is worth mentioning that this integrates spring with single test runs.

@BenMorganIO

This comment has been minimized.

Show comment
Hide comment
@BenMorganIO

BenMorganIO Mar 21, 2015

Contributor

👍

Contributor

BenMorganIO commented Mar 21, 2015

👍

@sunnyrjuneja

This comment has been minimized.

Show comment
Hide comment
@sunnyrjuneja

sunnyrjuneja Mar 25, 2015

Hi everybody, Is this in the rails 4.2.1 release? I just updated rails and it didn't show up in help.

λ →  bin/rails -v
Rails 4.2.1
λ →  bin/rails -h
Usage: rails COMMAND [ARGS]

The most common rails commands are:
 generate    Generate new code (short-cut alias: "g")
 console     Start the Rails console (short-cut alias: "c")
 server      Start the Rails server (short-cut alias: "s")
 dbconsole   Start a console for the database specified in config/database.yml
             (short-cut alias: "db")
 new         Create a new Rails application. "rails new my_app" creates a
             new application called MyApp in "./my_app"

In addition to those, there are:
 destroy      Undo code generated with "generate" (short-cut alias: "d")
 plugin new   Generates skeleton for developing a Rails plugin
 runner       Run a piece of code in the application environment (short-cut alias: "r")

All commands can be run with -h (or --help) for more information.

sunnyrjuneja commented Mar 25, 2015

Hi everybody, Is this in the rails 4.2.1 release? I just updated rails and it didn't show up in help.

λ →  bin/rails -v
Rails 4.2.1
λ →  bin/rails -h
Usage: rails COMMAND [ARGS]

The most common rails commands are:
 generate    Generate new code (short-cut alias: "g")
 console     Start the Rails console (short-cut alias: "c")
 server      Start the Rails server (short-cut alias: "s")
 dbconsole   Start a console for the database specified in config/database.yml
             (short-cut alias: "db")
 new         Create a new Rails application. "rails new my_app" creates a
             new application called MyApp in "./my_app"

In addition to those, there are:
 destroy      Undo code generated with "generate" (short-cut alias: "d")
 plugin new   Generates skeleton for developing a Rails plugin
 runner       Run a piece of code in the application environment (short-cut alias: "r")

All commands can be run with -h (or --help) for more information.
@arthurnn

This comment has been minimized.

Show comment
Hide comment
@arthurnn

arthurnn Mar 25, 2015

Member

@whatasunnyday no, this is on rails 5 only.
I am extracting this to a gem, to make it work on rails 4.1 and 4.2 !

Member

arthurnn commented Mar 25, 2015

@whatasunnyday no, this is on rails 5 only.
I am extracting this to a gem, to make it work on rails 4.1 and 4.2 !

@sunnyrjuneja

This comment has been minimized.

Show comment
Hide comment
@sunnyrjuneja

sunnyrjuneja Mar 25, 2015

@arthurnn I just realized that it said landed on master, not 4.2.1. I'm dumb! Sorry for the useless comment. Anyways, thanks for the great work ❤️ ❤️ ❤️.

sunnyrjuneja commented Mar 25, 2015

@arthurnn I just realized that it said landed on master, not 4.2.1. I'm dumb! Sorry for the useless comment. Anyways, thanks for the great work ❤️ ❤️ ❤️.

@BenMorganIO

This comment has been minimized.

Show comment
Hide comment
@BenMorganIO

BenMorganIO Mar 25, 2015

Contributor

@whatasunnyday I was hoping for it to be in 4.2.1 as well lol

Contributor

BenMorganIO commented Mar 25, 2015

@whatasunnyday I was hoping for it to be in 4.2.1 as well lol

@emilebosch

This comment has been minimized.

Show comment
Hide comment
@emilebosch

emilebosch Apr 1, 2015

Hey guys, is this gonna be implementing fail fast, and rerun failed tests? This would be awesome.

emilebosch commented Apr 1, 2015

Hey guys, is this gonna be implementing fail fast, and rerun failed tests? This would be awesome.

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