Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from
Sep 16, 2020

Conversation

MarvinTorres
Copy link
Contributor

@MarvinTorres MarvinTorres commented Aug 18, 2020

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

@MarvinTorres MarvinTorres marked this pull request as draft August 18, 2020 03:06
@MarvinTorres MarvinTorres changed the title Changed keys.extend(..) to keys.append(..) Change keys.extend(..) to keys.append(..) Aug 18, 2020
  - This hopefully ensures that kytos#64 will not reoccur.
@MarvinTorres MarvinTorres marked this pull request as ready for review August 19, 2020 02:29
@hdiogenes hdiogenes requested review from a team, cmagnobarbosa and josemauro and removed request for a team September 9, 2020 19:59
tests/unit/test_graph.py Outdated Show resolved Hide resolved
graph.py Show resolved Hide resolved
@hdiogenes
Copy link
Member

hdiogenes commented Sep 9, 2020

#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
Copy link
Contributor Author

MarvinTorres commented Sep 11, 2020

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

@hdiogenes hdiogenes added this to the 2020.2 milestone Sep 16, 2020
@hdiogenes hdiogenes merged commit d8ba617 into kytos:master Sep 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

KytosGraph adds excessive metadata to its links
2 participants