Opened 23 months ago

Last modified 15 months ago

#6137 enhancement new

Add unit tests for twisted.python.dist.getAllScripts

Reported by: exarkun Owned by: introom
Priority: normal Milestone:
Component: core Keywords:
Cc: thijs Branch: branches/test-scripts-6137-2
(diff, github, buildbot, log)
Author: meissenPlate, introom, tomprince Launchpad Bug:

Description

It used to live in setup.py, where it was impossible to unit test anything, but now it lives in twisted.python.dist where it could totally be unit tested, so it should be.

Attachments (4)

ticket_6137_1.diff (3.0 KB) - added by meissenPlate 22 months ago.
Unit test for twisted.python.dist.getAllScripts + topfile entry.
ticket_6137_2.diff (4.1 KB) - added by meissenPlate 21 months ago.
Revised patch adding test + topfile entry.
ticket_6137_3.diff (4.5 KB) - added by meissenPlate 21 months ago.
6137_introom.patch (1.5 KB) - added by introom 17 months ago.

Download all attachments as: .zip

Change History (23)

comment:1 Changed 22 months ago by meissenPlate

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

comment:2 Changed 22 months ago by meissenPlate

  • Owner meissenPlate deleted
  • Status changed from assigned to new

Here's a unit test for getAllScripts.

I mocked out getScripts to avoid testing it indirectly and making this all more complicated. getScripts already has good coverage.

I had to perform some shenanigans with the current working directory, because getAllScripts wants to assume it's in a very specific location. In the process I learned that Trial does NOT reset the CWD after each test suite. You know you've done something wrong when you write a test that breaks all the other tests.

By the way, I don't think there was much danger of getAllScripts going wrong in the first place. It's a simple list comprehension with some decoration. The real potential for mess-ups is in getScripts, which is well tested, and in twisted.python.dist.twisted_subprojects, if someone ever forgets to update that list when a new script is added. But anyway, it has a test now.

Changed 22 months ago by meissenPlate

Unit test for twisted.python.dist.getAllScripts + topfile entry.

comment:3 Changed 22 months ago by meissenPlate

  • Keywords review added

comment:4 Changed 22 months ago by therve

  • Keywords review removed
  • Owner set to meissenPlate
  1. You can use addCleanup instead of implementing tearDown.
  1. There are too many "should" in the test docstring: try to describe behavior.
  1. Instead of using sorted() for the assert, I'd rather use set(), which makes it more clear that order doesn't matter.
  1. The isdir check is not tested.
  1. Please use camelCase for variables.

Thanks!

comment:5 Changed 21 months ago by meissenPlate

  • Keywords review added

Sorry for the slow response. The semester ended and suddenly I was busy. Probably not so busy that I couldn't have redone this patch at least, but I delayed anyway.

  1. Ah, good idea! "self.addCleanup(os.chdir, oldCWD)". Pretty slick.
  1. Fixed. Two of the remaining "should"s are from the perspective of getScripts, which tries to enumerate the scripts that should be included in a distribution. The other two are in comments. They could be replaced with "will", but that sounds much weirder inside the test than inside the docstring. Inside the docstring "will" sounds just fine. I'll change the comments as well if you recommend it though, I don't know Twisted's documentation voice very well yet.
  1. Can't use set(), set() wouldn't detect duplications. I used collections.Counter() instead. Looks better than sorted(), thanks :).
  1. You are quite right. getAllScripts ignores files in bin/ that happen to have the same name as a Twisted subproject listed in twisted_subprojects. The test now tests for this behavior.
  1. Fixed. Sorry about that.

Changed 21 months ago by meissenPlate

Revised patch adding test + topfile entry.

comment:6 Changed 21 months ago by meissenPlate

  • Owner meissenPlate deleted

comment:7 Changed 21 months ago by exarkun

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

