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?