Ticket #6137 enhancement new

Opened 18 months ago

Last modified 10 months ago

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

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

Change History

1

Changed 18 months ago by meissenPlate

  • owner set to meissenPlate
  • status changed from new to assigned

2

Changed 18 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 18 months ago by meissenPlate

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

3

Changed 18 months ago by meissenPlate

  • keywords review added

4

Changed 17 months ago by therve

  • keywords review removed
  • owner set to meissenPlate

1. You can use addCleanup instead of implementing tearDown.

2. There are too many "should" in the test docstring: try to describe behavior.

3. Instead of using sorted() for the assert, I'd rather use set(), which makes it more clear that order doesn't matter.

4. The isdir check is not tested.

5. Please use camelCase for variables.

Thanks!

5

Changed 16 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.

2. 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.

3. Can't use set(), set() wouldn't detect duplications. I used collections.Counter() instead. Looks better than sorted(), thanks :).

4. 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.

5. Fixed. Sorry about that.

Changed 16 months ago by meissenPlate

Revised patch adding test + topfile entry.

6

Changed 16 months ago by meissenPlate

  • owner meissenPlate deleted

7

Changed 16 months ago by exarkun

  • owner set to exarkun
  • status changed from new to assigned

8

Changed 16 months ago by exarkun

  • status changed from assigned to new
  • keywords review removed
  • owner changed from exarkun to meissenPlate
  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!

9

Changed 16 months ago by meissenPlate

  • keywords review added
  • owner meissenPlate deleted

1. Fixed.

2. Based on that article, maybe even "spy", but no one else calls it that. It's now stubGetScripts.

3. 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.

4. 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.

5. Fixed. I didn't find a replacement for os.chdir though.

6. That makes sense. I poked at the comments for a while, and I think they are better now.

7. 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 16 months ago by meissenPlate

10

Changed 16 months ago by glyph

  • branch set to branches/test-scripts-6137
  • branch_author set to glyph

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

11

Changed 16 months ago by glyph

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

12

Changed 15 months ago by thijs

  • owner set to meissenPlate
  • keywords review removed
  • cc thijs added

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

13

Changed 12 months ago by introom

  • status changed from new to assigned
  • owner changed from meissenPlate to introom

Changed 12 months ago by introom

14

Changed 12 months ago by introom

  • status changed from assigned to new
  • owner introom deleted
  • keywords review added

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

15

Changed 12 months ago by thijs

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

16

Changed 12 months ago by thijs

  • branch_author changed from glyph to meissenPlate, introom

Thanks for your patch. Forced a new  build.

17

Changed 10 months ago by tomprince

  • branch changed from branches/test-scripts-6137 to branches/test-scripts-6137-2
  • branch_author changed from meissenPlate, introom to meissenPlate, introom, tomprince

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

18

Changed 10 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)

19

Changed 10 months ago by tom.prince

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