Tidy tests #22
Tidy tests #22
Conversation
- make it use the base ArmstrongTestCase - move the setUp() and assertInContext() methods out of the base class and into the class they are used in - remove an unnecessary WellViewTestCase class
| def default_kwargs(self): | ||
| return { | ||
| "template_name": "index.html", | ||
| "well_title": self.well.title, | ||
| } | ||
|
|
||
| def assertInContext(self, var_name, other, template_or_context): |
There was a problem hiding this comment.
It's best to keep assert* methods like this outside of the test classes they're used. Invariably, you end up wanting to use them elsewhere if they're useful. If this is to be moved anywhere, it should be in to armstrong.dev so the method is available everywhere.
There was a problem hiding this comment.
For now should I move it back into _utils.py.TestCase?
There was a problem hiding this comment.
That's one option, or if you'd like to move this into armstrong.dev entirely that's a good option as well.
…Django TestCase in support test, and leaving that support test out of the package testing.
|
The |
|
halp! Not quite ready. Version conflicts. I'll update requirements.base. |
`armstrong.dev` now has the `assertInContext` method. `django-reversion` should be at 1.4, same as the package.json file and same as armstrong.core.arm_context
|
@tswicegood _before you merge_ This includes/builds-off the additional test added in this original PR #20. Merge that one first and this PR will get a little simpler. |
| def test_raises_NotImplementedError_on_exclude(self): | ||
| def test_raises_NotImplementedError_on_all_and_exclude(self): | ||
| merge_qs = MergeQuerySet(self.qs_a, self.qs_b) | ||
| with self.assertRaises(NotImplementedError): |
There was a problem hiding this comment.
Can you please break these out into multiple test methods? If this assertRaises failed, we wouldn't know that the next assertRaises also fails (or passes) until we fixed the first method. It's generally a good rule to keep one assertion per test method -- or at least one concern. Here, all() and exclude() are two different completely different methods that just happen to raise the same error.
| @@ -1,3 +1,3 @@ | |||
| django>=1.3 | |||
| django-reversion==1.3.3 | |||
| django-reversion==1.4 | |||
There was a problem hiding this comment.
Better change. Let's nix this file entirely and update requirements/dev.txt to not include it.
| add_n_random_images_to_well, | ||
| generate_random_image, | ||
| generate_random_story, | ||
| generate_random_well) |
There was a problem hiding this comment.
I prefer the previous style here, but whatever.
There was a problem hiding this comment.
Ditto. I've reverted some of these in joncotton#1 to keep it consistent with imports elsewhere. They should really either be one import, one line; or multi-line imports with () like this was originally.
|
This looks ready to go. There are a lot of different things going on in this PR that were hard to isolate. A PR for adding missing tests (like #22 was, but removed from this one), one for PEP 8 fixes, and one for code clarification would've been easier to sign off on. |
|
Well #20 was a separate PR just adding the tests. It didn't get merged by the time I wrote this. |
|
@joncotton sorry that didn't get merged in before this started. It's best to start all of your feature branches off of |
This is a better, cleaner version of this attempted PR #18.
It removes unused imports. It orders them in what I think is a cleaner way (you can tell me to stop this if you just think it's horrible). I found some new things too.
TestCasenow usesArmstrongTestCaseinstead ofDjangoTestCaseTestCasedid setUp() that was only used inSimpleWellViewTestso I moved those tasks overIt's meant as an extension to PR #20. So the actual diff is simpler. Here's the diff between those two.