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.

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.

Ah, good idea! "self.addCleanup(os.chdir, oldCWD)". Pretty slick.

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.

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.

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.

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

Vertical whitespace - separate methods by two blank lines and classes by three.

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

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.

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.

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

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

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.

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.

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.

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.