Ticket #2855 enhancement closed fixed

Opened 7 years ago

Last modified 6 years ago

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

1

Changed 7 years ago by therve

  • owner therve deleted
  • priority changed from high to highest
  • keywords review added

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

2

Changed 6 years ago by exarkun

  • owner set to therve
  • keywords review removed

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.

3

Changed 6 years ago by therve

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

Refs #2855

4

Changed 6 years ago by therve

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

Refs #2855

5

Changed 6 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.

6

Changed 6 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.

7

Changed 6 years ago by therve

(In [21557]) Add some more tests.

Refs #2855

8

Changed 6 years ago by therve

  • owner therve deleted
  • keywords review added

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

9

Changed 6 years ago by exarkun

  • owner set to therve
  • keywords review removed

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.

10

Changed 6 years ago by therve

(In [21591]) Some tweaks

Refs #2855

11

Changed 6 years ago by therve

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

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

12

Changed 6 years ago by therve

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

Refs #2855

13

Changed 6 years ago by therve

  • status changed from closed to reopened
  • resolution fixed deleted

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
]

14

Changed 6 years ago by therve

  • keywords review added
  • status changed from reopened to new
  • owner therve deleted

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

15

Changed 6 years ago by exarkun

  • owner set to therve
  • keywords review removed

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

16

Changed 6 years ago by therve

(In [21626]) Something smarter

Refs #2855

17

Changed 6 years ago by therve

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

Refs #2855

18

Changed 6 years ago by therve

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

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

19

Changed 3 years ago by <automation>

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