Merge pull request #65 from HPENetworking/connect_extend

fix: dev: Several fixes in the usage of the connection argument.

lib/topology/platforms/shell.py

@@ -235,7 +235,7 @@ def is_connected(self, connection=None):
         """
 
     @abstractmethod
-    def connect(self, connection=None, *args, **kwargs):
+    def connect(self, *args, connection=None, **kwargs):
         """
         Creates a connection to the shell.
 
@@ -248,7 +248,7 @@ def connect(self, connection=None, *args, **kwargs):
         """
 
     @abstractmethod
-    def disconnect(self, connection=None, *args, **kwargs):
+    def disconnect(self, *args, connection=None, **kwargs):
         """
         Terminates a connection to the shell.
 
@@ -281,7 +281,7 @@ def execute(self, command, connection=None):
     def __call__(self, command, connection=None):
         return self.execute(command, connection=connection)
 
-    def _setup_shell(self, connection=None, *args, **kwargs):
+    def _setup_shell(self, *args, connection=None, **kwargs):
         """
         Method called by subclasses that will be triggered after matching the
         initial prompt.
@@ -365,13 +365,14 @@ class PExpectShell(BaseShell):
     """
 
     def __init__(
-            self, prompt,
-            initial_command=None, initial_prompt=None,
-            user=None, user_match='[uU]ser:',
-            password=None, password_match='[pP]assword:',
-            prefix=None, timeout=None, encoding='utf-8',
-            try_filter_echo=True, auto_connect=True,
-            spawn_args=None, errors='ignore', **kwargs):
+        self, prompt,
+        initial_command=None, initial_prompt=None,
+        user=None, user_match='[uU]ser:',
+        password=None, password_match='[pP]assword:',
+        prefix=None, timeout=None, encoding='utf-8',
+        try_filter_echo=True, auto_connect=True,
+        spawn_args=None, errors='ignore', **kwargs
+    ):
 
         self._connections = OrderedDict()
         self._default_connection = None
@@ -434,7 +435,7 @@ def _get_connect_command(self):
         :return: The command to be used when connecting to the shell.
         """
 
-    def _get_connection(self, connection):
+    def _get_connection(self, connection=None):
         """
         Get the pexpect object for the given connection name.
 
