Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#7032 enhancement closed fixed (fixed)

Coding standard should state how to name test classes

Reported by: lvh Owned by: khorn
Priority: normal Milestone:
Component: core Keywords: documentation easy
Cc: Branch: branches/document-test-class-names-7032
branch-diff, diff-cov, branch-cov, buildbot
Author: khorn

Description

IIUC, all of the following exist in Twisted:

  • FooTests
  • FooTestCase
  • TestFoo

I really don't care which one it is, but we should pick one, and possibly file tickets for all the ones that don't match, once we've picked one.

Attachments (1)

doc_7032.patch (1.7 KB) - added by eeshangarg 2 years ago.

Download all attachments as: .zip

Change History (16)

comment:1 Changed 2 years ago by rwall

My preference is FooTests

Here's an example of a recently added test module which follows that style

  • source:trunk/twisted/python/test/test_tzhelper.py

comment:2 Changed 2 years ago by eeshangarg

  • Owner set to eeshangarg

comment:3 Changed 2 years ago by eeshangarg

Hi!

Here is what I think:

FooTestCase: There are currently 305 test classes that use this style, and there are three code examples in the Testing Standard itself that use this style.

TestFoo: There are 254 test classes that currently use this style, and this style of naming test classes does not appear in any kind of documentation(.rst).

FooTests: There are 209 test classes that use this style, and there are two code examples in the Testing Standard that use this style.

So, according to these numbers, I think it would be easier to modify the name of the test classes that use FooTests and TestFoo to use FooTestCase. And I think we will need to file separate tickets for the tests for each individual component of Twisted.

Also, one last thing, I think the code examples in the Testing Standard should also use the style that we choose to use.

I am really looking forward to working on this, please let me know what you think. :)

Thanks & regards, Eeshan Garg

comment:4 Changed 2 years ago by exarkun

Please use FooTests. Thanks.

comment:5 Changed 2 years ago by lvh

+1; please use FooTests. While the numbers do point out a "winner" they don't mention how old that relative code is. I think we all use FooTests now.

It should(?!) be fairly easy to do this with a regex over the codebase (right?) in which case there's little difference between changing any two to the remaining, blessed style.

Separate tickets for each component sounds fine to me. Also means that we don't block for a long time because this one thing somewhere depends on a particular test having a particular name (it shouldn't; but I wouldn't be surprised if a few examples pop out of the woodwork).

Thanks for working on this, Eeshan!

Changed 2 years ago by eeshangarg

comment:6 Changed 2 years ago by eeshangarg

  • Keywords review added
  • Owner changed from eeshangarg to lvh

Hi! I just submitted a patch which adds guidelines to the Testing standard about how to name test classes. I also modified the code examples in the testing standard to use the new style. I asked on #twisted-dev and I was advised that the "Test Implementation Guidelines" section of the testing standard would be an appropriate place for the new guidelines.

Please let me know if further changes are needed and I'll make them right away.

Thanks for letting me contribute to my favorite project!

Regards, Eeshan Garg

comment:7 Changed 2 years ago by exarkun

Thanks. It would also be nice if this rule was codified in twistedchecker (hardly anyone reads the full contribution guidelines but it's easy to point people at a tool that will find their style mistakes automatically).

This is not at all a blocker for resolving this ticket but it would be a nice enhancement related to this change.

comment:8 Changed 2 years ago by eeshangarg

Hi! I just filed a new ticket for tests in twisted.words. I have already submitted a patch for it and I have also added the review keyword. My patch changes all tests in twisted.words to use FooTests now. I will try to submit patches for all of the other tests as soon as I can. :)

https://twistedmatrix.com/trac/ticket/7622

Thanks! Eeshan

comment:9 Changed 2 years ago by khorn

  • Owner changed from lvh to khorn

comment:10 Changed 2 years ago by khorn

  • Author set to khorn
  • Branch set to branches/document-test-class-names-7032

(In [43073]) Branching to document-test-class-names-7032.

comment:11 Changed 2 years ago by khorn

(In [43074]) Apply contributed patch to document the naming of test classes in Twisted. Refs #7032

comment:12 Changed 2 years ago by khorn

This looks good. Merging.

comment:13 Changed 2 years ago by khorn

  • Resolution set to fixed
  • Status changed from new to closed

(In [43075]) Merge document-test-class-names-7032: Document the recommended naming of test classes in Twisted.

Author: eeshangarg Reviewers: khorn Fixes: #7032

Add some documentation to the Testing Standard which specifies the correct way of naming test classes.

comment:14 Changed 2 years ago by khorn

  • Keywords review removed

comment:15 Changed 2 years ago by exarkun

Also from the coding standard:

Documentation should be formatted with a single sentence or clause per line.

(which perhaps reveals how worthwhile it is to add tons of niggling little gotchas like this to the coding standard).

Last edited 2 years ago by exarkun (previous) (diff)
Note: See TracTickets for help on using tickets.