Opened 3 years ago

Closed 13 months ago

#7037 enhancement closed fixed (fixed)

Write test for cftp._cbPutTargetAttrs or remove untested / unused code.

Reported by: adiroiban Owned by: adiroiban
Priority: normal Milestone:
Component: conch Keywords:
Cc: z3p Branch: branches/cftp-tests-7037-2
branch-diff, diff-cov, branch-cov, buildbot
Author: glyph

Description

$ pyflakes --version
0.7.3
$ pyflakes twisted/conch/
twisted/conch/scripts/cftp.py:431: undefined name 'files'

There is still an error which I would like to fix before merge, but I need help and I have no idea what is going on there... maybe this needs a separate ticket!

    def _cbPutTargetAttrs(self, attrs, path, local):
        if not stat.S_ISDIR(attrs['permissions']):
            return "Wildcard put with non-directory target."
        return self._cbPutMultipleNext(None, **files**, path)

Attachments (2)

7037-1.diff (20.2 KB) - added by adiroiban 3 years ago.
7037-2.diff (24.4 KB) - added by adiroiban 2 years ago.

Download all attachments as: .zip

Change History (17)

comment:1 Changed 3 years ago by DefaultCC Plugin

  • Cc z3p added

Changed 3 years ago by adiroiban

comment:2 Changed 3 years ago by adiroiban

  • Keywords review added
  • Owner adiroiban deleted

I first wanted to write test into TestOurServerCmdLineClient but since they are functional tests (hard to debug) I went for writing unit tests.


I first started with writing test and documentation for getFilename as I had no idea what it should do.

I have updated _getFilename to always return strings and to always return tupple and not tupple or list ... or string or None.


Then I continue to write test for cmd_PUT command.

When writing a test for a transfer for an empty found I found out there is a bug in _printProgressBar.

So i fixed printProgressBar and wrote a test.


I continue with writing test for all 4 branches for cmd_PUT.

The method checking for cftp output message is obfuscated, but I don't know how to better write a checker for that output.


The cbPutTargetAttrs code which triggered this ticket was checking that requested remote path is a directory or not. The check was done only for wildchards. The other branches in cmd_PUT were not checking for remote directory.

I have removed the code with problems to simplify the cmd_PUT.

I have refactored the code to use common code for all branches.

I don't like the single=True flag argument in _cbPutMultipleNext but I don't know how to change it so that code duplication is minimized.

I have renamed l, lf, rez variables as I found them hard to read.

The actual transfer was extracted into _putRemoteFile to have shorted methods which are easier to read.


I haver updated StdioClientTests so that self.client will use a single transport... not 2 separate transports. Hope this will reduce confusion for future tests.


No pyflakes errors are found in changed files.

Twistedchecker diff functionality is broken and I did not checked the code with it. Hope it will be fixed soon as I have submitted a patch for that.


Conch test pass on my py 2.7 $ ./bin/trial twisted.conch


This ticket grow pretty big. Let me know if you want to send changes for _getFilename and _printProgressBar in separate tickets.

Thanks

comment:3 Changed 2 years ago by glyph

  • Author set to glyph
  • Branch set to branches/cftp-tests-7037

(In [43452]) Branching to cftp-tests-7037.

comment:4 Changed 2 years ago by glyph

  • Keywords review removed
  • Owner set to adiroiban

Hi adi,

