twisted.vfs.test.test_ftp is enough to drive a sane man mad

[exarkun]

FTPAdapterTestCase does some things that aren't wonderful.

Interacting with protocol-level code when testing an adapter from one
well-defined interface to another well-defined interface should be avoided. 
Having to instantiating the FTP() class should raise red flags.

Setting a bunch of attributes on the FTP instance is also borderline.  While it
works now, there's no guarantee these things will remain stable.  While these
attributes don't begin with an underscore, effort should be taken to avoid
relying on just setting them to work.  The undocumented dtpInstance is
particularly eggregious, since at least the others are mentioned in the class
doc string or strongly implied (in the case of "factory").

Calling ftp_ methods is equally unfortunate.  Which this reflects the current
implementation strategy of FTP, it is by no means guaranteed to remain stable. 
The interface guaranteed by a protocol is "dataReceived".  If bytes are going to
be rammed into one, that is the API to use.  This has the advantage of being
extremely stable, both in name, argument signature, and return signature.

Other problems which radix is in the process of fixing are the use of
"Deferred.result" - this is another nominally private attribute.  The only real
way to get the result of a Deferred is with a callback.  Also, relying on
Deferreds to have a result "synchronously" (ie, making a call, getting back a
Deferred, immediately using its result) is a mistake.  Implementations may
change so that what was once synchronous becomes asynchronous.  Tests should not
need to be rewritten in the face of this.  Testing for failures should be done
with TestCase.assertFailure().

Finally, a much saner overall strategy for these tests would be to make calls
directly onto the adapter.  It implements IFTPShell.  It *must* implemented
IFTPShell.  That is its purpose for existing.  Assert things about behavior of
methods on that interface, and all the protocol-specific code falls away.  The
tests become simpler and more obvious.

[therve]

I think I'll steal that. It seems that this should happen before #1223, did I understand correctly?

[cablehead]

Hey therve,

Thanks, you're awesome. And yep, that's right about #1223.

[therve]

(In [20994]) Docstrings cleanups.

Refs #1264.

[therve]

This is ready to review in vfs-ftp-1264+2735. It handles #2735 too. Andy, can you have a look? Thanks.

[cablehead]

Hey therve - bloody awesome! :) I've brained dump thoughts while looking through this area. Some of the thoughts are just general vfs - ftp comments, and not directly related to the 2 tickets in question.

Just checking - makeVFSAsync-1223-2 is the HEAD for vfs. vfs-ftp-1264+2735 is branched off trunk. Since its just fixing up the current tests though, it should be fine, it'll just need to be merged against makeVFSAsync-1223-2. makeVFSAsync-1223-2 is now pretty different to trunk, making merging difficult.

I've planned that if makeVFSAsync-1223-2 ever got into good enough shape it could be brought back to trunk, I will look through changes made to trunk since that branches last merge forward, and manually ensure they are covered.

The comment for FileSystemToIFTPShellAdaptor's init says it takes a IFileSystemContainer, but its still being passed a vfs.pathutils.Filesystem

    @type filesystem: L{ivfs.IFileSystemContainer}.

vfs.pathutils is scheduled for death. Ideally adapter's should just take an IFileSystemNode. Actually this should probably be a seperate ticket. (#2780)

I don't quite get why there's support for web2.streams, and producers/consumers. I'll check this with spiv.

The errors thrown by the adapter are inconsistent. It doesn't look like IFTPShell expects errors of any certain type, so I guess that's ok. ivfs in makeVFSAsync-1223-2 defines errors to be thrown by backends a little better, so will hopefully clean this up a bit.

    ...
    return self.assertFailure(d, ivfs.NotFoundError) 
    ...
    return self.assertFailure(d, IOError)

FTPAdapterTestCase could possibly be removed now that there are direct tests for the adapter.

A couple of integration tests with the protocol would be nice. This'll pick up if IFTPShell changes - or if ftp makes undocumented assumptions about IFTPShell. As exarkun points out though, it would be a lot more robust to drive these through an ftp server's dataReceived.

Alternatively - and this is what we've talked about for vfs backends a bit as well, there could be a base suite of IFTPShell tests, which all implementations subclass and get put through. This gets most the benefits as integration tests of warning about interface changes, or undocumented interface assumptions without the gore.

