Opened 5 years ago

Closed 5 years ago

#4020 defect closed fixed (fixed)

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 (15)

comment:1 Changed 5 years ago by exarkun

#4135 and #4193 were duplicates of this.

comment:2 Changed 5 years ago by jesstess

  • Cc jesstess added
  • Owner changed from jml to jesstess

comment:3 Changed 5 years ago by jesstess

  • Author set to jesstess
  • Branch set to branches/remove-safely-4020

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

comment:4 Changed 5 years ago by jesstess

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

refs #4020

comment:5 Changed 5 years ago by jesstess

  • Keywords review added
  • Owner jesstess deleted

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.

comment:6 Changed 5 years ago by thijs

  • Cc thijs added
  • Keywords review removed
  • Owner set to jesstess

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

comment:7 Changed 5 years ago by jesstess

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

refs #4020

comment:8 Changed 5 years ago by jesstess

  • Keywords review added
  • Owner jesstess deleted

comment:9 Changed 5 years ago by TimAllen

  • Owner set to TimAllen

comment:10 Changed 5 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.

comment:11 Changed 5 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.

comment:12 Changed 5 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

comment:13 Changed 5 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'

comment:14 Changed 5 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.

comment:15 Changed 5 years ago by jesstess

  • Resolution set to fixed
  • Status changed from new to closed

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