Thanks for your contribution. As usual, sorry for the long delay on reviews.

  1. This will need a NEWS file.
  2. twistedchecker was, sadly, not happy with the patch.
  3. There are a bunch of test failures, but only on some of the builders. Not quite sure what's going on with this:
    Traceback (most recent call last):
      File "/var/lib/buildbot/bot-glyph-1/ubuntu64-py2.6-epoll/Twisted/twisted/conch/test/test_cftp.py", line 613, in test_cmd_PUTMultipleWithRemotePath
        (second, secondName, ['100% 0.0B']),
      File "/var/lib/buildbot/bot-glyph-1/ubuntu64-py2.6-epoll/Twisted/twisted/conch/test/test_cftp.py", line 482, in checkPutMessage
        self.assertEqual(expectedOutput, actualOutput)
      File "/var/lib/buildbot/bot-glyph-1/ubuntu64-py2.6-epoll/Twisted/twisted/trial/_synctest.py", line 447, in assertEqual
        % (msg, pformat(first), pformat(second)))
    twisted.trial.unittest.FailTest: not equal:
    a = ['twisted.conch.test.test_cftp/StdioClientTests/test_cmd_PUTMultipleWithRemotePa/FAQbBd/temp100% 0.0B',
     'Transferred twisted.conch.test.test_cftp/StdioClientTests/test_cmd_PUTMultipleWithRemotePa/FAQbBd/temp to temp',
     'twisted.conch.test.test_cftp/StdioClientTests/test_cmd_PUTMultipleWithRemotePa/FAQbBd/second-name100% 0.0B',
     'Transferred twisted.conch.test.test_cftp/StdioClientTests/test_cmd_PUTMultipleWithRemotePa/FAQbBd/second-name to second-name']
    b = ['twisted.conch.test.test_cftp/StdioClientTests/test_cmd_PUTMultipleWithRemotePa/FAQbBd/second-name100% 0.0B',
     'Transferred twisted.conch.test.test_cftp/StdioClientTests/test_cmd_PUTMultipleWithRemotePa/FAQbBd/second-name to second-name',
     'twisted.conch.test.test_cftp/StdioClientTests/test_cmd_PUTMultipleWithRemotePa/FAQbBd/temp100% 0.0B',
     'Transferred twisted.conch.test.test_cftp/StdioClientTests/test_cmd_PUTMultipleWithRemotePa/FAQbBd/temp to temp']
    
  4. InMemoryRemoteFile should probably be a top-level class, not a nested class.
  5. I think this looks mostly sensible as a testing strategy, but I would appreciate it for the re-review if you could be a bit more specific in docstrings. For example; InMemoryRemoteFile says it is "like a remote sftp file", but should that come with an L{} link to the thing that it's a fake for? Similarly for InMemorySFTPClient. And for test_printProgressBarEmptyFile, it says "I" can print the progress for empty files - who is "I"?

Thanks again for all your contributions. This is a good improvement to test coverage and I hope we can get the test failures and coding standard issues addressed.

comment:5 Changed 2 years ago by adiroiban

  • Keywords review added
  • Owner adiroiban deleted

Hi,

  1. I was thinking that this is a valid news file. What should I do here?
diff --git twisted/conch/topfiles/7037.misc twisted/conch/topfiles/7037.misc
new file mode 100644
index 0000000..e69de29
  1. Hope all is ok now. I failed to get a local setup similar to buildbot config and Twistedecheker is still a PITA. When I try to check my files on my local computer, I get tons of unrelated errors
twistedchecker --diff trunk.errors twisted/conch/scripts/cftp.py twisted/conch/test/test_cftp.py

I see that twistedchecker wants me to duplicate the docstring from the interface. Why?

Also, it want me to duplicate the docstring of InMemorySFTPClient.openFile. Why? filetransfer.FileTransferClient.openFile had a huge docstring. What do we gain by copying it in this test?

I can copy those @param and @type docstring, but really don't see what we gain with that.

  1. The problem is that this test uses globbing... and it looks like globbing code is not consistent over all platforms... I will update the test so that the transfer order will not count.
  1. Done.
  1. True. I tried to improve the docstrings.

My plan was to use "I" or "It" instead of the long "L{StdioClient._printProgressBar}". From the test name, I was hoping that it is clear that I is '_printProgressBar' .

As a testing strategy, my plan was to name all test after the targeted method (SUT http://xunitpatterns.com/SUT.html) and arrange the tests in AAA (http://c2.com/cgi/wiki?ArrangeActAssert) .. in this way it should be clear who is 'It" or "I". Is this a stupid think to do?

I am trying to make test writing fun, easy and fast... reducing boilerplates, repetitive text and docstring duplication.

This is why, from this point of view, I prefer the test_printProgressBar_empty() naming convention. It makes it easier to read who is the SUT


I have attached a new diff based on master.

Thanks!

Changed 2 years ago by adiroiban

comment:6 Changed 21 months ago by glyph

  • Branch changed from branches/cftp-tests-7037 to branches/cftp-tests-7037-2

(In [44002]) Branching to cftp-tests-7037-2.

comment:7 Changed 21 months ago by glyph

  • Keywords review removed
  • Owner set to adiroiban

Hi adiroiban,

Sorry for the long wait. It looks like there are still a bunch of failures on the builders, though; many test timeouts, and still a bunch of coding-standard violations on twistedchecker.

Hopefully now that you can put stuff into the CI infrastructure yourself, we can reduce the latency on this ...

I like the fact that this branch adds a bunch more comprehensible unit-y tests.

I would prefer that you don't replace L{...} with It at the beginning of test docstrings though. This is more ambiguous, and sort of detracts from the point of the branch.

Double-spacing after @type to make it line up with @param is a little unusual; not a coding standard violation, but still not quite consistent with the usual style. I've written a little thing to automate the conventional formatting of docstrings as you write them: https://github.com/glyph/python-docstring-mode

It's for Emacs, but it could be adapted to Vim (in fact I originally wrote it for Sublime Text…)

comment:8 Changed 21 months ago by adiroiban

  • Keywords review added
  • Owner adiroiban deleted
  1. I have reverted the It substitutions. Since the test methods not start with test_SUT, I tried not to duplicate that info in the docstring.
  1. I haver revert the double spacing after @type.
  1. I have imported division from future to prepare for future port work and make sure that tests are still valid.
  1. With this change pyflakes twisted/counch is clean :) as this is I start working on this ticket in the first place.
  1. Remaining twistedcheker errors are because it ask to duplicate docstring from an interface.

