Opened 2 years ago

Closed 20 months ago

Last modified 20 months 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
(github, patch, buildbot, log)
Author: khorn Launchpad Bug:

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 21 months ago.

Download all attachments as: .zip

Change History (16)

comment:1 Changed 22 months ago by rwall

My preference is FooTests

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

comment:2 Changed 21 months ago by eeshangarg

  • Owner set to eeshangarg

comment:3 Changed 21 months 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 21 months ago by exarkun

Please use FooTests. Thanks.

comment:5 Changed 21 months 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 21 months ago by eeshangarg

comment:6 Changed 21 months 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 21 months 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 20 months 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 20 months ago by khorn

  • Owner changed from lvh to khorn

comment:10 Changed 20 months 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 20 months ago by khorn

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

comment:12 Changed 20 months ago by khorn

This looks good. Merging.

comment:13 Changed 20 months 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 20 months ago by khorn

  • Keywords review removed

comment:15 Changed 20 months 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 20 months ago by exarkun (previous) (diff)
Note: See TracTickets for help on using tickets.