carpentries / amy

A web-based workshop administration application built using Django.
https://amy.carpentries.org
MIT License
114 stars 72 forks source link

Use test class assertions instead of raw assert? #157

Closed gvwilson closed 9 years ago

gvwilson commented 9 years ago

Should we use assertion methods inherited from Django's TestCase class or raw Python assert statements?

wking commented 9 years ago

On Tue, Jan 20, 2015 at 06:01:07PM -0800, Greg Wilson wrote:

Should we use assertion methods inherited from Django's TestCase class or raw Python assert statements?

I think so, since you'll get automatically generated messages explaining why the test failed. You can write that out by hand, but I think that's just extra code to maintain (e.g we currently hard-code the expected value in the error string here 1, which is not very DRY).

abought commented 9 years ago

+1 to clear reporting.

Additionally, some behaviors (eg exceptions) are difficult to test using bare assertions, whereas Testcase offers functionality such as self.assertRaises. Might as well use the capabilities of the framework in a consistent manner.

pbanaszkiewicz commented 9 years ago

Personally I prefer py.test with assert statements over Java-ish unittest TestCases.

Having assertions in my tests seems more natural.

Additionally I have test marking (you can, for example, run only tests marked as "regression" or "fast"), parametrization (no more for-loops in tests), or even test parallelization (for testing on multiple cores).

The are some downsides, too, like loosing client and response objects…

Anyway, I know we're not considering switching to py.test, but that's only my 2 cents on "assert vs self.assert*".

wking commented 9 years ago

On Thu, Jan 22, 2015 at 03:22:03PM -0800, Piotr Banaszkiewicz wrote:

Additionally I have test marking…,

You can do this in Python ≥3.1 with skipIf 1.

parametrization …,

You can do this in Python ≥3.4 with subTest 2.

or even test parallelization…

This isn't possible with the current unittest implementation, but since it's just a speed issue, I don't consider it to be a big enough loss to be worth requiring an external testing library.

pbanaszkiewicz commented 9 years ago

Hey @wking,

You can do this in Python ≥3.1 with skipIf [1].

Skipping and conditional skipping is only a small subset of what you can do with tests marking.

You can do this in Python ≥3.4 with subTest [2].

That's a horrible syntax… But I'm glad this feature has finally landed in mainline Python.

This isn't possible with the current unittest implementation, but since it's just a speed issue, I don't consider it to be a big enough loss to be worth requiring an external testing library.

In my opinion, py.test is worth considering. Here are prons:

However there are some cons, too:

Personally I really like using py.test and highly recommend it.

This thread is turning into off-topic "vim vs emacs" kind of war, so let's say that if we stick to unittest we should use unittest-style testing with .assert* methods; otherwise (in case of switching to py.test) we should use Python assertions.

gvwilson commented 9 years ago

How's this as a new rule? Every post on this thread must be accompanied by at least one test case for a part of Amy that isn't currently exercised. First up: testing CSV upload... :-)

pbanaszkiewicz commented 9 years ago

First up: testing CSV upload... :-)

Yeah, I'm waiting for https://github.com/pbanaszkiewicz/amy/pull/3 (@sburns) :)

gvwilson commented 9 years ago

Do you need a review?

pbanaszkiewicz commented 9 years ago

I need to know if @sburn is going to finish https://github.com/pbanaszkiewicz/amy/pull/3, otherwise I can do it.

Then I'll send a ready for review PR.

wking commented 9 years ago

On Fri, Jan 23, 2015 at 03:57:09AM -0800, Piotr Banaszkiewicz wrote:

This thread is turning into off-topic "vim vs emacs" kind of war, so let's say that if we stick to unittest we should use unittest-style testing with .assert* methods; otherwise (in case of switching to py.test) we should use Python assertions.

Works for me. I think some tooling debate is good though. For example, I wasn't aware of the parallel py.test / serial unittest distinction before you mentioned it.

I think the best argument for sticking with unittest is that it's in the standard library, and I don't think we're doing anything exciting enough in our testing to justify the bells and whistles of an extra dependency (which is the same argument I made against using requests

118). Tracking another external dependency is fine (like we'll be

doing with DataTables after #134, but I'd rather have stronger motivation (“sortable tables are much more usable than unsortable tables”) before adding a given dependency.

pbanaszkiewicz commented 9 years ago

A few months have passed and what we see in the test suite is that we often use assert statement and only go back to unittest methods when necessary. This brings a conclusion that we should look into py.test in the future.

However, I don't think anyone would be willing to rewrite >80 tests (well py.test claims to be supporting Django TestCases easily, but we'd need to check). So for now I'm closing this issue.