Couple of typos:

def registerConsumer(self, consumer): 
    """ 
    Keep _trakc_ of the consumer and return a Deferred fired when finished. 
    """ 

class _FileToConsumerAdapter(object): 
    """ 
    An _adaptater_ for writing to a VFS file. 
    """ 

[cablehead]

Yeah, I'm pretty sure we can drop the web2.stream stuff out of the ftp adapter (#2781), which makes tests a bit simpler.

[therve]

Replying to cablehead:

A couple of integration tests with the protocol would be nice. This'll pick up if IFTPShell changes - or if ftp makes undocumented assumptions about IFTPShell. As exarkun points out though, it would be a lot more robust to drive these through an ftp server's dataReceived.

I started to look at this. This can be good, but it would needed some work on the ftp implementation to be cleanly feasible (especially DTP management). If you're OK, I'll open a ticket for that, but that'll delay this one even more.

[cablehead]

Ok, after a quick irc chat - a common base suite of tests for IFTPShell implementations would be great. This will highlight omissions in the interface spec, like which errors to return etc.

I think the list of things to do to close #1264 and #2735 are:

• add tests for IFTPShellTestCase in twisted.test.test_ftp
• import and reuse this in twisted.vfs.test.test_ftp
• this'll probably mean some cleaning up of IFTPShell's interface
• fix up the 3 comments noted above
• delete FTPAdapterTestCase

[therve]

(In [21136])

• Create IFTPShellTestCase, and use for both ftp and vfs tests
• A bunch of cleanups in both vfs and ftp code

Refs #1264

[therve]

Alright, something can be checked out. I wasn't very protective on current vfs interface, mainly on errors raised: the shell needed to get consistent errors.

The branch is a bit too big to my taste. If you confirm, I'll create another ticket to merge the ftp changes first.

[exarkun]

twisted.vfs.test.test_sftp.SFTPAdapterTest.test_openFileTrunc failing with the branch merged into trunk at the moment, not sure if that's expected or not.

[therve]

(In [21192]) Manage O_TRUNC in inmem backend.

Refs #1264

[therve]

It wasn't expected, it's an effect due to a correction I made in inmem backend. That should be better now.

[cablehead]

Hey Therve,

Apologies for delay in review :(

This looks nice!

• No worries about the trunk vfs interface. It's overhauled in: http://twistedmatrix.com/trac/browser/branches/makeVFSAsync-1223-2/twisted/vfs/ivfs.py

• Should we do something similar with ftp.IReadFile and ftp.IWriteFile ?

• Merging forward makeVFSAsync-1223-2 against this is starting to look hairy. It might be easier for me to reapply the changes from that branch manually to a new branch, once this branch is merged with trunk.

[therve]

(In [21217]) Add tests for the IReadFile and IWriteFile interfaces.

Refs #1264

[therve]

No worry for the delay :).

I added tests for ftp.IReadFile and ftp.IWriteFile, back to review.

[cablehead]

Amazing :) I'm for merging and closing these 2 tickets out.

[therve]

(In [21219]) Workaround for win32.

Refs #1264

[therve]

(In [21220]) Merge vfs-ftp-1264+2735

Author: therve Reviewers: cablehead, exarkun Fixes #1264 Fixes #2735

Add tests for FTP interfaces IFTPShell, IReadFile and IWriteFile. This led to some cleanups in ftp.FTPShell, and removed several bugs in vfs ftp adapter and inmem backend.

[therve]

(In [21222]) Revert [21220]: wrong commit

Refs #1264

[therve]

I messed up in my commit.

[therve]

(In [21224]) Some cleanups.

Refs #1264

[therve]

I put it back for review, because there were a few typos in docstrings remaining.

[cablehead]

Couple more typo's:

vfs/adapters/test/test_ftp.py:

+        """
+        Create a filesystem for tests and intantiate a FTP shell.
+        """

vfs/adapters/ftp.py:

+    """
+    An adaptater for writing to a VFS file.
+    """

twisted/test/test_ftp.py:

     """
     Test functionnalities for FTP shells classes.
     """

+        """
+        openForWring should fail with L{ftp.FileNotFoundError} when trying to
+        create a file in a non-existing directory.
+        """

[cablehead]

I think that's it, let's merge this sucker!