comment:8 Changed 21 months ago by exarkun

  • Keywords review removed
  • Owner changed from exarkun to meissenPlate
  • Status changed from assigned to new
  1. Use [] to create a new empty list, not list().
  2. mock_getScripts isn't a mock. See (eg) http://msdn.microsoft.com/en-us/magazine/cc163358.aspx - it's more like a stub.
  3. Also, mock_getScripts doesn't follow the naming convention. mockGetScripts (or stubGetScripts would be better.
  4. A problem with testing like this is that the test doesn't actually demonstrate getAllScripts works, since getScripts and mock_getScripts may not actually behave compatibly. This is where "verified fakes" come into play. A recent thread on twisted-python discussed this a bit: <http://twistedmatrix.com/pipermail/twisted-python/2012-October/026155.html>. I don't think you necessarily need to do anything about this as part of this ticket, but it suggests some future work to do for this test module, and is something to think about in the future when writing tests.
  5. twisted.python.filepath should be preferred over os.path where possible. So, at least in the new test method, please use FilePath instead of os.path.
  6. Some of the comments in the test use the word "right" or "correct". Instead, they should say what is correct. It's usually self-evident that code in a test is looking for something "correct". What's really useful is an explanation to a future maintainer of why that result is correct. For example, instead of Check that getAllScripts called getScripts on the right directories., write getAllScripts should call getScripts on each directory which corresponds to a subproject, and also on the current directory (for the core subproject).
  7. Vertical whitespace - separate methods by two blank lines and classes by three.

Thanks!

comment:9 Changed 21 months ago by meissenPlate

  • Keywords review added
  • Owner meissenPlate deleted
  1. Fixed.
  1. Based on that article, maybe even "spy", but no one else calls it that. It's now stubGetScripts.
  1. I left the underscore there initially to make it clear I was just sticking a tag ("mock", now "stub") on a preexisting function name ("getScripts"). Sorry about that. Fixed.
  1. I think I get it. Verified fakes are fakes with their own unit tests that make sure they act convincingly like the real thing. I didn't make any changes to the unit tests here based on this information. If getScripts weren't used in twisted/topfiles/setup.py as well as getAllScripts then I would have said that the right thing to do would be to just test them together.
  1. Fixed. I didn't find a replacement for os.chdir though.
  1. That makes sense. I poked at the comments for a while, and I think they are better now.
  1. Opps! Fixed.

I didn't see anything in the standard about the spacing between the docstring and that class level mega-comment, so I've got double blank lines there too. Seems sensible to me.

Changed 21 months ago by meissenPlate

comment:10 Changed 20 months ago by glyph

  • Author set to glyph
  • Branch set to branches/test-scripts-6137

(In [36751]) Branching to 'test-scripts-6137'

comment:11 Changed 20 months ago by glyph

I created a branch and forced a build to make the reviewer's job easier.

comment:12 Changed 20 months ago by thijs

  • Cc thijs added
  • Keywords review removed
  • Owner set to meissenPlate

The builds failed for python2.6 because of collections.Counter, which is not available on that platform (and Twisted still supports that version).

comment:13 Changed 17 months ago by introom

  • Owner changed from meissenPlate to introom
  • Status changed from new to assigned

Changed 17 months ago by introom

comment:14 Changed 17 months ago by introom

  • Keywords review added
  • Owner introom deleted
  • Status changed from assigned to new

addressed incompatibility in py2.6 by defining a helper function named getCount instead of directly using collections.Counter

comment:15 Changed 17 months ago by thijs

(In [38215]) apply 6137_introom.patch, add news file. refs #6137

comment:16 Changed 17 months ago by thijs

  • Author changed from glyph to meissenPlate, introom

Thanks for your patch. Forced a new build.

comment:17 Changed 15 months ago by tomprince

  • Author changed from meissenPlate, introom to meissenPlate, introom, tomprince
  • Branch changed from branches/test-scripts-6137 to branches/test-scripts-6137-2

(In [38729]) Branching to test-scripts-6137-2.

comment:18 Changed 15 months ago by tom.prince

  • Owner set to introom

Thanks for your contribution. A couple of minor points:

  1. setUp should have a docstring describing what it does.
  2. The docstring for test_getsCorrectScripts says "the scripts [...] that should be included". But it doesn't explain what scripts should be included.
    • I think integrating the class-level comment into the docstring might address this.
  3. The two helper functions in test_getsCorrectScripts (stubGetScripts and getCount) both have a comment obliquely describing what they do. It would be better if they were turned into proper docstrings, that directly describe what they do.
  4. Currently, stubGetScripts always returns the same thing, regardless of its arguments. In reality, getScripts returns paths that include the directory passed to it. Adapting stubGetScripts to do the same would be good.

Please resubmit for review, after addressing the above points (particularly 2)

comment:19 Changed 15 months ago by tom.prince

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