Opened 6 years ago

Closed 5 years ago

#6255 defect closed fixed (fixed)

Can't compare KnownHostsFile instances

Reported by: Jean-Paul Calderone Owned by: Jean-Paul Calderone
Priority: normal Milestone:
Component: conch Keywords: easy
Cc: z3p, Rachee Singh Branch: branches/knownhostsfile-savepath-6255
branch-diff, diff-cov, branch-cov, buildbot
Author: exarkun

Description (last modified by Itamar Turner-Trauring)

This makes it difficult to test code using KnownHostsFile, for example to determine if the code has opened the right file.

The only identifying attribute of instances of this class is private, _savePath. Making it public would be sufficient for testing purposes, since then code could at least just directly compare the path. The attribute should be made public in a read-only manner, however, since current code does not support changing it.

Attachments (2)

ticket6255.patch (421 bytes) - added by Rachee Singh 6 years ago.
Patch file for the proposed fix for Ticket #6255
ticket6255-round2.patch (2.7 KB) - added by Rachee Singh 6 years ago.
Patch file for the proposed fix for Ticket #6255, incorporating review comments.

Download all attachments as: .zip

Change History (25)

comment:1 Changed 6 years ago by DefaultCC Plugin

Cc: z3p added

comment:2 Changed 6 years ago by Itamar Turner-Trauring

Keywords: easy added

comment:3 Changed 6 years ago by Jean-Paul Calderone

There's an "or" in the ticket description. For this ticket to actually be easy, that should probably be removed.

comment:4 Changed 6 years ago by Itamar Turner-Trauring

Description: modified (diff)

I updated the ticket's description.

For the record, I removed the following alternate suggestion, on the basis that comparison of contents might be another thing to compare, and it's not clear which one is the natural one for built-in operators:

Alternatively `KnownHostsFile` could support `==` and `!=` directly (via `__eq__` and `__ne__` - or better, `FancyEqMixin`).

comment:5 Changed 6 years ago by Rachee Singh

Owner: set to Rachee Singh
Status: newassigned

comment:6 Changed 6 years ago by Rachee Singh

As per what I understood from the ticket description and comments, the preferable thing to do here is:

  1. Make _savePath public.
  2. Make _savePath read-only so no one tries to modify it (mistakenly).

Does that sound correct?

Should the patch include a test case to ensure that the proposed variable "savePath" (no longer private) cannot be written to (we could expect an Exception in the test case)?

comment:7 Changed 6 years ago by Itamar Turner-Trauring

Those two are the requirements, not necessarily the implementation. You could achieve both by having a function that returns _savePath.

Any expected behavior should be tested. If you're writing a function that returns _savePath there's no public API to write to _savePath though, so there's nothing to test for writing.

comment:8 in reply to:  7 ; Changed 6 years ago by Rachee Singh

Thanks for the help. Consolidating what I have understood in terms of how we would go about fixing it:

def init(self, savePath):

... self._savePath = savePath ...

+ @property + def savePath: + return self._savePath

_savePath can be accessed by knownHostsFile.savePath. But knownHostsFile.savePath = "abc" would do result in: AttributeError: can't set attribute

Does that sound okay?

comment:9 in reply to:  8 Changed 6 years ago by Rachee Singh

I apologize for the bad formatting, here is the cleaner looking snippet from the last comment I posted:

    def __init__(self, savePath):
        ...
        self._savePath = savePath
        ...

+    @property
+    def savePath:
+        return self._savePath

comment:10 Changed 6 years ago by Rachee Singh

Cc: Rachee Singh added

comment:11 Changed 6 years ago by Jean-Paul Calderone

Does that sound okay?

Please attach changes as diff files, don't paste them as inline comments. Also please read ReviewProcess and follow the steps outlined there. Feel free to ask questions about the process - perhaps on the mailing list or the #twisted-dev irc channel - if any part of it isn't clear.

Changed 6 years ago by Rachee Singh

Attachment: ticket6255.patch added

Patch file for the proposed fix for Ticket #6255

comment:12 Changed 6 years ago by Rachee Singh

Keywords: review added
Owner: Rachee Singh deleted
Status: assignednew

comment:13 Changed 6 years ago by Julian Berman

Keywords: review removed
Owner: set to Rachee Singh

This looks OK, but as in your other patch, needs to be tested :).

Take exarkun up on his offer and read that link and come ask questions if you have any.

comment:14 Changed 6 years ago by Rachee Singh

Thanks :-). As per comment #7, I don't know what test cases I can add in. But I did look at twisted/conch/test/test_knownhosts.py. It has a few statements like:

self.assertEqual(exception.path, hostsFile._savePath)

Shouldn't these be changed to:

self.assertEqual(exception.path, hostsFile.savePath)

if the proposed fix was to be accepted?

comment:15 Changed 6 years ago by Julian Berman

Itamar was referring to testing writing the attribute (I.e. testing h.savePath = 12), which you don't have to do because it's not a thing that will be supported. In other words, you don't have to write a test here to ensure that a thing that isn't supported isn't supported.

Reading the attribute is though, so at least one test should use the public way to access it.

Changing the existing tests to use your property would sound fine, but I haven't looked at the actual code for the tests specifically yet. I'd recommend doing that and submitting that for review.

comment:16 Changed 6 years ago by Rachee Singh

Thanks for the feedback, I changed twisted/conch/test/test_knownhosts.py to use hostsFile.savePath instead of hostsFile._savePath. The diff incorporating your comments is attached.

Changed 6 years ago by Rachee Singh

Attachment: ticket6255-round2.patch added

Patch file for the proposed fix for Ticket #6255, incorporating review comments.

comment:17 Changed 6 years ago by Julian Berman

Keywords: review added

Hey, sorry, this got lost. It's meant to be up for review right?

Putting it up, I'll try to take a look myself tonight, though from a quick glance it looks good.

comment:18 Changed 6 years ago by Julian Berman

Owner: Rachee Singh deleted

comment:19 Changed 5 years ago by Jean-Paul Calderone

Author: exarkun
Branch: branches/knownhostsfile-savepath-6255

(In [38492]) Branching to 'knownhostsfile-savepath-6255'

comment:20 Changed 5 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to Jean-Paul Calderone

Thanks for your work on this racheesingh.

A few comments:

  1. Methods should be separated from each other by two blank lines
  2. savePath should have a docstring
  3. The read-only testing issue here is perhaps a little bit subtle. If savePath were an attribute, then I would say that testing the behavior of assigning to it is not necessary. Documenting that it should not have its value changed would be sufficient. However, the patch implements savePath as a read-only property. The property definition is extra code beyond an attribute definition, and it seems like it merits some test coverage. Fortunately this is a really easy test to write, so it doesn't hurt to err on the side of more testing in this case.
  4. The change should have a news fragment. This is a feature addition, and it's not a very big one but conch isn't likely to have a lot of feature additions in the next release so we may as well advertise it.

These changes are minor, I'll make them, verify buildbot likes them, and then merge. Thanks again.

comment:21 Changed 5 years ago by Jean-Paul Calderone

(In [38493]) apply patch with fixes

refs #6255

comment:23 Changed 5 years ago by Jean-Paul Calderone

Resolution: fixed
Status: newclosed

(In [38494]) Merge knownhostsfile-savepath-6255

Author: racheesingh Reviewer: Julian, exarkun Fixes: #6255

Give twisted.conch.client.knownhosts.KnownHostsFile a public savePath attribute which identifies the filesystem path to which information is saved and from which it is loaded.

Note: See TracTickets for help on using tickets.