Opened 7 years ago

Closed 7 years ago

#2855 enhancement closed fixed (fixed)

Add tests for ftp.IFTPShell

Reported by: therve Owned by:
Priority: highest Milestone:
Component: ftp Keywords:
Cc: Branch:
Author: Launchpad Bug:

Description

Tests are good. This is extracted from #1264.

Change History (19)

comment:1 Changed 7 years ago by therve

  • Keywords review added
  • Owner therve deleted
  • Priority changed from high to highest

This is ready to review in ftpshell-tests-2855.

comment:2 Changed 7 years ago by exarkun

  • Keywords review removed
  • Owner set to therve

A better test API for createFile would probably have it taking the file contents as an argument instead of looking it up as an attribute on self.

createDirectory documentation should probably talk about whether path is relative or absolute, whether it can contain multiple segments, etc. Maybe same for createFile.

There could be some more tests in the direction of test_openForReading and test_openForWriting - tests which use the returned object and then make sure the correct bytes are read/written. The test coverage provided by IReadWriteTestsMixin is good, but there's no association between IReadWriteTestsMixin.createFileReader/Writer and the objects returned by self.shell.openForReading/Writing in the IFTPShellTestsMixin tests. One possibility is to do an isinstance test on what openForReading/Writing ultimately results in, to make sure it is the same class as IReadWriteTestsMixin ends up testing. This might require making IReadWriteTestsMixin and IFTPShellTestsMixin know more about each other.

FTPReadWriteTestCase.tearDown doesn't need to exist, I think. Leaving it behind helps with debugging, and it doesn't use an appreciable amount of resources.

comment:3 Changed 7 years ago by therve

(In [21536]) Change createFile, some docs.

Refs #2855

comment:4 Changed 7 years ago by therve

(In [21539]) Use the shell instead of intantiating the objects directly.

Refs #2855

comment:5 Changed 7 years ago by therve

  • Keywords review added
  • Owner therve deleted

Thanks for the review. For the main point about the objects, I used the shell in FTPReadWriteTestCase, so we're sure that we used the same objects.

comment:6 Changed 7 years ago by exarkun

  • Keywords review removed
  • Owner set to therve

The EISDIR case of errnoToFailure is untested. (so are a few others, but they're not new in this branch)

All of the exception cases for FTPAnonymousShell.stat are untested. Same for FTPAnonymousShell.list, although that code was mostly there already.

I wonder if the unused exceptions should just be deleted. Nothing ever used or raised them... but some code still might have been importing them or using them for some (pointless) reason. I guess it's fine.

comment:7 Changed 7 years ago by therve

(In [21557]) Add some more tests.

Refs #2855

comment:8 Changed 7 years ago by therve

  • Keywords review added
  • Owner therve deleted

I've added a bunch of tests, back to review.

comment:9 Changed 7 years ago by exarkun

  • Keywords review removed
  • Owner set to therve

TestConsumer doesn't work with streaming producers, might be worth mentioning in the docstring. Also it doesn't document its attributes, and doesn't use streamingProducer at all.

The rest looks good.

comment:10 Changed 7 years ago by therve

(In [21591]) Some tweaks

Refs #2855

comment:11 Changed 7 years ago by therve

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

(In [21592]) Merge ftpshell-tests-2855

Author: therve
Reviewers: exarkun, jml, cablehead
Fixes #2855

Add tests to the twisted.protocols.ftp.IFTPShell interface, to test the
implementation in ftp itself, and to allow to test against other implementations
in the future (eg, vfs).

comment:12 Changed 7 years ago by therve

(In [21623]) Revert r21592: regression under win32-py24

Refs #2855

comment:13 Changed 7 years ago by therve

  • Resolution fixed deleted
  • Status changed from closed to reopened

The error:

[FAIL]: twisted.test.test_ftp.FTPShellTestCase.test_accessNotFound

Traceback (most recent call last):
  File "c:\twistedbuildbot24\W32-full2.4-scmikes-select\Twisted\twisted\trial\unittest.py", line 302, in _eb
    raise self.failureException(output)
twisted.trial.unittest.FailTest: 
Expected: (<class twisted.protocols.ftp.FileNotFoundError at 0x01AE4480>,)
Got:
[Failure instance: Traceback: exceptions.WindowsError: [Errno 3] The system cannot find the path specified: 'c:\\twistedbuildbot24\\W32-full2.4-scmikes-select\\Twisted\\_trial_temp\\twisted.test.test_ftp\\FTPShellTestCase\\test_accessNotFound\\_wfzl2\\temp\\foo/*.*'
c:\twistedbuildbot24\W32-full2.4-scmikes-select\Twisted\twisted\trial\unittest.py:608:_run
c:\twistedbuildbot24\W32-full2.4-scmikes-select\Twisted\twisted\internet\defer.py:107:maybeDeferred
c:\twistedbuildbot24\W32-full2.4-scmikes-select\Twisted\twisted\internet\utils.py:144:runWithWarningsSuppressed
c:\twistedbuildbot24\W32-full2.4-scmikes-select\Twisted\twisted\test\test_ftp.py:1903:test_accessNotFound
--- <exception caught here> ---
c:\twistedbuildbot24\W32-full2.4-scmikes-select\Twisted\twisted\protocols\ftp.py:1566:access
c:\twistedbuildbot24\W32-full2.4-scmikes-select\Twisted\twisted\python\filepath.py:477:listdir
]

comment:14 Changed 7 years ago by therve

  • Keywords review added
  • Owner therve deleted
  • Status changed from reopened to new

The fix is in r21625. Again, a problem with error reporting.

comment:15 Changed 7 years ago by exarkun

  • Keywords review removed
  • Owner set to therve

Windows sucks. and Python on Windows sucks. Maybe someday we can get an API with uniform error reporting. Until then, +1, please merge.

comment:16 Changed 7 years ago by therve

(In [21626]) Something smarter

Refs #2855

comment:17 Changed 7 years ago by therve

(In [21644]) That didn't work.

Refs #2855

comment:18 Changed 7 years ago by therve

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

(In [21645]) Merge ftpshell-tests-2855-2

Author: therve
Reviewers: exarkun, jml, cablehead
Fixes #2855

Add tests to the twisted.protocols.ftp.IFTPShell interface, to test the
implementation in ftp itself, and to allow to test against other
implementations in the future (eg, vfs).

comment:19 Changed 3 years ago by <automation>

  • Owner therve deleted
Note: See TracTickets for help on using tickets.