Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

Generated tests rely on test dir in load path rather than File.dirnam…

…e shenanigans. ruby -Itest test/unit/foo_test.rb to run a test by hand (that's a capital I as in Island).

git-svn-id: http://svn-commit.rubyonrails.org/rails/trunk@9133 5ecf4fe2-1ee6-0310-87b1-e25e094e27de
  • Loading branch information...
commit e8170805df1a32119db9d328daee1239b338ac71 1 parent 827b529
@jeremy jeremy authored
View
2  railties/lib/rails_generator/generators/components/controller/templates/functional_test.rb
@@ -1,4 +1,4 @@
-require File.dirname(__FILE__) + '<%= '/..' * class_nesting_depth %>/../test_helper'
@drnic
drnic added a note

I disagree with this. I think test files should be executable as is without external loading of dependencies. The previous version here was better.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+require 'test_helper'
class <%= class_name %>ControllerTest < ActionController::TestCase
# Replace this with your real tests.
View
2  railties/lib/rails_generator/generators/components/integration_test/templates/integration_test.rb
@@ -1,4 +1,4 @@
-require "#{File.dirname(__FILE__)}<%= '/..' * class_nesting_depth %>/../test_helper"
+require 'test_helper'
class <%= class_name %>Test < ActionController::IntegrationTest
# fixtures :your, :models
View
2  railties/lib/rails_generator/generators/components/mailer/templates/unit_test.rb
@@ -1,4 +1,4 @@
-require File.dirname(__FILE__) + '<%= '/..' * class_nesting_depth %>/../test_helper'
+require 'test_helper'
class <%= class_name %>Test < ActionMailer::TestCase
tests <%= class_name %>
View
2  railties/lib/rails_generator/generators/components/model/templates/unit_test.rb
@@ -1,4 +1,4 @@
-require File.dirname(__FILE__) + '<%= '/..' * class_nesting_depth %>/../test_helper'
+require 'test_helper'
class <%= class_name %>Test < ActiveSupport::TestCase
# Replace this with your real tests.
View
2  railties/lib/rails_generator/generators/components/observer/templates/unit_test.rb
@@ -1,4 +1,4 @@
-require File.dirname(__FILE__) + '<%= '/..' * class_nesting_depth %>/../test_helper'
+require 'test_helper'
class <%= class_name %>ObserverTest < Test::Unit::TestCase
# Replace this with your real tests.
View
2  railties/lib/rails_generator/generators/components/resource/templates/functional_test.rb
@@ -1,4 +1,4 @@
-require File.dirname(__FILE__) + '<%= '/..' * class_nesting_depth %>/../test_helper'
+require 'test_helper'
class <%= controller_class_name %>ControllerTest < ActionController::TestCase
# Replace this with your real tests.
View
2  railties/lib/rails_generator/generators/components/scaffold/templates/functional_test.rb
@@ -1,4 +1,4 @@
-require File.dirname(__FILE__) + '<%= '/..' * class_nesting_depth %>/../test_helper'
+require 'test_helper'
class <%= controller_class_name %>ControllerTest < ActionController::TestCase
def test_should_get_index

41 comments on commit e817080

@augustl

Meh this sucks. Means that you can’t do, say, the run focused unit test command in textmate. What’s the point of this change, really?

@augustl

If there was an edit comment feature on Github, I’d edit my comment to make it less harsh and flame-ish, sorry about that.

Now, there’s plenty of textmate users around, and this change voids many textmate users habits, especially the “run focused unit test” command. Or just command + r to run just that test file.

You can at least run one file explicitly via the command line, but that’s very tideous compared to doing it directly from textmate.

@jeremy
Owner

This change plays much nicer with Ruby’s load path tracking. Please submit a fix to Textmate. It just needs to -Itest or use the rake test task.

@augustl

Good point, this should be something the Rails bundle in textmate groks

@jeremy
Owner

-Itest adds to the load path so you don’t have to hack relative paths in every test. This isn’t external loading of dependencies at all; it’s a simplification.

@augustl

I think the point about being able to run it on the command line without extra parameters – ruby tests/units/foo_test.rb – is a very valid argument as well, and obviously fixing the textmate bundle won’t affect that.

@tarmo

One can always do this: export RUBYOPT=“${RUBYOPT} -Itest”

Though I’m unaware of any negative side-effects doing this could have.

@tarmo

One can always do this: export RUBYOPT=“${RUBYOPT} -Itest”

Though I’m unaware of any negative side-effects doing this could have.

@ddemaree

@tarmo: Will TextMate’s bundled Ruby honor that option? The -Itest argument is a little disruptive, but TextMate’s built-in Ruby runner seems to be the big thing here.

@augustl

Don’t know much ‘bout textmate, but I guess you could piggyback on the Ruby bundle’s Ruby Mate stuff and just set some environment variables and stuff if you’re in Rails’ test scope?

Then again, this still doesn’t help if you wanna do ‘ruby tests/units/foo_test.rb’.

@ddemaree

@leethal: The TextMate issue is twofold. One, the testing commands are split between the Ruby and Rails bundles (mostly the Ruby one). The bundles share a common environment (RubyMate and TM’s built-in Ruby binary) but are separate plugins, and neither bundle’s code is terribly DRY. Plus it’s the Ruby bundle, not Rails, so you probably wouldn’t want to make any big changes just to accommodate a Rails quirk.

Two, TextMate allows the user to supercede any part of any bundle with their own code/modifications which are layered on top of the “master” copy of the bundle at runtime. So if you were to install an updated version of the Ruby or Rails bundles (or if one came packed with a new release of TextMate.app), it’s possible that some or all of the changes would be blocked by stuff in your user’s copy of that bundle. Or not. It depends.

My point here isn’t that it’s impossible to work around this change in TM, but rather that there isn’t a trivial workaround, and even if there were it may not be trivial to apply it.

That’s the worst case scenario, but still — while the dirname shenanigans are ugly and maybe a little inefficient, they’re also unambiguous and runner-agnostic. TextMate is only one tool (though a very popular one), so I wouldn’t suggest that Rails bend to its weird quirks. But the old way of doing this worked with any Ruby tool without hassle.

@ddemaree

@leethal: The TextMate issue is twofold. One, the testing commands are split between the Ruby and Rails bundles (mostly the Ruby one). The bundles share a common environment (RubyMate and TM’s built-in Ruby binary) but are separate plugins, and neither bundle’s code is terribly DRY. Plus it’s the Ruby bundle, not Rails, so you probably wouldn’t want to make any big changes just to accommodate a Rails quirk.

Two, TextMate allows the user to supercede any part of any bundle with their own code/modifications which are layered on top of the “master” copy of the bundle at runtime. So if you were to install an updated version of the Ruby or Rails bundles (or if one came packed with a new release of TextMate.app), it’s possible that some or all of the changes would be blocked by stuff in your user’s copy of that bundle. Or not. It depends.

My point here isn’t that it’s impossible to work around this change in TM, but rather that there isn’t a trivial workaround, and even if there were it may not be trivial to apply it.

That’s the worst case scenario, but still — while the dirname shenanigans are ugly and maybe a little inefficient, they’re also unambiguous and runner-agnostic. TextMate is only one tool (though a very popular one), so I wouldn’t suggest that Rails bend to its weird quirks. But the old way of doing this worked with any Ruby tool without hassle.

@ddemaree

And WOW, how the crap did that happen? Apologies for the double-post. I blame sunspots.

@jamesgolick

“But the old way of doing this worked with any Ruby tool without hassle.”

+1

@jeremy
Owner

This is not a “Rails quirk.” Many Ruby libraries expect a consistent load path that’s set up in their Rakefile’s test task.

The old way was broken and led to double-requires. A bug. I appreciate that tools relied on the convenience of naively running a single test, but these tools can and should be changed to be smarter about how to run tests.

Again, I suggest tools use the Rake task. If a Rakefile is available, you’re guaranteed it runs tests the way the author intends tests to be run.

If no Rakefile is available, let the user configure how they’d like to run the test with, say, a command line with %s filename substitution.

Anyone have an updated TM bundle yet?

@jeremy
Owner

There’s a Lighthouse ticket for this as well, by the way: http://rails.lighthouseapp.com/projects/8994/tickets/218-script-generate-model-creates-wrong-unit-test-file

@jeremy
Owner

And commit f74ba37 is going to break TM’s task even harder. Let’s get that task up to speed now so we can live happily ever after.

@ddemaree

@jeremy: Yeah, I just saw that ticket; it’s the first Google result for ‘TextMate Rails test “-Itest”’.

I just opened up the copy of RubyMate that’s packed with the most current TM release, and patched it to work with Rails unit tests:

For TextMate.app/Contents/SharedSupport/Bundles/Ruby.tmbundle/Support/RubyMate/run_script.rb: http://pastie.textmate.org/private/gdlzq1pzkg1ocklma8izw

Obviously my Ruby knowledge outside a Rails context isn’t great, so I can’t say whether this would break tests outside of a Rails environment. (I don’t think it would, but if someone could confirm?)

And even though I’m using 2.1 final for my project rather than Edge, I added the new self.test method (from the aforementioned commit) to my test_helper (because it’s groovy) and TM is fine with it.

@ddemaree

Side note: TextMate already passes in the -I flag if it detects that the file you’re trying to execute is a test, and one of the ways it decides is to look for ‘test_helper’ somewhere in the document. TextMate is already trying to do the right thing, but it’s looking in the wrong places at least w/r/t this problem. (It checks ./lib and ./test/unit, but not ./test.)

@augustl

Another good reason to use the good ol’ relative require: Rails is supposed to be easy and hassle free. I don’t see the point with this change, when everything worked nicely before.

@nate

leethal +1

@jeremy
Owner

leethal, as I stated above: different relative requires of the same file cause Ruby to load it multiple times. This causes bugs. Fixing it is the point of this change. Now tests play well with Ruby’s load path tracking instead of causing subtle issues with constant collision and method redefinition.

TextMate has an online ticket tracker and a public mailing list. Please patch their ruby bundle, ideally relying on rake if available, and contribute your work.

@augustl

Which bugs? I haven’t had any problems with it whatsoever when running tests.

Also, I still can not run ‘ruby tests/units/foo_test.rb’ on the command line if the textmate bundle is fixed.

@NZKoz
Owner

The bugs it causes are to do with requiring test helper multiple times when you’re running tests. This causes things like method aliasing etc to break, and a bunch of warnings about constants being redefined.

Either way, as annoying as this is, it’s done. If you really want to avoid this you can:

export RUBYOPT=-Itest

@ddemaree

@leethal: The point of Rails isn’t to be easy or hassle-free, it’s to provide a set of useful tools with sensible defaults that reliably handle common problem cases. In other words: not easy, but ‘easy within certain parameters.’ The TextMate thing is frustrating (especially given TM’s loose understanding of the words “current”, “working” and “directory”), but I don’t see how having to add a 6-char flag to your ruby invocation breaks easy-ness.

@jeremy and @NZKoz: Thanks for taking the time to explain (and re-explain) the change.

@alk

How about something like this?

($: << File.expand_path(File.join(File.dirname(FILE), “..”))).uniq! require ‘test_helper’

@rsanheim

If you wrap the relative path with File.expand_path you don’t get double requires.

@lancecarlson

I and several other people I know think this is a completely unnecessary change and kills a lot of compatibility with several editors as well as confuses those users that use ruby .

@raul

+1 to old version: as rsanheim said, File.expand_path avoids double requires so where are the real benefits?

@augustl

The Rails core devs are starting to sound like debian core devs. Epeen ftl =/

@jeremy
Owner

rsanheim, File.expand_path doesn’t behave on Windows, unfortunately, or it would already be used.

lancecarlson, please read up for the reason for the change. I hope any confusion will abate quickly as those who use ruby directly either type -Itest, add a shell alias for same, add to RUBYOPT, or simply use the rake test task.

raul, please read up for the reason for the change.

leethal, you don’t need to be rude. You’ve made your opinion clear already. Thanks.

@augustl

@jeremy: honest or rude, your call.

So, have anyone done anything to fix the textmate issue yet?

@greatseth

I didn’t know about the -I option. I like this change. What is the big deal with requiring this extra command line argument? Add a shell alias or simple script to wrap invocation of the new, required options.

I gave up on the “run in TextMate” stuff a long time ago due to inconsistencies with the environment TextMate setup and that of running tests standalone and/or as part of the Rails test tasks.

-1 * detractors.size +1 * jeremy!

@NZKoz
Owner

@leethal: constructive comments or insults, your call.

The textmate bundle is open source and you can submit patches to them, feel free to have them contact us directly if they need more information.

@wpc

rsanheim + File.expand_path taking good care of it

@augustl

rsanheim, gogo, make a patch pretty please!

@Roman2K

I’m with jeremy: don’t use expand_path to avoid double requires. It would imply that any source file depending on another one would have to require it with the expanded path too, that’s ridiculous.

Why not simply adjust load paths somewhere and require relative paths everywhere (which is exactly what this change does)?

Then, to run a single test file without having to specify -I lib:test, add this to your ~/.bash_login: export RUBYLIB=“lib:test:$RUBYLIB”. It seems to work with RubyMate too (⌘R with the test file opened).

@ian

Fix without RUBYOPTS or -l (It’s ugly but whatever, it works)

Going off a suggestion from @ddemaree I mucked around with run_script.rb

On line 28 add “:..” so that it looks like: cmd.insert(1, “-I#{e_sh lib_path}:#{e_sh test_path}:..”)

Everything should be fine now. I realize it’s not optimal but it’s an individual test being run through TextMate, so whatever.

@cpjolicoeur

This whole discussion is crazy.

Commits to Rails core should not ever be made based on what someone’s editor of choice happens to be. If this breaks Textmate, that is Textmates problem. If it breaks other editors, it is the other editors problems. Those features where built into the editor based on the Rails code that existed at the time, so now they need to update their editors to comply with the Rails code at the present.

@jacobo

perhaps the convention for running tests should be changed:

just like you don't run rspec tests with ruby test/unit/some_test.rb you run then with spec

so maybe we should update test-unit to have it's own runner too (and the test runner should include test dir into load path by convention), so you would do

testunit test/unit/some_test.rb

@bmarini

Here's my lazy new practice from the command line:
cd test
ruby unit/some_test.rb

Please sign in to comment.
Something went wrong with that request. Please try again.