Skip to content
This repository has been archived by the owner on May 9, 2023. It is now read-only.

Full test coverage multiaddr and fixes #9

Merged
merged 6 commits into from
May 4, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 5 additions & 8 deletions multiaddr/multiaddr.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,13 +38,9 @@ def __init__(self, string_addr=None, bytes_addr=None):

Only one of string_addr or bytes_addr may be set
"""
if string_addr is not None and bytes_addr is not None:
raise ValueError(
"Only one of 'string_addr' or 'bytes_addr' may be set")

if string_addr is not None:
if string_addr is not None and bytes_addr is None:
self._bytes = string_to_bytes(string_addr)
elif bytes_addr is not None:
elif bytes_addr is not None and string_addr is None:
self._bytes = bytes_addr
else:
raise ValueError("Invalid address type, must be bytes or str")
Expand Down Expand Up @@ -105,8 +101,9 @@ def decapsulate(self, other):
"""
s1 = str(self)
s2 = str(other)
idx = s1.rindex(s2)
if idx < 0:
try:
idx = s1.rindex(s2)
except ValueError:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, good catch!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

# if multiaddr not contained, returns a copy
return copy(self)
try:
Expand Down
56 changes: 56 additions & 0 deletions tests/test_multiaddr.py
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,11 @@ def test_encapsulate():
decapsulated_2 = decapsulated.decapsulate(m4)
assert str(decapsulated_2) == ""

m5 = Multiaddr("/ip6/::1")
decapsulated_3 = decapsulated.decapsulate(m5)

assert str(decapsulated_3) == "/ip4/127.0.0.1/udp/1234"


def assert_value_for_proto(multi, proto, expected):
assert multi.value_for_protocol(proto) == expected
Expand Down Expand Up @@ -218,3 +223,54 @@ def test_get_value():
assert_value_for_proto(a, P_IP4, "0.0.0.0")
assert_value_for_proto(a, P_UDP, "12345")
assert_value_for_proto(a, P_UTP, "")


def test_bad_initialization_no_params():
with pytest.raises(ValueError):
Multiaddr()


def test_bad_initialization_too_many_params():
with pytest.raises(ValueError):
Multiaddr("/ip4/0.0.0.0", string_to_bytes("/ip4/0.0.0.0"))


def test_get_value_too_many_fields_protocol(monkeypatch):
"""
This test patches the Multiaddr's string representation to return
an invalid string in order to test that value_for_protocol properly
throws a ValueError. This avoids some of the error checking in
the constructor and is easier to patch, thus the actual values
that the constructor specifies is ignored by the test.
"""
monkeypatch.setattr("multiaddr.multiaddr.Multiaddr.__str__",
lambda ignore: '/udp/1234/5678')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not quite sure I understand this test. It looks like you're patching Multiaddr.__str__ to return an invalid representation (and ignoring the actual constructor string, below), in order to ensure that value_for_protocol doesn't return bad results for a malformed object. Is this right?

Can you add a brief docstring saying as much, please?

Copy link
Contributor Author

@fredthomsen fredthomsen May 3, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, exactly right. Ignoring the constructor does make this test look odd.

I will add docstring specifying this as suggested. Also additionally would you like docstrings on other tests as well or just the harder to understand ones? I am not sure what the best practice is in this case.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also additionally would you like docstrings on other tests as well or just the harder to understand ones?

Only on hard to understand ones is my typical heuristic.

Ignoring the constructor does make this test look odd.

The necessity of a monkeypatch here probably means I designed the Multiaddr class poorly. I'll keep this in the back of my mind for a refactor sometime later, but for now patching is a-ok.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The necessity of a monkeypatch here probably means I designed the Multiaddr class poorly. I'll keep this in the back of my mind for a refactor sometime later, but for now patching is a-ok.

Not sure I agree in general although maybe in this case. This may very well be a code path that could never get hit in reality.

a = Multiaddr("/ip4/127.0.0.1/udp/1234")
with pytest.raises(ValueError):
a.value_for_protocol(P_UDP)


def test_multi_addr_str_corruption():
a = Multiaddr("/ip4/127.0.0.1/udp/1234")
a._bytes = b"047047047"

with pytest.raises(ValueError):
str(a)


def test_decapsulate_corrupted_bytes(monkeypatch):
def raiseException(self, other):
raise Exception

a = Multiaddr("/ip4/127.0.0.1/udp/1234")
u = Multiaddr("/udp/1234")
monkeypatch.setattr("multiaddr.multiaddr.Multiaddr.__init__",
raiseException)

with pytest.raises(ValueError):
a.decapsulate(u)


def test__repr():
a = Multiaddr("/ip4/127.0.0.1/udp/1234")
assert(repr(a) == "<Multiaddr %s>" % str(a))