On Jul 4, 2011, at 8:01 AM, Jacob Kaplan-Moss wrote: > On Mon, Jul 4, 2011 at 8:54 AM, Jim Dalton <jim.dal...@gmail.com> wrote: >> I've created a ticket for this and uploaded a patch: >> https://code.djangoproject.com/ticket/16401 >> Please feel free to review and let me know if this is a good idea in the >> first place, as well as if the patch makes sense and is the right approach. > > Hm. > > While I think the feature's a good idea (I've been stung by tests > running against a live cache) I can't say I'm particularly thrilled by > the approach here. Monkeypatching is acceptable in situations like > these, but there's a limit... and I think patching five methods is > starting to reach it. Remember: as much as possible, tests should > operate just like production. Any difference, no matter how small, > introduces the worst kind of bugs: ones that only show up in > production.
Yeah, I agree with you in principle there. I'm definitely an advocate of avoiding monkey patching like the plague in production code. While I also agree that this code looks a bit hairy when you sort of squint your eyes at it, if you actually look closely at what's going on, we're not modifying things in any way that's tricky or complicated. Really, the only two things we are doing are adding a bit of code that that tracks keys and then swapping a few methods out. Pretty similar to what we do already with Template.render -- just x5. The monkey patching stuff is not modifying any arguments or results, nor is it changing the observable behavior of the modified methods. The tests demonstrate this pretty thoroughly. The only place I've managed to think of where this is different from production is in the length of the key. Because we are appending "_test_" to the start of KEY_PREFIX, you're going to get 6 less characters in your key than you would otherwise. It *is* a difference, so I don't want to downplay it, but at the same time it's a difference where you'll get an error in testing that you wouldn't get in production -- which is better than the opposite. The only other place where someone might run into trouble is if they themselves were monkey patching the cache and their changes collided somehow. I understand that this is the kind of thing that happens in Ruby a lot, and it strikes me as unpleasant. Anyhow, that's not to say that there isn't a better solution or that your proposal below isn't preferable (more on that in a sec), but I'd argue that my patch is at least a plausible solution, i.e. it works in lieu of a better alternative. > I have an alternate proposal: > > Instead of using the default cache defined in CACHES, use a "test > cache." This can safely be flushed, meaning that no monkeypatching is > needed. The added benefit here is that this works similarly to the > database engine, so it'll be fairly easy to understand. > > Now, unlike DB test we can't just prefix "test_" to the database name > (because we haven't got one), nor can we prefix keys (because not all > cache backends can do something like "delete test:*"). So I'd suggest > we look for a "test" backend defined in CACHES and fall back to a > local memory cache if not defined. > > I think this simplifies things immensely, and also gives a fair bit of > control -- if I want to test against a real cache backend, I just > define CACHES['test'] and roll. So, in many ways I like this idea. I agree that the implementation is going to be more straightforward. Here's just a quick list of concerns or possible drawbacks I was able to think of: * Not sure how to handle multiple cache backends. I imagine if you're someone who has implemented multiple cache backends, then you are using them in specific ways for specific reasons. If your tests end up sharing a test backend, that could result in unpredictable behavior. One possible resolution would be to amend your proposal a bit: For each cache backend defined, we swap it out with the locmem backend unless you have yourself defined a test_CACHE_NAME version of that cache. So e.g. fi I have three backends named "default", "file" and "db", I then need to define test_default, test_file and test_db if I want to specify how the test framework is going to set up the test cache. Otherwise, it'll fall back to replacing it with a locmem cache. * At least when it comes to memcached, this doesn't help you much with the cache.clear() issue, since cache.clear() will blank your entire memcached cache. So if we're calling cache.clear() at the end of each test and it's blanking the whole cache, I start to wonder why we bother at all? In other words, why not just call cache.clear() at the end of every test and call it a day, and make a note in the documentation that running the test will clear any cache you have defined in BACKENDS, so use it with caution. One response is that the same thing is not true for db, file and locmem -- in these cases you'll only be deleting the test cache which was set up for them and leaving everything else intact. But IDK, I mean I get the sense that memcached is what most people are using for caching in production, so to me it's far and away the most import cache to consider. Those were my major concerns, though I'm not sure any of them are fatal at least from my perspective. At present our choices boil down to: 1. Monkey patch cache, keep track of set keys, flush set keys at the end of each test. 2. Default to locmem cache, use a 'test' cache (or possibly 'test_CACHE_NAME') instead if found. cache.clear() on test after each test. 3. Just cache.clear() after every test, note in docs, call it a day. 4. Do nothing i.e. keep status quo. Speaking personally, any of 1-3 are better than #4. I also wonder if #3 is worth considering. It's certainly the easiest, and part of me wonders if objections to it are purely hypothetical. Anyhow, thanks for giving me your feedback Jacob and obviously thanks for your alternative proposal. I'd be happy to pitch in and work on that idea if that's what you guys all lean toward. In the meantime I'd be interested in other opinions as well. Cheers, Jim -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-developers@googlegroups.com. To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