[cablehead]

glyph rightly requested that it would be good for this branch to get a review from a more seasoned twisted contributor.

radix volunteered for something in the vicinity of that discussion - so assigning your way radix :) - hope that's cool.

[cablehead]

btw, therve, was the reason for the trunk revert just svn mis-usage?

[radix]

Ok, I'll take a look at it. I've skimmed it briefly and am slightly concerned about the API changes in the ftp code. Will discuss after dinner.

[therve]

The revert happened because I merged the wrong revision of the branch, before the fixes of [21219] for win32.

[therve]

(In [21229]) Adjust docstrings.

Refs #1264

[cablehead]

there's still the typo: openForWring in test_ftp.py

[cablehead]

The only changes in API for ftp are standardizing some of the the errors to be raised.

Tests were added for ftp.FTPShell. As this hadn't had tests before, it highlighted omissions in IFTPShell.

[exarkun]

* twisted/test/test_ftp.py

• IFTPShellTestCase should be named something like IFTPShellTestsMixin, since it is intended to be mixed in and isn't a TestCase.
• IFTPShellTestCase.directoryExists doesn't document how the check should report its results, nor the type of its parameter (which information is also missing from the other methods on this class).
• The first two lines of test_removeDirectory and test_removeDirectoryOnFile seem to be testing the fixture, rather than the shell implementation. Maybe tests such as these should be separated into test methods dedicated solely to ensuring the validity of the fixture?
• TestConsumer.write's docstring safes Safe but should probably say Save.
• Similar naming issue with IWriteFileTestCase.
• Docstrings for IWriteFileTestCase.test_{read,write} could be improved

* twisted/protocols/ftp.py

• Is there test coverage for the changes to FTPAnonymousShell.list? In particular, the list comprehension which does fPath.child to the listdir results.
• two of the win32 special-cases have nice comments, but one of them just says Workaround win32 crappiness and the one in FTPShell.removeFile has no comment at all.

I only skimmed the twisted/vfs/ changes. They seemed okay, but someone (maybe me, later) should take a closer to look at them.

[therve]

Thanks for the review!

Replying to exarkun:

IFTPShellTestCase should be named something like IFTPShellTestsMixin, since it is intended to be mixed in and isn't a TestCase.

Done.

IFTPShellTestCase.directoryExists doesn't document how the check should report its results, nor the type of its parameter (which information is also missing from the other methods on this class).

Done.

The first two lines of test_removeDirectory and test_removeDirectoryOnFile seem to be testing the fixture, rather than the shell implementation. Maybe tests such as these should be separated into test methods dedicated solely to ensuring the validity of the fixture?

Right, I've added 2 tests for that.

TestConsumer.write's docstring safes Safe but should probably say Save.

Done.

Similar naming issue with IWriteFileTestCase.

Done.

Docstrings for IWriteFileTestCase.test_{read,write} could be improved

I've tried something :).

Is there test coverage for the changes to FTPAnonymousShell.list? In particular, the list comprehension which does fPath.child to the listdir results.

I think so, see the test_list test.

two of the win32 special-cases have nice comments, but one of them just says Workaround win32 crappiness and the one in FTPShell.removeFile has no comment at all.

Right, I've improved/added comments.

[cablehead]

#1223 has an overview for what's left for VFS to not suck so much.

