twisted.trial.test.test_deferred doesn't follow the coding standard

[itamar]

twisted.trial.test.test_deferred should be improved so that e.g. all tests have docstrings, whitespace follows the coding standard etc..

[sreepriya]

This patch contains docstrings added for some of the functions in classes TestSetUp, TestNeverFire, TestTester in twisted.trial.test.test_deferred.

[wsanchez]

(In [37631]) Branch for 6050.

[wsanchez]

(In [37633]) Code cleanup, refs #6050 Submitted by: sreepriya

[wsanchez]

(In [37634]) _loadSuite is not a test. Wrap doc text. refs #6050.

[wsanchez]

This is nice cleanup, thanks.

I've branched and applied your path as-is in r37633.

A couple of small nits:

_loadSuite is not a test, so I suggest:

-        This testcase loads the test suite and accumulates the results
+        Load the test suite and accumulate the results

Also: the docstrings should ideally wrap to under 80 columns.

I've corrected those in r37634.

That gets a number of functions cleaned up, but there remain a few without docstrings, so I don't think we can consider this ticket complete. If you have time to start from this branch to finish up, that'd be great.

[sreepriya]

The patch includes docstrings added for rest of the test cases in test_deferred. All functions contains docstrings now.

[Julian]

Hi.

Thanks for working on this.

The docstrings for most of these don't really provide a description of the test, they just repeat the code within it. Someone reading the test method can see the code that the test runs, so a test docstring is meant to describe *what* the test is meant to test, not *how* it does so. For example, these tests are testing that trial properly interacts with deferreds from test methods. So e.g. TestDeferred.test_fail's docstring should say something like "The test is failed if the returned deferred fails." The fact that it calls runTest is really immaterial, and is an implementation detail of these tests. Someone wanting to know about that can read the code for that information.

I'd suggest taking a look at #6301 for some help there.

[sreepriya]

I had worked on the docstrings to make it more understandable and removed the implementaiton details.

[exarkun]

Thanks for your work on this so far. Here are a few more thoughts on the patch.

1. _loadSuite's docstring explains about half of what's going on. I'm not sure what "accumulate" means here, though, and the docstring could also explain that the test suite is loaded by finding all the tests on klass. Also, what is klass?
2. Test method docstrings should not start with "Test that ...". Just explain what behavior is expected.
3. The best test docstrings go some way towards explaining *why* instead of *what*. In the setUp docstring, the text is basically just an english translation of the Python. It would be better just to say that the default timeout is being lowered in order to make the tests complete more quickly. It's the "complete more quickly" part that is interesting.
4. Many of the other new docstrings fall into a similar trap, restating the Python code rather than explaining something.
   1. For example, TestDeferred.test_pass is verifying two things (it should really be two tests, but that's a matter for another ticket). It is verifying that TestResult.wasSuccessful returns True if no failing tests were run with that result object. Then, it is also verifying that testsRun counts how many passing tests are run with the result object. Something to notice here is that this is a test that *runs* a test. The inner test will always pass because it doesn't do anything that could ever make it fail (and don't confuse the two tests with each other, the inner test is defined in detest.py). The outer test verifies some behavior of the test runner, which it can reliably predict because it knows what the outcome of the inner test is (a priori).
   2. Also worth noting is that all of the tests in this module are for trial's runner's support of tests that return a Deferred. So actually writing that in a docstring somewhere would be a big help. detest.DeferredTests.test_pass passes by returnined a Deferred with a successful result. We want TestResult.wasSuccessful and TestResult.testsRun to treat that as a succeeding test, and that's the point of test_pass on test_deferred.TestDeferred.

This is a pretty tricky ticket, since it involves figuring out the intent of the author of some code that is now probably around ten years old. It may involve talking to some people who are more familiar with the code to learn that intent in order to write it up.

Again, thanks for tackling this.