Opened 2 years ago

Last modified 18 months ago

#6050 defect new

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

Reported by: itamar Owned by: sreepriya
Priority: normal Milestone:
Component: trial Keywords:
Cc: jml, sreepriya1111@… Branch: branches/cleanup-6050
(diff, github, buildbot, log)
Author: wsanchez Launchpad Bug:

Description

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

Attachments (3)

test_def1.patch (4.2 KB) - added by sreepriya 19 months ago.
This patch contains docstrings added for some of the functions in classes TestSetUp, TestNeverFire, TestTester in twisted.trial.test.test_deferred.
patch_2.patch (6.5 KB) - added by sreepriya 19 months ago.
The patch includes docstrings added for rest of the test cases in test_deferred. All functions contains docstrings now.
patch3.patch (6.4 KB) - added by sreepriya 19 months ago.
I had worked on the docstrings to make it more understandable and removed the implementaiton details.

Download all attachments as: .zip

Change History (18)

comment:1 Changed 2 years ago by DefaultCC Plugin

  • Cc jml added

comment:2 Changed 21 months ago by sreepriya

  • Cc sreepriya1111@… added
  • Owner set to sreepriya
  • Status changed from new to assigned

comment:3 Changed 19 months ago by ncollins

  • Owner changed from sreepriya to ncollins
  • Status changed from assigned to new

comment:4 Changed 19 months ago by sreepriya

  • Owner changed from ncollins to sreepriya

Changed 19 months ago by sreepriya

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

comment:5 Changed 19 months ago by sreepriya

  • Keywords review added
  • Owner sreepriya deleted

comment:6 Changed 19 months ago by wsanchez

  • Owner set to wsanchez
  • Status changed from new to assigned

comment:7 Changed 19 months ago by wsanchez

  • Author set to wsanchez
  • Branch set to branches/cleanup-6050

(In [37631]) Branch for 6050.

comment:8 Changed 19 months ago by wsanchez

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

comment:9 Changed 19 months ago by wsanchez

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

comment:10 Changed 19 months ago by wsanchez

  • Keywords review removed
  • Owner changed from wsanchez to sreepriya
  • Status changed from assigned to new

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.

Changed 19 months ago by sreepriya

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

comment:11 Changed 19 months ago by sreepriya

  • Keywords review added
  • Owner sreepriya deleted

comment:12 Changed 19 months ago by Julian

  • Keywords review removed
  • Owner set to sreepriya

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.

Changed 19 months ago by sreepriya

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

comment:13 Changed 19 months ago by sreepriya

  • Keywords review added
  • Owner sreepriya deleted

comment:14 Changed 18 months ago by exarkun

  • Owner set to exarkun
  • Status changed from new to assigned

comment:15 Changed 18 months ago by exarkun

  • Keywords easy review removed
  • Owner changed from exarkun to sreepriya
  • Status changed from assigned to new

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.

Note: See TracTickets for help on using tickets.