For helpers / small functions doesn't hurt, and for models is still one class I don't understand why you see multiple classes here.

It's one test class per function or per class, not per method.

I kinda have strong feelings about this and I believe we need to stick to one rule, otherwise what's big function or not can be easily misinterpretted.


----- Original Message -----
From: "Andrei Hajdukewycz" <notifications@github.com>
To: "mozilla/mozillians" <mozillians@noreply.github.com>
Cc: "Giorgos Logiotatidis" <giorgos@mozilla.com>
Sent: Wednesday, January 29, 2014 8:42:32 PM
Subject: Re: [mozillians] Fix bug 938005 - Display current time in profile user's timezone (#765)

> +        with patch('mozillians.common.helpers.aware_now') as mock_aware_now:
> +            mock_aware_now.return_value = utc_time
> +            result = helpers.now_in_timezone(tz_name)
> +        ok_(is_aware(result))
> +        fmt_time = result.strftime("%H:%M %Z")
> +        eq_('07:04 EST', fmt_time)
> +
> +    def test_offset_of_timezone(self):
> +        # offset_of_timezone returns the offset in minutes of the named timezone as of now
> +        # Construct a time in UTC that will be "now"
> +        utc_time = datetime.datetime(1972, 1, 1, 3, 4, 5).replace(tzinfo=pytz.utc)
> +        tz_name = "US/Eastern"  # 5 hours difference from UTC on 1/1/1972
> +        with patch('mozillians.common.helpers.aware_now') as mock_aware_now:
> +            mock_aware_now.return_value = utc_time
> +            result = helpers.offset_of_timezone(tz_name)
> +        eq_(-300, result)

I think that convention makes sense for views, since one view function does a lot of stuff and may need a bunch of different tests, but I don't think it makes sense for things like helper tests where each function does one thing.

And in the case of models, it would lead to very strange things like having multiple test classes for one model class, when it clearly makes more sense to have one test class per one model class. In fact, I think a better general rule is one class: one test class. The only reason this doesn't work for views is that we aren't using class based views, so our views.py end up as a bunch of really big functions. Class-based views are probably better for organization, but I don't think changing all of our views to class-based is worthwhile...

Bottom line is, I don't think it's worthwhile to pad every test file with one-class-per-function-tested, UNLESS you are testing really big functions with multiple tests. In that case, absolutely it makes sense.

---
Reply to this email directly or view it on GitHub:
https://github.com/mozilla/mozillians/pull/765/files#r9278280