@@ -466,20 +467,20 @@ def send_command(
         # 1. Connection is missing
         # 2. Connection is disconnected
         if not self._auto_connect:
-            if not self.is_connected(connection):
+            if not self.is_connected(connection=connection):
                 raise DisconnectedError(connection)
 
         # If auto connect is true, always reconnect unless:
         # 1. Connection already exists, and
         # 2. Connection is connected
         else:
             try:
-                if not self.is_connected(connection):
-                    self.connect(connection)
+                if not self.is_connected(connection=connection):
+                    self.connect(connection=connection)
             except NonExistingConnectionError:
-                self.connect(connection)
+                self.connect(connection=connection)
 
-        spawn = self._get_connection(connection)
+        spawn = self._get_connection(connection=connection)
 
         # Create possible expect matches
         if matches is None:
@@ -520,7 +521,7 @@ def get_response(self, connection=None, silent=False):
         See :meth:`BaseShell.get_response` for more information.
         """
         # Get connection
-        spawn = self._get_connection(connection)
+        spawn = self._get_connection(connection=connection)
 
         # Convert binary representation to unicode using encoding
         text = spawn.before.decode(
@@ -558,52 +559,65 @@ def is_connected(self, connection=None):
         See :meth:`BaseShell.is_connected` for more information.
         """
         # Get connection
-        spawn = self._get_connection(connection)
+        spawn = self._get_connection(connection=connection)
         return spawn.isalive()
 
-    def connect(self, connection=None, *args, **kwargs):
+    def connect(self, *args, connection=None, **kwargs):
         """
         See :meth:`BaseShell.connect` for more information.
         """
         connection = connection or self._default_connection or '0'
 
-        if connection in self._connections and self.is_connected(connection):
+        connection_is_present = connection in self._connections
+
+        if connection_is_present and self.is_connected(connection=connection):
             raise AlreadyConnectedError(connection)
 
         spawn = Spawn(
             self._get_connect_command().strip(),
             **self._spawn_args
         )
 
-        spawn.logfile_read = get_logger(
-            OrderedDict([
-                ('node_identifier', self._node_identifier),
-                ('shell_name', self._shell_name),
-                ('connection', connection)
-            ]),
-            category='pexpect_read',
-        )
+        # If the disconnect is called on a connection and then connect is
+        # called again on the same connection, this will be called twice,
+        # making each message from that connection to be logged twice.
+        if connection_is_present:
+            present_connection = self._connections[connection]
 
-        spawn.logfile_send = get_logger(
-            OrderedDict([
-                ('node_identifier', self._node_identifier),
-                ('shell_name', self._shell_name),
-                ('connection', connection)
-            ]),
-            category='pexpect_send',
-        )
+            spawn.logfile_read = present_connection.logfile_read
+            spawn.logfile_send = present_connection.logfile_send
+            spawn._connection_logger = present_connection._connection_logger
 
-        # Add a connection logger
-        # Note: self._node and self._name were added to this shell in the
-        #       node's call to its _register_shell method.
-        spawn._connection_logger = get_logger(
-            OrderedDict([
-                ('node_identifier', self._node_identifier),
-                ('shell_name', self._shell_name),
-                ('connection', connection)
-            ]),
-            category='connection'
-        )
+        else:
+            spawn.logfile_read = get_logger(
+                OrderedDict([
+                    ('node_identifier', self._node_identifier),
+                    ('shell_name', self._shell_name),
+                    ('connection', connection)
+                ]),
+                category='pexpect_read',
+            )
+
+            spawn.logfile_send = get_logger(
+                OrderedDict([
+                    ('node_identifier', self._node_identifier),
+                    ('shell_name', self._shell_name),
+                    ('connection', connection)
+                ]),
+                category='pexpect_send',
+            )
+
+            # Add a connection logger
+            # Note: self._node and self._name were added to this shell in the
+            #       node's call to its _register_shell method.
+            spawn._connection_logger = get_logger(
+                OrderedDict([
+                    ('node_identifier', self._node_identifier),
+                    ('shell_name', self._shell_name),
+                    ('connection', connection)
+                ]),
+                category='connection'
+            )
 
         self._connections[connection] = spawn
 
@@ -625,7 +639,7 @@ def expect_sendline(prompt, command):
             expect_sendline(self._initial_prompt, self._initial_command)
 
             # Setup shell before using it
-            self._setup_shell(connection)
+            self._setup_shell(*args, connection=connection, **kwargs)
 
             # Wait for command response to match the prompt
             spawn.expect(
@@ -641,12 +655,12 @@ def expect_sendline(prompt, command):
         if self.default_connection is None:
             self.default_connection = connection
 
-    def disconnect(self, connection=None, *args, **kwargs):
+    def disconnect(self, *args, connection=None, **kwargs):
         """
         See :meth:`BaseShell.disconnect` for more information.
         """
         # Get connection
-        spawn = self._get_connection(connection)
+        spawn = self._get_connection(connection=connection)
         if not spawn.isalive():
             raise AlreadyDisconnectedError(connection)
         spawn.close()
@@ -709,12 +723,12 @@ def __init__(
             **kwargs
         )
 
-    def _setup_shell(self, connection=None, *args, **kwargs):
+    def _setup_shell(self, *args, connection=None, **kwargs):
         """
         Overriden setup function that will disable the echo on the device on
         the shell and set a pexpect-safe prompt.
         """
-        spawn = self._get_connection(connection)
+        spawn = self._get_connection(connection=connection)
 
         # Wait initial prompt
         spawn.expect(

test/test_topology_platforms_shell.py

@@ -22,25 +22,19 @@
 from __future__ import unicode_literals, absolute_import
 from __future__ import print_function, division
 
-# mock is located in unittest from Python 3.3 onwards, but as an external
-# package in Python 2.7, that is why the following is done:
-try:
-    from unittest.mock import patch, Mock, call
-
-    # Work around for http://bugs.python.org/issue25532
-    # Prevents infinite memory allocation
-    call.__wrapped__ = None
-
-except ImportError:
-    from mock import patch, Mock, call
-
 from pytest import fixture, raises
 
 from topology.platforms.shell import (
     PExpectBashShell, NonExistingConnectionError, AlreadyDisconnectedError,
     DisconnectedError
 )
 
+from unittest.mock import patch, Mock, call
+
+# Work around for http://bugs.python.org/issue25532
+# Prevents infinite memory allocation
+call.__wrapped__ = None
+
 
 class Shell(PExpectBashShell):
     def __init__(self, prompt, **kwargs):
@@ -331,7 +325,7 @@ def test_connect(spawn, shell):
     class SetupShellError(Exception):
         pass
 
-    def _setup_shell(self, connection=None):
+    def _setup_shell(*args, connection=None, **kwargs):
         raise SetupShellError
 
     shell._setup_shell = _setup_shell
@@ -344,30 +338,29 @@ def test_connect_disconnect_connect(spawn, shell):
     """
     Test that the connect - disconnect - connect use case works properly.
     """
-    for connection in [None, '1']:
+    for connection in ['0', '1']:
 
-        # Shell not created yet
+        # Connection not created yet
         with raises(NonExistingConnectionError):
             shell.is_connected(connection=connection)
 
         # First shell call and explicit reconnection case
-        for i in [1, 2]:
-            shell.connect(connection=connection)
-            assert shell.is_connected(connection=connection)
+        shell.connect(connection=connection)
 
-            shell.send_command('command'.format(i), connection=connection)
-            shell._connections[connection or '0'].sendline.assert_called_with(
-                'command'.format(i)
-            )
-            assert shell.is_connected(connection=connection)
+        assert shell.is_connected(connection=connection)
 
-            shell.disconnect(connection=connection)
-            assert not shell.is_connected(connection=connection)
+        shell.send_command('command 0', connection=connection)
+
+        shell._connections[connection].sendline.assert_called_with('command 0')
+
+        shell.disconnect(connection=connection)
 
-        # Second case, automatic reconnect
         assert not shell.is_connected(connection=connection)
-        shell.send_command('a call to'.format(i), connection=connection)
-        shell._connections[connection or '0'].sendline.assert_called_with(
-            'a call to'
-        )
+
+        # Second case, automatic reconnect
+
+        shell.send_command('command 1', connection=connection)
+
+        shell._connections[connection].sendline.assert_called_with('command 1')
+
         assert shell.is_connected(connection=connection)

tox.ini

@@ -1,5 +1,5 @@
 [tox]
-envlist = py27, py3, coverage, doc
+envlist = py3, coverage, doc
 
 [testenv]
 passenv = http_proxy https_proxy
@@ -30,11 +30,6 @@ commands =
         {posargs:--topology-platform debug} \
         {toxinidir}/test {envsitepackagesdir}/topology
 
-[testenv:py27]
-deps =
-    -rrequirements.dev.txt
-    -rrequirements.py27.txt
-
 [testenv:doc]
 basepython = python3
 deps =