Broken/Conflicting Tests in ActionPack

I'd like to get to work patching ActionPack, but there's an
unfortunate annoyance stopping me for now.

I've described it here: http://dev.rubyonrails.org/ticket/8849

The gist of it is that render_test.rb and new_render_test.rb define
conflicting types for Customer, and thus effect each other. I haven't
fixed this myself because I'd like to know the intent here, and what
the proper solution is. Is render_test.rb just old code that should
be thrown away or merged into new_render_test.rb? Or should we
instead focus on encapsulating their individual definitions of
Customer?

I'd like someone to look at this before I go proposing the changes.

This sounds an awful lot like http://dev.rubyonrails.org/ticket/8714.

- Matt

Oh goodie, so this is being taken care of after all? I hope my
redundant report has drawn some more attention to this. I mean, with
the emphasis on test-based development, the tests should be rather
important, yes?

I hope this patch, or a better fix, is adopted soon!

> This sounds an awful lot likehttp://dev.rubyonrails.org/ticket/8714.

Seems it should be relatively straightforward to extract the
definition to a file which is required by the tests which need it?
I'd be happy to apply a patch which did that.

As for the more hypothetical problem of tests messing with each other
and causing problems. Any patches which fix real problems caused by
that will be gladly applied.

Argh, this isn't the only problem. Try checking for namespace
intrusions in the other tests. Things like "Module Fun" sneak in and
modify other things. This whole testing suite needs some serious
refactoring. I don't think rake's tests were ever meant to have
anything outside of the test class in the same file.

Looking at the way things are arranged, there's a lot of incompatible
duplication. Also, I noticed there's a controller_fixtures directory
that is not even being used. Wouldn't this be the prime place to put
examples of controllers that would be used in the tests, instead of
defining those controllers in the tests themselves?

Argh, this isn't the only problem. Try checking for namespace
intrusions in the other tests. Things like "Module Fun" sneak in and
modify other things. This whole testing suite needs some serious
refactoring. I don't think rake's tests were ever meant to have
anything outside of the test class in the same file.

Looking at the way things are arranged, there's a lot of incompatible
duplication. Also, I noticed there's a controller_fixtures directory
that is not even being used. Wouldn't this be the prime place to put
examples of controllers that would be used in the tests, instead of
defining those controllers in the tests themselves?

What are the practical implications of these issues that you're
describing? I guess I'm just not following you?

The tests could certainly be broken up to have all the controllers
defined in seperate files. However they're still going be run in one
interpreter.

Sold. I'll work up a patch that meets with your approval. Any particular
place you would suggest that file should go? I'm really unfamiliar with the
Rails codebase, so I don't know where a good place for this file would be.
The best I can find through a quick trawl is
actionpack/test/controller/controller_fixtures/app/models, but there's no
directory there currently and it seems a bit presumptuous to be creating new
directories with my first real patch. <grin>

If you can let me know where the best place to put this pseudo-model is,
I'll get the patch sorted out ASAP.

- Matt

What are the practical implications of these issues that you're
describing? I guess I'm just not following you?

We're talking about how some test files are defining identifiers in
the global namespace which are affecting each other, possibly causing
different results when the tests are run alone or in a different
order. In this case it's just repeated definitions which need to be
placed elsewhere. But, it seems like there should be a hard and fast
rule in testing against placing anything mutable in the global
namespace, lest another test reference it and get different results.

The tests could certainly be broken up to have all the controllers
defined in seperate files. However they're still going be run in one
interpreter.

Yes, but as long as they're all using the same controller, rather than
using different ones that could clobber each other differently based
on test order and cause undefined results, I'd count it as an
improvement. But truly, finding a way to purge this information
between tests would be quite beneficial!

The best I can find through a quick trawl is
actionpack/test/controller/controller_fixtures/app/models, but there's no
directory there currently and it seems a bit presumptuous to be creating new
directories with my first real patch. <grin>

Hehe, go ahead. I've been meaning to tear into this source tree,
though I'm a little timid about submitting monster patch files that
might break other people's pending patches. What would be really nice
would be for someone with cvs write access to take care of this, after
skimming through the ticket tracker and applying any relevant
actionpack/test-related patches.

The best I can find through a quick trawl is
actionpack/test/controller/controller_fixtures/app/models

How about creating a file in the same directory where render_test.rb
is located (i.e. actionpack/test/controller)? There's a file there
fake_controllers.rb that's used by 4 test files:
assert_select_test.rb, routing_test.rb, selector_test.rb,
test_test.rb. Seems to me like this is just a similar case of
refactoring (so maybe create a fake_models.rb?). This is just a
suggestion though as I'm still new at this and still trying to make my
first patch. What do you guys think?

I hadn't noticed the fake_controllers.rb, since I usually don't put anything
but *_test.rb in my tests directory. That looks like a good precedent,
though, so I'll rework my patch in that direction and Koz can tell me if
it's bodgy. <grin>

- Matt

Koz, a patch has been made for ticket #8714 that extracted the
definition to a file as described by Matt in the recent posts. Can you
please check it and comment if you still find any problems with it?