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

simplify rake test vs rake test:all #17348

Merged
merged 1 commit into from Nov 12, 2014

Conversation

Projects
None yet
5 participants
@DavidGeukers
Copy link
Contributor

DavidGeukers commented Oct 21, 2014

Remove rake test:all and replace rake test with former definition of rake test:all. Also rename rake test:all:db to rake test:db

@dhh

This comment has been minimized.

Copy link
Member

dhh commented Oct 22, 2014

@jeremy I vaguely remember that we had some issues with this setup? Do you remember?

Also, does this work well with Spring and its default rake test route?

@DavidGeukers DavidGeukers force-pushed the DavidGeukers:rake_test_all branch 2 times, most recently Oct 22, 2014

@arthurnn

This comment has been minimized.

Copy link
Member

arthurnn commented Oct 22, 2014

Paired with @DavidGeukers in this one. we tested spring in a local app, and everything worked fine..
We probably will have to change docs in this PR too, and add a changelog entry. But we wanted to validate the approach before moving forward.

@jeremy

View changes

railties/lib/rails/test_unit/testing.rake Outdated
task :db => %w[db:test:prepare test:all]
end
desc "Run tests quickly, but also reset db"
task :db => %w[db:test:prepare test]

This comment has been minimized.

@jeremy

jeremy Oct 22, 2014

Member

Should probably keep the old task with a deprecation warning.

This comment has been minimized.

@DavidGeukers

DavidGeukers Nov 7, 2014

Author Contributor

deprecation added

@jeremy

View changes

railties/lib/rails/test_unit/testing.rake Outdated
# Inspired by: http://ngauthier.com/2012/02/quick-tests-with-bash.html
desc "Run tests quickly by merging all types and not resetting db"
Rails::TestTask.new(:all) do |t|
Rails::TestTask.new(:run) do |t|

This comment has been minimized.

@jeremy

jeremy Oct 22, 2014

Member

This changes default test behavior to run everything, which may now include e.g. acceptance/browser/system tests.

For behavior parity, we'd use a tighter glob, or introduce config so people can specify which tests dirs to include by default.

t.pattern = "test/{units,models,etc}/**/*_test.rb"

This comment has been minimized.

@arthurnn

arthurnn Oct 22, 2014

Member

how about a blacklist instead of a whitelist?

This comment has been minimized.

@DavidGeukers

DavidGeukers Nov 7, 2014

Author Contributor

Have been working on different solutions to this, but I'm increasingly becoming convinced that the best solution is to default to running all tests & have users override rake test in their app if they need custom behaviour.

Thoughts?

@jeremy

View changes

railties/lib/rails/test_unit/testing.rake Outdated
@@ -3,7 +3,7 @@ require 'rails/test_unit/sub_test_task'

task default: :test

desc 'Runs test:units, test:functionals, test:generators, test:integration, test:jobs together'
desc "Run tests quickly by merging all types and not resetting db"

This comment has been minimized.

@jeremy

jeremy Oct 22, 2014

Member

This isn't actually what the rake test ... task does anymore (or quite what it did before)

This comment has been minimized.

@DavidGeukers

DavidGeukers Nov 7, 2014

Author Contributor

updated

@phoet

This comment has been minimized.

Copy link
Contributor

phoet commented Nov 4, 2014

i like the idea of having rake test run everything in the test folder. it happened to me in several new apps that we added stuff like test/jobs and ran them locally on a per file basis but our CI was actually not picking them up with the default rake task. this even led to breaking apps in production which is really sad...

so we usually put stuff like this in a lib/rake/test.rake so they get executed as "unit-tests"

namespace :test do
  %w(lib job services).each do |name|
    desc "Run #{name} tests"
    Rails::TestTask.new(name => "test:prepare") do |t|
      t.pattern = "test/#{name}/**/*_test.rb"
    end

    task :units => name
  end
end

@DavidGeukers DavidGeukers force-pushed the DavidGeukers:rake_test_all branch Nov 7, 2014

@DavidGeukers

This comment has been minimized.

Copy link
Contributor Author

DavidGeukers commented Nov 7, 2014

Will add changelog & docs changes once a final approach is decided on.

@dhh

This comment has been minimized.

Copy link
Member

dhh commented Nov 10, 2014

I say that "rake test" should just run tests/*_/__test.rb, and then anyone can add their own suites that include less than that if they need be. rake test:without_integration as an app specific thing is just fine.

Let's get this wrapped up and included in 4.2.0.rc1

@dhh dhh added this to the 4.2.0 milestone Nov 10, 2014

Simplify rake test vs rake test:all
Renames `rake test:all` to `rake test` by changing old `rake test:run` to previous version of `rake test:all`.  Removes old definition of `rake test`. Also renames `rake test:all:db` to `rake test:db` and deprecates `rake test:all` & `rake test:all:db`

@DavidGeukers DavidGeukers force-pushed the DavidGeukers:rake_test_all branch to 3b12abb Nov 11, 2014

@DavidGeukers

This comment has been minimized.

Copy link
Contributor Author

DavidGeukers commented Nov 11, 2014

Good to go?

@dhh

This comment has been minimized.

Copy link
Member

dhh commented Nov 12, 2014

Looks good to me. Let's roll with this.

dhh added a commit that referenced this pull request Nov 12, 2014

Merge pull request #17348 from DavidGeukers/rake_test_all
simplify rake test vs rake test:all

@dhh dhh merged commit 2a31ea5 into rails:master Nov 12, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@DavidGeukers DavidGeukers deleted the DavidGeukers:rake_test_all branch Nov 13, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.