Once this (#1264) ticket is merged, I'll get #2815 ready for review as quickly as I can.

[jml]

pyflakes gives the following warnings:

$ pyflakes twisted/protocols/ftp.py
twisted/protocols/ftp.py:27: redefinition of unused 'grp' from line 25
twisted/protocols/ftp.py:27: redefinition of unused 'pwd' from line 25
$ pyflakes twisted/vfs/adapters/ftp.py
twisted/vfs/adapters/ftp.py:22: 'stream' imported but unused
$ pyflakes twisted/test/test_ftp.py
twisted/test/test_ftp.py:418: redefinition of fuction 'checkDownload' from line 412
twisted/test/test_ftp.py:427: redefinition of fuction 'checkDownload' from line 418
twisted/test/test_ftp.py:439: redefinition of fuction 'checkDownload' from line 427

The diff for this branch is 1817 lines long. This is probably too big (as I think you mention in the comments on the ticket.)

twisted/protocols/ftp.py:

• The exceptions all need docstrings. Given the size of this branch, I would be happy if you filed a ticket that indicated that they need docstrings.
• I find the names fName, fEntries, and fPath (in list()) to be confusing. It's not clear what the f prefix implies. Please rename these variables.
• list() also needs a docstring.
• FTPShell needs a docstring. Please add one.
• In the comment in openForWriting, change EACESS to EACCESS.

twisted/vfs/adapters/ftp.py:

• The XXX comment in access() is confusing. If it is actually XXX-worthy, then it must make clear what needs to be changed.

twisted/test/test_ftp.py

• I don't particularly like the style of test mixin that you use here. However, the code is both clear and clean, and my alternative would be less so at this point, so I won't recommend changes.
• Add a blank line between TestConsumer docstring and implements.

[therve]

I opened #2855 to first check the ftp changes in.

[therve]

(In [21600]) Merge forward, resolve conflicts, adapt tests to changes in trunk.

Refs #1264

[therve]

(In [21601]) Cleanups.

Refs #1264

[therve]

Back to review in vfs-ftp-1264+2735-2. The diff is only 800 lines now :).

[exarkun]

• twisted/vfs/test/test_ftp.py
  • DirectFTPAdapterTestCase class name and class docstring could be improved: Direct is only informative if you knew how the code used to work (ie, by going through the protocol layer). All tests should directly test the unit they are concerned with, so the name/docstring should reflect something more interesting, or at least leave out this redundant information. It might be good to say that the test case is verifying that FileSystemToIFTPShellAdaptor implements IFTPShell in a particular way (maybe mentioning if it is read-only, read-write, any other interesting properties, etc).
• twisted/vfs/test/test_inmem.py
  • There's no module docstring.
  • FakeDirectory.createFile has logic for raising an exception if an existing file is opened exclusively, I don't see any tests for this though. Did I miss them?
  • Similarly, I don't see tests for the IOError which that method sometimes raises.
• twisted/vfs/adapters/ftp.py
  • the list method slices the result of filesystem.children - dropping the first two elements. I assume these are . and ... Does the interface actually guarantee this, though?
  • there should be a ticket for doing a real implementation of the access method (and maybe also or another one for extending the general IFTPShell tests to cover this, since they don't appear to now). The XXX comment in that method should point to the ticket.
  • _FileToConsumerAdapter.registerAdapter shouldn't assert streaming. I guess it should raise NotImplementedError and there should be a ticket for implementing support for non-streaming producers here.

[therve]

(In [21607]) Process review comments

Refs #1264

[therve]

Ok that should be better.

Replying to exarkun:

* the list method slices the result of filesystem.children - dropping the first two elements. I assume these are . and ... Does the interface actually guarantee this, though?

What interface are you talking about? The ftp interface doesn't expect this (because os.listdir doesn't give that). And the vfs interface explicitly add these two elements.

[therve]

Put it back to review now that #2855 has been remerged.

[exarkun]

Replying to therve:

Ok that should be better. Replying to exarkun:

* the list method slices the result of filesystem.children - dropping the first two elements. I assume these are . and ... Does the interface actually guarantee this, though?

What interface are you talking about? The ftp interface doesn't expect this (because os.listdir doesn't give that). And the vfs interface explicitly add these two elements.

I was wondering if ivfs.IFileSystemContainer.children guaranteed that the first two elements of the returned sequence would be . and ... The docstring for that method doesn't say anything about those elements.

The rest of the fixes look good.

[therve]

(In [21727]) Add documentation for children, cleanups.

Refs #1264

[therve]

Is the previous modification OK?

[jml]

The diff in [21727] looks good. I've a non-thorough review of the overall diff as well.

Put a blank line (and a docstring if you are feeling generous) between the class declaration of FileConsumer and the following implements() line in twisted/protocols/ftp.py and then merge away.

[therve]

(In [21792]) Generous is my second name.

Refs #1264

[therve]

(In [21793]) Merge vfs-ftp-1264+2735-2

Author: therve Reviewers: jml, exarkun, cablehead Fixes #1264 Fixes #2735

Refactor tests for the vfs ftp adapter, using the new IFTPShellTestsMixin, and fixe behavior both in the ftp adapter and in the inmem backend.

[cablehead]

woo!