Add unit tests for twisted.python.dist.getAllScripts

[exarkun]

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.

[meissenPlate]

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.

[meissenPlate]

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

[therve]

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!

[meissenPlate]

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.

[meissenPlate]

Revised patch adding test + topfile entry.

[exarkun]

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!

[meissenPlate]

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.

[glyph]

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

[glyph]

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

[thijs]

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

[introom]

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

[thijs]

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

[thijs]

Thanks for your patch. Forced a new  build.