Opened 5 years ago

Last modified 13 months ago

#6050 defect new

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

Reported by: Itamar Turner-Trauring Owned by:
Priority: normal Milestone:
Component: trial Keywords:
Cc: Jonathan Lange, sreepriya Branch: branches/cleanup-6050
branch-diff, diff-cov, branch-cov, buildbot
Author: wsanchez

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 4 years 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 4 years 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 4 years ago.
I had worked on the docstrings to make it more understandable and removed the implementaiton details.

Download all attachments as: .zip

Change History (19)

comment:1 Changed 5 years ago by DefaultCC Plugin

Cc: Jonathan Lange added

comment:2 Changed 4 years ago by sreepriya

Cc: sreepriya added
Owner: set to sreepriya
Status: newassigned

comment:3 Changed 4 years ago by Nick Collins

Owner: changed from sreepriya to Nick Collins
Status: assignednew

comment:4 Changed 4 years ago by sreepriya

Owner: changed from Nick Collins to sreepriya

Changed 4 years ago by sreepriya

Attachment: test_def1.patch added

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

comment:5 Changed 4 years ago by sreepriya

Keywords: review added
Owner: sreepriya deleted

comment:6 Changed 4 years ago by Wilfredo Sánchez Vega

Owner: set to Wilfredo Sánchez Vega
Status: newassigned

comment:7 Changed 4 years ago by Wilfredo Sánchez Vega

Author: wsanchez
Branch: branches/cleanup-6050

(In [37631]) Branch for 6050.

comment:8 Changed 4 years ago by Wilfredo Sánchez Vega

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

comment:9 Changed 4 years ago by Wilfredo Sánchez Vega

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

comment:10 Changed 4 years ago by Wilfredo Sánchez Vega

Keywords: review removed
Owner: changed from Wilfredo Sánchez Vega to sreepriya
Status: assignednew

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 4 years ago by sreepriya

Attachment: patch_2.patch added

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

comment:11 Changed 4 years ago by sreepriya

Keywords: review added
Owner: sreepriya deleted

comment:12 Changed 4 years ago by Julian Berman

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 4 years ago by sreepriya

Attachment: patch3.patch added

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

comment:13 Changed 4 years ago by sreepriya

Keywords: review added
Owner: sreepriya deleted

comment:14 Changed 4 years ago by Jean-Paul Calderone

Owner: set to Jean-Paul Calderone
Status: newassigned

comment:15 Changed 4 years ago by Jean-Paul Calderone

Keywords: easy review removed
Owner: changed from Jean-Paul Calderone to sreepriya
Status: assignednew

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.

comment:16 Changed 13 months ago by Wilfredo Sánchez Vega

Owner: sreepriya deleted

Removing assignee

Note: See TracTickets for help on using tickets.