Opened 9 years ago

Closed 7 years ago

Last modified 7 years ago

#1264 defect closed fixed (fixed)

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

Reported by: exarkun Owned by:
Priority: highest Milestone:
Component: core Keywords: tests
Cc: exarkun, spiv, cablehead, therve Branch: branches/vfs-ftp-1264+2735-2
(diff, github, buildbot, log)
Author: Launchpad Bug:

Description


Change History (59)

comment:1 Changed 9 years ago by 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.

comment:2 Changed 7 years ago by therve

  • Owner changed from cablehead to therve

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

comment:3 Changed 7 years ago by cablehead

Hey therve,

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

comment:4 Changed 7 years ago by therve

(In [20994]) Docstrings cleanups.

Refs #1264.

comment:5 Changed 7 years ago by therve

  • Cc therve added
  • Keywords review added
  • Owner changed from therve to cablehead

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

comment:6 Changed 7 years ago by therve

  • Priority changed from high to highest

comment:7 follow-up: Changed 7 years ago by cablehead

  • Keywords review removed
  • Owner changed from cablehead to therve

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

comment:8 Changed 7 years ago by 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.

comment:9 in reply to: ↑ 7 Changed 7 years ago by 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.

comment:10 Changed 7 years ago by 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

comment:11 Changed 7 years ago by 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

comment:12 Changed 7 years ago by therve

  • Keywords review added
  • Owner changed from therve to cablehead

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.

comment:13 Changed 7 years ago by exarkun

  • Keywords review removed
  • Owner changed from cablehead to therve

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.

comment:14 Changed 7 years ago by therve

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

Refs #1264

comment:15 Changed 7 years ago by therve

  • Keywords review added
  • Owner changed from therve to cablehead

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

comment:16 Changed 7 years ago by cablehead

  • Keywords review removed
  • Owner changed from cablehead to therve

Hey Therve,

Apologies for delay in review :(

This looks nice!

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

comment:17 Changed 7 years ago by therve

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

Refs #1264

comment:18 Changed 7 years ago by therve

  • Keywords review added
  • Owner changed from therve to cablehead

No worry for the delay :).

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

comment:19 Changed 7 years ago by cablehead

  • Keywords review removed

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

comment:20 Changed 7 years ago by cablehead

  • Owner changed from cablehead to therve

comment:21 Changed 7 years ago by therve

(In [21219]) Workaround for win32.

Refs #1264

comment:22 Changed 7 years ago by therve

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

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

comment:23 Changed 7 years ago by therve

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

Refs #1264

comment:24 Changed 7 years ago by therve

  • Resolution fixed deleted
  • Status changed from closed to reopened

I messed up in my commit.

comment:25 Changed 7 years ago by therve

(In [21224]) Some cleanups.

Refs #1264

comment:26 Changed 7 years ago by therve

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

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

comment:27 Changed 7 years ago by cablehead

  • Owner set to cablehead

comment:28 Changed 7 years ago by 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.
+        """

comment:29 Changed 7 years ago by cablehead

  • Keywords review removed
  • Owner changed from cablehead to therve

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

comment:30 Changed 7 years ago by cablehead

  • Keywords review added
  • Owner changed from therve to radix

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.

comment:31 Changed 7 years ago by cablehead

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

comment:32 Changed 7 years ago by 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.

comment:33 Changed 7 years ago by therve

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

comment:34 Changed 7 years ago by therve

(In [21229]) Adjust docstrings.

Refs #1264

comment:35 Changed 7 years ago by cablehead

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

comment:36 Changed 7 years ago by 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.

comment:37 Changed 7 years ago by cablehead

  • Owner changed from radix to spiv

comment:38 follow-up: Changed 7 years ago by exarkun

  • Keywords review removed
  • Owner changed from spiv to therve
  • 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.

comment:39 in reply to: ↑ 38 Changed 7 years ago by therve

  • Keywords review added
  • Owner therve deleted

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.

comment:40 Changed 7 years ago by 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.

comment:41 Changed 7 years ago by jml

  • Keywords review removed
  • Owner set to therve

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.

comment:42 Changed 7 years ago by therve

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

comment:43 Changed 7 years ago by therve

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

Refs #1264

comment:44 Changed 7 years ago by therve

(In [21601]) Cleanups.

Refs #1264

comment:45 Changed 7 years ago by therve

  • Keywords review added
  • Owner therve deleted

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

comment:46 follow-up: Changed 7 years ago by exarkun

  • Keywords review removed
  • Owner set to 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.

comment:47 Changed 7 years ago by exarkun

  • Owner changed from exarkun to therve

comment:48 Changed 7 years ago by therve

(In [21607]) Process review comments

Refs #1264

comment:49 in reply to: ↑ 46 ; follow-up: Changed 7 years ago by therve

  • Keywords review added
  • Owner therve deleted

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.

comment:50 Changed 7 years ago by therve

  • Keywords review removed
  • Owner set to therve

comment:51 Changed 7 years ago by therve

  • Keywords review added
  • Owner therve deleted

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

comment:52 in reply to: ↑ 49 Changed 7 years ago by exarkun

  • Keywords review removed
  • Owner set to therve

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.

comment:53 Changed 7 years ago by therve

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

Refs #1264

comment:54 Changed 7 years ago by therve

  • Branch set to branches/vfs-ftp-1264+2735-2
  • Keywords review added
  • Owner therve deleted

Is the previous modification OK?

comment:55 Changed 7 years ago by jml

  • Keywords review removed
  • Owner set to therve

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.

comment:56 Changed 7 years ago by therve

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

Refs #1264

comment:57 Changed 7 years ago by therve

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

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

comment:58 Changed 7 years ago by cablehead

woo!

comment:59 Changed 4 years ago by <automation>

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