kytos / pathfinder

Kytos main path finder Network Application (NApp)
https://napps.kytos.io/kytos/pathfinder
MIT License
1 stars 21 forks source link

Change keys.extend(..) to keys.append(..) #65

Closed MarvinTorres closed 4 years ago

MarvinTorres commented 4 years ago

@hdiogenes

This fixes #64. This also updates test_update_links so its keys setup implementation matches update_links.

The unit test for update_links did not catch this error because it filled up the array differently:

test = ["apple", "banana", "grape"]
test2 = []

test3 = []

# update_links implementation
for t in test:
    test2.extend(t)

# test_update_links implementation
test3.extend(t for t in test)

test2 is ['a', 'p', 'p', 'l', 'e', 'b', 'a', 'n', 'a', 'n', 'a', 'g', 'r', 'a', 'p', 'e'] test3 is ['apple', 'banana', 'grape'], as intended.

In addition, it uses a helper method get_link_mock from kytos.lib.helpers to set up a topology, and this topology uses single-letter attributes for each of its links:

def get_link_mock(endpoint_a, endpoint_b):
    """Return a link mock."""
    link = create_autospec(Link)
    link.endpoint_a = endpoint_a
    link.endpoint_b = endpoint_b
    link.metadata = {"A": 0}
    return link

And #64 does not occur if update_links works with links that have only single-letter attributes. Namely, this code would detect a difference in metadata names and throw an AssertionError if it was working with multi-letter attributes:

        for metadata in all_metadata:
            keys.extend(key for key in metadata.keys())
        # This is where the assertion would fail
        mock_set_default_metadata.assert_called_with(keys)

But each link has only one attribute and that attribute is a single letter. So the test sees the equal lists and incorrectly assumes that its method under test is working as expected.

hdiogenes commented 4 years ago

64 does not occur if update_links works with links that have only single-letter attributes. [...]

But each link has only one attribute and that attribute is a single letter. So the test sees the equal lists and incorrectly assumes that its method under test is working as expected.

If the tests can't expose the problem because they only test single-letter attributes, the PR could be more complete if you changed the tests so that they can break, so they could:

  1. Expose the underlying problem in the current implementation;
  2. Prove that your modification fixes it;
  3. Prevent the bug from being introduced again in the future.
MarvinTorres commented 4 years ago

@hdiogenes The method that returns mock links with single-letter attributes is in the Kytos core. I will create an issue in the core repository.