jenkins-bot has submitted this change and it was merged. (
https://gerrit.wikimedia.org/r/399829 )
Change subject: ClusterShell backend: fix execute() return code
..
ClusterShell backend: fix execute() return code
* The return code of the execute() method was not respecting the parent
class contract for its return code when there are no commands set or
no hosts to target.
* Breaking API change: make the Target class raise an WorkerError
exception on instantiation if there are no target hosts.
* Breaking API change: make the execute() method raise a WorkerError
exception if there are no commands to execute.
Change-Id: Idb59df41dac12d78803d560af3c22da9f1df4d1a
---
M cumin/tests/unit/test_transport.py
M cumin/tests/unit/transports/test_clustershell.py
M cumin/tests/unit/transports/test_init.py
M cumin/transports/__init__.py
M cumin/transports/clustershell.py
5 files changed, 25 insertions(+), 24 deletions(-)
Approvals:
jenkins-bot: Verified
Gehel: Looks good to me, but someone else must approve
Volans: Looks good to me, approved
diff --git a/cumin/tests/unit/test_transport.py
b/cumin/tests/unit/test_transport.py
index 52159b3..b821141 100644
--- a/cumin/tests/unit/test_transport.py
+++ b/cumin/tests/unit/test_transport.py
@@ -12,13 +12,13 @@
def test_missing_transport():
"""Not passing a transport should raise CuminError."""
with pytest.raises(CuminError, match=r"Missing required parameter
'transport'"):
-Transport.new({}, transports.Target([]))
+Transport.new({}, transports.Target(['host1']))
def test_invalid_transport():
"""Passing an invalid transport should raise CuminError."""
with pytest.raises(ImportError, match=r'No module named
non_existent_transport'):
-Transport.new({'transport': 'non_existent_transport'},
transports.Target([]))
+Transport.new({'transport': 'non_existent_transport'},
transports.Target(['host1']))
def test_missing_worker_class():
@@ -27,11 +27,11 @@
del module.worker_class
with mock.patch('importlib.import_module', lambda _: module):
with pytest.raises(AttributeError, match=r'worker_class'):
-Transport.new({'transport': 'invalid_transport'},
transports.Target([]))
+Transport.new({'transport': 'invalid_transport'},
transports.Target(['host1']))
@pytest.mark.parametrize('transport', [name for _, name, ispkg in
pkgutil.iter_modules(transports.__path__)
if not ispkg])
def test_valid_transport(transport):
"""Passing a valid transport should return an instance of BaseWorker."""
-assert isinstance(Transport.new({'transport': transport},
transports.Target([])), transports.BaseWorker)
+assert isinstance(Transport.new({'transport': transport},
transports.Target(['host1'])), transports.BaseWorker)
diff --git a/cumin/tests/unit/transports/test_clustershell.py
b/cumin/tests/unit/transports/test_clustershell.py
index ccaba3f..6387984 100644
--- a/cumin/tests/unit/transports/test_clustershell.py
+++ b/cumin/tests/unit/transports/test_clustershell.py
@@ -96,23 +96,18 @@
assert kwargs['handler'] == self.worker._handler_instance
def test_execute_no_commands(self):
-"""Calling execute() without commands should return immediately."""
+"""Calling execute() without commands should raise WorkerError."""
self.worker.handler = ConcreteBaseEventHandler
self.worker.commands = None
-assert self.worker.execute() is None
+with pytest.raises(WorkerError, match=r'No commands provided\.'):
+self.worker.execute()
assert not self.worker.task.shell.called
def test_execute_one_command_no_mode(self):
-"""Calling execute() with only one command without mode should raise
CuminError."""
+"""Calling execute() with only one command without mode should raise
WorkerError."""
self.worker.commands = [self.commands[0]]
-with pytest.raises(CuminError, match=r'An EventHandler is
mandatory\.'):
+with pytest.raises(WorkerError, match=r'An EventHandler is
mandatory\.'):
self.worker.execute()
-
-def test_execute_no_target_hosts(self):
-"""Calling execute() with a target without hosts should return
immediately."""
-self.target.hosts = []
-self.worker.handler = ConcreteBaseEventHandler
-assert self.worker.execute() is None
assert not self.worker.task.shell.called
def test_execute_wrong_mode(self):
diff --git a/cumin/tests/unit/transports/test_init.py
b/cumin/tests/unit/transports/test_init.py
index c40dfdc..d326d0e 100644
--- a/cumin/tests/unit/transports/test_init.py
+++ b/cumin/tests/unit/transports/test_init.py
@@ -316,6 +316,11 @@
self.hosts_list = ['host' + str(i) for i in xrange(10)]
self.hosts = NodeSet.fromlist(self.hosts_list)
+def
