Ticket #4020 defect closed fixed

Opened 5 years ago

Last modified 4 years ago

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

Reported by: ivank Owned by: jesstess
Priority: normal Milestone:
Component: trial Keywords:
Cc: jesstess, thijs Branch: branches/remove-safely-4020
(diff, github, buildbot, log)
Author: jesstess Launchpad Bug:

Description

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.

Change History

1

Changed 4 years ago by exarkun

#4135 and #4193 were duplicates of this.

2

Changed 4 years ago by jesstess

  • owner changed from jml to jesstess
  • cc jesstess added

3

Changed 4 years ago by jesstess

  • branch set to branches/remove-safely-4020
  • branch_author set to jesstess

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

4

Changed 4 years ago by jesstess

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

refs #4020

5

Changed 4 years ago by jesstess

  • owner jesstess deleted
  • keywords review added

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.

6

Changed 4 years ago by thijs

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

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

7

Changed 4 years ago by jesstess

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

refs #4020

8

Changed 4 years ago by jesstess

  • keywords review added
  • owner jesstess deleted

9

Changed 4 years ago by TimAllen

  • owner set to TimAllen

10

Changed 4 years ago by TimAllen

  • keywords review removed
  • owner changed from TimAllen to jesstess
  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.

11

Changed 4 years ago by 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.

12

Changed 4 years ago by jesstess

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

refs #4020

13

Changed 4 years ago by jesstess

  • keywords review added
  • owner jesstess deleted
  • 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'

14

Changed 4 years ago by exarkun

  • keywords review removed
  • owner set to jesstess
  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.

15

Changed 4 years ago by jesstess

  • status changed from new to closed
  • resolution set to fixed

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

Note: See TracTickets for help on using tickets.