Please review all changes from the new branch. Branch sent to buildbot ... will have to wait for results :( Thanks!

Last edited 21 months ago by adiroiban (previous) (diff)

comment:9 Changed 19 months ago by glyph

Hi adi; thanks for working on this; I will try to review it soon. In the future, please force builds so reviews will have builds ready when they go to start a review.

comment:10 Changed 19 months ago by glyph

Oh wait. I didn't read the previous comment. I didn't see any results on the buildbot - I wonder why that is.

comment:11 Changed 19 months ago by glyph

  • Keywords review removed
  • Owner set to adiroiban

Thanks again for your work on this.

  1. I really appreciate the new docstrings. Just for future reference, it would be nice to say what a Deferred fires with whenever it's returned from something; if it's just None, say None.
  2. StdioClientTests.checkPutMessage has an extra blank line before its parameter list.
  3. twistedchecker has a few insightful things to say about missing epytext. I realize that this is an annoying amount of duplicate documentation to write, but it's indicating a problem, which is that there's no specified interface for FileTransferClient and what its behavior is; if there were, you might be able to say what it was an implementation of and get away with that.
  4. Beyond just the epytext, openFileSideEffects has a very specific structure which is manipulated by test cases. It needs to be documented.
  5. InMemorySFTPClient.transport needs documentation as well. It is standing in for a much more specific type than ITransport, and it should say what that type is. I think it would actually be useful to make a StringTransport subclass since SSHChannel is in this sense a subtype of ITransport because it supports additional operations, and it would make it easier for test maintainers to understand the structure of the test fake if there were a parallel (documented) class.

Some subjective observations:

  1. While openFileSideEffects is enough, it would be even better to make the add-a-side-effect code into a method, so that the narrative flow of the tests made more sense, and more error-handling / bad-data-caching utilities could be enhanced into the fake over time, making it easier to debug tests.
  2. There are multiple test fakes here, InMemorySFTPClient and InMemoryRemoteFile, which expose their test-inspection state as public attributes on the same object that is given to the system under test. While widely done throughout Twisted (and the broader Python community) this is a bit of an anti-pattern. I took some time out of this review to write a blog post about it to make the more general point so that I didn't put a gigantic screed into this specific ticket that is really a more general problem. But consider splitting these test fakes up so that they don't expose more public attributes than they ought to.

All of this feedback is pretty minor refactoring or restructuring of test code, and the implementation itself is vastly better documented and more comprehensible with this change, so please address the points above to your satisfaction and then merge. Thanks again for working on this!

comment:12 Changed 13 months ago by adiroiban

  • Keywords review added
  • Owner adiroiban deleted

Thanks for the review.

For non-subjective comments:

  1. I have update the docstring to inform that deferred is fired with None
  2. Fix checkPutMessage extra space.
  3. I have not duplicated the docstrings. I think that we should update pydoctor to handle this case. For InMemoryRemoteFile we do have an Interface and pydoctor has the same complain.
  4. Done. Please check
  5. Done. Please check.

For subjective comments:

  1. True. Done.
  2. True. Thanks for the blog post. Done for InMemorySFTPClient. For InMemoryRemoteFile I kept the old code as only getvalue is the extra method and refactoring the code would result in a lot of proxy methods. The code would have be much better if we had something similar to lazr.delegates to help with composition.

Please check latest changes.

Thanks!

comment:13 Changed 13 months ago by hawkowl

  • Keywords review removed
  • Owner set to adiroiban

Tests pass, lgtm. Please land.

comment:14 Changed 13 months ago by adiroiban

Thanks. Will merge... after creating the topfile :)

Thanks again. Happy to twisted/conch/ clean of any pyflakes errors :)

comment:15 Changed 13 months ago by adiroiban

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

(In [46318]) Merge cftp-tests-7037-2: Clean cftp upload part.

Author: adiroiban Reviewer: glyph, hawkowl Fixes: #7037

Note: See TracTickets for help on using tickets.