twisted.trial.runner.TrialRunner._removeSafely has typo and untested branch

[ivank]

It incorrectly uses FilePath, which doesn't exist. (filepath.FilePath does)

http://twistedmatrix.com/trac/browser/trunk/twisted/trial/runner.py#L728

This was found with Pyflakes.

[exarkun]

#4135 and #4193 were duplicates of this.

[jesstess]

(In [27904]) Branching to 'remove-safely-4020'

[jesstess]

(In [27905]) Fix typo and add unit tests for the 3 possible branches in _removeSafely.

refs #4020

[jesstess]

Changes to the branch include:

• fixing the typo
• epytext in _removeSafely
• addition of unit tests for each of the 3 branches in _removeSafely
• updating the copyrights on runner.py and test_runner.py
• addition of .misc file

Buildbot results  here.

[thijs]

I'd say it's a bug so a bugfix news file might be more appropriate..

[jesstess]

(In [27909]) .misc ==> .bugfix

refs #4020

[TimAllen]

1. If I were reading a hypothetical Twisted 10.0 announcement, I'd find it difficult to figure out what the message in your .bugfix file meant to me as an ordinary developer. Something like "Twisted Trial no longer crashes if it can't remove an old _trial_temp directory" would be more explanatory, I think. People can look up the ticket number if they want the embarrassing details.
2. In twisted/trial/test/test_runner.py, you might as well define dummyRemove() and dummyMoveTo() inside the tests where they're used; they're pretty simple.
3. Again in the tests, rather than "filepath.FilePath(os.path.join(directory, '_trial_marker')).touch()" you could just say "dirPath.child('_trial_marker').touch()"
4. Why do you call stdout.restore() before the final assertEquals? self.patch() monkey-patches are automatically reverted at the end of the test anyway.
5. Rather than slicing the output string for comparison purposes, just say failUnlessIn("could not remove FilePath", out.getvalue()).
6. In test_removeSafelyRemoveFailsMoveFails, you should check that the OSError that gets re-raised is the one you expect - make dummyRemove() and dummyMoveTo() pass some recognizable parameter to OSError() and check the properties of the OSError that failUnlessRaises returns. Likewise, you should use failUnlessIn to check that both error messages are present in the captured stdout.
7. Why have you added these tests to twisted.trial.test.test_runner.TestUntilFailure? You could have added them to twisted.trial.test.test_runner.TestRunner and replaced the higher-level test_noMarker test in that TestCase.

[exarkun]

Coding standard note, For consistency, Twisted unit tests should use the assert forms rather than the fail forms and the singular names rather than the plural names.

[jesstess]

(In [27939]) Move trial marker tests into test_runner.TestRunner and address the rest of TimAllen's review points.

refs #4020

[jesstess]

• re 1-6: done.
• re 7: done. No reason other than 'I didn't see test_noMarker when I was glancing through the file to decide where to put these tests'

[exarkun]

1. Avoid the fail* assertions. assertRaises should be used instead of failUnlessRaises. assertIn should be used instead of failUnlessIn.
2. Use the ends-with-s spellings of the assert* methods. assertEquals instead of assertEqual, etc.
3. What are the new pairs of single quotes in the _removeSafely docstring around C{_trial_marker}? C{"_trial_marker"} would make sense, I think.

Otherwise looks good. Please merge once the above points are fixed.

[jesstess]

(In [27950]) Merge remove-safely-4020

Author: jesstess Reviewers: TimAllen, exarkun Fixes: #4020

Fix namespace typo so that Twisted Trial no longer crashes if it can't remove an old _trial_temp directory, and add unit tests for the previously untested branches in TrialRunner._removeSafely.