Opened 5 years ago

Last modified 4 years ago

#6484 enhancement new

Add unit tests for twisted.python.dist.get_setup_args

Reported by: Thijs Triemstra Owned by: mulhern
Priority: normal Milestone:
Component: core Keywords: easy
Cc: Thijs Triemstra Branch: branches/get_setup_args-tests-6484
branch-diff, diff-cov, branch-cov, buildbot
Author: tomprince

Description (last modified by Thijs Triemstra)

get_setup_args in twisted.python.dist is not completely covered by unit tests.

Attachments (2)

my-twisted-patch.patch (4.1 KB) - added by mulhern 5 years ago.
my-twisted-patch.2.patch (6.5 KB) - added by mulhern 5 years ago.

Download all attachments as: .zip

Change History (13)

comment:1 Changed 5 years ago by mulhern

Two tests for get_setup_args already exist in class SetupTest.

Possible fix is to rename class SetupTest to GetSetupArgsTest as only tests in this class are for get_setup_args.

Also, unit testing for get_setup_args not completely straightforward as it expects a context where twisted directory is a subdirectory of the current directory when one of its keyword arguments is twisted_subproject.

comment:2 Changed 5 years ago by Itamar Turner-Trauring

Things like "should I rename this class" can be addressed by reviewer given some code to review, especially when it's not too significant. E.g. API design is worth discussing up front, but this sort of thing just submit a patch and see what the reviewer says.

You can find reusable code for creating directories for testing purposes in twisted.python.test.test_release.StructureAssertingMixin.

comment:3 Changed 5 years ago by Itamar Turner-Trauring

If the existing tests provide full test coverage (sounds like they don't), you can close this ticket as invalid.

comment:4 Changed 5 years ago by mulhern

Owner: set to mulhern
Status: newassigned

Changed 5 years ago by mulhern

Attachment: my-twisted-patch.patch added

comment:5 Changed 5 years ago by mulhern

Keywords: review added
Owner: mulhern deleted
Status: assignednew

Added a few more tests for get_setup_args and changed test class to GetSetupArgsTest to more fully reflect what it does.

comment:6 Changed 5 years ago by Thijs Triemstra

Description: modified (diff)
Keywords: review removed
Owner: set to mulhern

Thanks for your patch mulhern. After applying your patch and running coverage it turns out there are 3 lines still not tested:

  1. the RuntimeError on line 85nice
  2. if os.path.exists(plugin): on line 94
  3. if 'cmdclass' not in kw: on 101
  4. if 'ext_modules' not in kw: on 118

Can you check if you can add coverage for these lines as well, especially point 1 (others are indirectly covered it seems but would be good to have full coverage)? Thanks.

Changed 5 years ago by mulhern

Attachment: my-twisted-patch.2.patch added

comment:7 Changed 5 years ago by mulhern

Keywords: review added
Owner: mulhern deleted

Coverage tool shows all lines covered.

comment:8 Changed 4 years ago by Tom Prince

Author: tomprince
Branch: branches/get_setup_args-tests-6484

(In [38760]) Branching to get_setup_args-tests-6484.

comment:9 Changed 4 years ago by Tom Prince

(In [38761]) Apply my-twisted-patch.2.patch from mulhern.

Refs: #6484

comment:10 Changed 4 years ago by Tom Prince

Owner: set to mulhern

Thanks for your contribution.

  1. test_Plugins and test_TwistedSubproject<n> don't follow the naming convention: the captilization should be test_plugins and test_twsitedSubproject<n>.
  2. test_TwistedSubproject<n>:
    1. These should have more meaninful names.
    2. ...1 and ...3 are failing for me (it appears due to ordering).
    3. The docstrings could be clearer:
      • "several other arguments": what arguments, and how are there values determined
      • "regarles of the value of L{plugins}": I would be inclined to say that C{plugins} is ignored if twisted_subproject is specified.
    4. I don't see any tests that the value of plugins is actually ignored: all the test actually pass an empty value.
  3. whitespace
    • there is a bunch of trailing whitespace (I've fixed this)
    • there should be exactly two lines between methods.

Please resubmit for review after addressing the above points.

comment:11 Changed 4 years ago by Tom Prince

Keywords: review removed
Note: See TracTickets for help on using tickets.