Skip to content

Commit a8dcad9

Browse files
smoserServer Team CI Bot
smoser
authored and
Server Team CI Bot
committed
tests: Disallow use of util.subp except for where needed.
In many cases, cloud-init uses 'util.subp' to run a subprocess. This is not really desirable in our unit tests as it makes the tests dependent upon existance of those utilities. The change here is to modify the base test case class (CiTestCase) to raise exception any time subp is called. Then, fix all callers. For cases where subp is necessary or actually desired, we can use it via   a.) context hander CiTestCase.allow_subp(value)   b.) class level self.allowed_subp = value Both cases the value is a list of acceptable executable names that will be called (essentially argv[0]). Some cleanups in AltCloud were done as the code was being updated.
1 parent db50bc0 commit a8dcad9

23 files changed

+289
-175
lines changed

‎cloudinit/analyze/tests/test_dump.py‎

Lines changed: 33 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -5,8 +5,8 @@
55

66
from cloudinit.analyze.dump import (
77
dump_events, parse_ci_logline, parse_timestamp)
8-
from cloudinit.util import subp, write_file
9-
from cloudinit.tests.helpers import CiTestCase
8+
from cloudinit.util import which, write_file
9+
from cloudinit.tests.helpers import CiTestCase, mock, skipIf
1010

1111

1212
class TestParseTimestamp(CiTestCase):
@@ -15,21 +15,9 @@ def test_parse_timestamp_handles_cloud_init_default_format(self):
1515
"""Logs with cloud-init detailed formats will be properly parsed."""
1616
trusty_fmt = '%Y-%m-%d %H:%M:%S,%f'
1717
trusty_stamp = '2016-09-12 14:39:20,839'
18-
19-
parsed = parse_timestamp(trusty_stamp)
20-
21-
# convert ourselves
2218
dt = datetime.strptime(trusty_stamp, trusty_fmt)
23-
expected = float(dt.strftime('%s.%f'))
24-
25-
# use date(1)
26-
out, _err = subp(['date', '+%s.%3N', '-d', trusty_stamp])
27-
timestamp = out.strip()
28-
date_ts = float(timestamp)
29-
30-
self.assertEqual(expected, parsed)
31-
self.assertEqual(expected, date_ts)
32-
self.assertEqual(date_ts, parsed)
19+
self.assertEqual(
20+
float(dt.strftime('%s.%f')), parse_timestamp(trusty_stamp))
3321

3422
def test_parse_timestamp_handles_syslog_adding_year(self):
3523
"""Syslog timestamps lack a year. Add year and properly parse."""
@@ -39,17 +27,9 @@ def test_parse_timestamp_handles_syslog_adding_year(self):
3927
# convert stamp ourselves by adding the missing year value
4028
year = datetime.now().year
4129
dt = datetime.strptime(syslog_stamp + " " + str(year), syslog_fmt)
42-
expected = float(dt.strftime('%s.%f'))
43-
parsed = parse_timestamp(syslog_stamp)
44-
45-
# use date(1)
46-
out, _ = subp(['date', '+%s.%3N', '-d', syslog_stamp])
47-
timestamp = out.strip()
48-
date_ts = float(timestamp)
49-
50-
self.assertEqual(expected, parsed)
51-
self.assertEqual(expected, date_ts)
52-
self.assertEqual(date_ts, parsed)
30+
self.assertEqual(
31+
float(dt.strftime('%s.%f')),
32+
parse_timestamp(syslog_stamp))
5333

5434
def test_parse_timestamp_handles_journalctl_format_adding_year(self):
5535
"""Journalctl precise timestamps lack a year. Add year and parse."""
@@ -59,37 +39,22 @@ def test_parse_timestamp_handles_journalctl_format_adding_year(self):
5939
# convert stamp ourselves by adding the missing year value
6040
year = datetime.now().year
6141
dt = datetime.strptime(journal_stamp + " " + str(year), journal_fmt)
62-
expected = float(dt.strftime('%s.%f'))
63-
parsed = parse_timestamp(journal_stamp)
64-
65-
# use date(1)
66-
out, _ = subp(['date', '+%s.%6N', '-d', journal_stamp])
67-
timestamp = out.strip()
68-
date_ts = float(timestamp)
69-
70-
self.assertEqual(expected, parsed)
71-
self.assertEqual(expected, date_ts)
72-
self.assertEqual(date_ts, parsed)
42+
self.assertEqual(
43+
float(dt.strftime('%s.%f')), parse_timestamp(journal_stamp))
7344

45+
@skipIf(not which("date"), "'date' command not available.")
7446
def test_parse_unexpected_timestamp_format_with_date_command(self):
75-
"""Dump sends unexpected timestamp formats to data for processing."""
47+
"""Dump sends unexpected timestamp formats to date for processing."""
7648
new_fmt = '%H:%M %m/%d %Y'
7749
new_stamp = '17:15 08/08'
78-
7950
# convert stamp ourselves by adding the missing year value
8051
year = datetime.now().year
8152
dt = datetime.strptime(new_stamp + " " + str(year), new_fmt)
82-
expected = float(dt.strftime('%s.%f'))
83-
parsed = parse_timestamp(new_stamp)
8453

8554
# use date(1)
86-
out, _ = subp(['date', '+%s.%6N', '-d', new_stamp])
87-
timestamp = out.strip()
88-
date_ts = float(timestamp)
89-
90-
self.assertEqual(expected, parsed)
91-
self.assertEqual(expected, date_ts)
92-
self.assertEqual(date_ts, parsed)
55+
with self.allow_subp(["date"]):
56+
self.assertEqual(
57+
float(dt.strftime('%s.%f')), parse_timestamp(new_stamp))
9358

9459

9560
class TestParseCILogLine(CiTestCase):
@@ -135,7 +100,9 @@ def test_parse_logline_returns_event_for_journalctl_logs(self):
135100
'timestamp': timestamp}
136101
self.assertEqual(expected, parse_ci_logline(line))
137102

138-
def test_parse_logline_returns_event_for_finish_events(self):
103+
@mock.patch("cloudinit.analyze.dump.parse_timestamp_from_date")
104+
def test_parse_logline_returns_event_for_finish_events(self,
105+
m_parse_from_date):
139106
"""parse_ci_logline returns a finish event for a parsed log line."""
140107
line = ('2016-08-30 21:53:25.972325+00:00 y1 [CLOUDINIT]'
141108
' handlers.py[DEBUG]: finish: modules-final: SUCCESS: running'
@@ -147,7 +114,10 @@ def test_parse_logline_returns_event_for_finish_events(self):
147114
'origin': 'cloudinit',
148115
'result': 'SUCCESS',
149116
'timestamp': 1472594005.972}
117+
m_parse_from_date.return_value = "1472594005.972"
150118
self.assertEqual(expected, parse_ci_logline(line))
119+
m_parse_from_date.assert_has_calls(
120+
[mock.call("2016-08-30 21:53:25.972325+00:00")])
151121

152122

153123
SAMPLE_LOGS = dedent("""\
@@ -162,10 +132,16 @@ def test_parse_logline_returns_event_for_finish_events(self):
162132
class TestDumpEvents(CiTestCase):
163133
maxDiff = None
164134

165-
def test_dump_events_with_rawdata(self):
135+
@mock.patch("cloudinit.analyze.dump.parse_timestamp_from_date")
136+
def test_dump_events_with_rawdata(self, m_parse_from_date):
166137
"""Rawdata is split and parsed into a tuple of events and data"""
138+
m_parse_from_date.return_value = "1472594005.972"
167139
events, data = dump_events(rawdata=SAMPLE_LOGS)
168140
expected_data = SAMPLE_LOGS.splitlines()
141+
self.assertEqual(
142+
[mock.call("2016-08-30 21:53:25.972325+00:00")],
143+
m_parse_from_date.call_args_list)
144+
self.assertEqual(expected_data, data)
169145
year = datetime.now().year
170146
dt1 = datetime.strptime(
171147
'Nov 03 06:51:06.074410 %d' % year, '%b %d %H:%M:%S.%f %Y')
@@ -183,12 +159,14 @@ def test_dump_events_with_rawdata(self):
183159
'result': 'SUCCESS',
184160
'timestamp': 1472594005.972}]
185161
self.assertEqual(expected_events, events)
186-
self.assertEqual(expected_data, data)
187162

188-
def test_dump_events_with_cisource(self):
163+
@mock.patch("cloudinit.analyze.dump.parse_timestamp_from_date")
164+
def test_dump_events_with_cisource(self, m_parse_from_date):
189165
"""Cisource file is read and parsed into a tuple of events and data."""
190166
tmpfile = self.tmp_path('logfile')
191167
write_file(tmpfile, SAMPLE_LOGS)
168+
m_parse_from_date.return_value = 1472594005.972
169+
192170
events, data = dump_events(cisource=open(tmpfile))
193171
year = datetime.now().year
194172
dt1 = datetime.strptime(
@@ -208,3 +186,5 @@ def test_dump_events_with_cisource(self):
208186
'timestamp': 1472594005.972}]
209187
self.assertEqual(expected_events, events)
210188
self.assertEqual(SAMPLE_LOGS.splitlines(), [d.strip() for d in data])
189+
m_parse_from_date.assert_has_calls(
190+
[mock.call("2016-08-30 21:53:25.972325+00:00")])

‎cloudinit/cmd/tests/test_status.py‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,8 @@ def test__is_cloudinit_disabled_false_on_sysvinit(self):
3939
ensure_file(self.disable_file) # Create the ignored disable file
4040
(is_disabled, reason) = wrap_and_call(
4141
'cloudinit.cmd.status',
42-
{'uses_systemd': False},
42+
{'uses_systemd': False,
43+
'get_cmdline': "root=/dev/my-root not-important"},
4344
status._is_cloudinit_disabled, self.disable_file, self.paths)
4445
self.assertFalse(
4546
is_disabled, 'expected enabled cloud-init on sysvinit')
@@ -50,7 +51,8 @@ def test__is_cloudinit_disabled_true_on_disable_file(self):
5051
ensure_file(self.disable_file) # Create observed disable file
5152
(is_disabled, reason) = wrap_and_call(
5253
'cloudinit.cmd.status',
53-
{'uses_systemd': True},
54+
{'uses_systemd': True,
55+
'get_cmdline': "root=/dev/my-root not-important"},
5456
status._is_cloudinit_disabled, self.disable_file, self.paths)
5557
self.assertTrue(is_disabled, 'expected disabled cloud-init')
5658
self.assertEqual(

‎cloudinit/config/tests/test_snap.py‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -162,6 +162,7 @@ def test_add_assertions_adds_assertions_as_dict(self, m_subp):
162162
class TestRunCommands(CiTestCase):
163163

164164
with_logs = True
165+
allowed_subp = [CiTestCase.SUBP_SHELL_TRUE]
165166

166167
def setUp(self):
167168
super(TestRunCommands, self).setUp()
@@ -424,8 +425,10 @@ def test_handle_runs_commands_provided(self):
424425
'snap': {'commands': ['echo "HI" >> %s' % outfile,
425426
'echo "MOM" >> %s' % outfile]}}
426427
mock_path = 'cloudinit.config.cc_snap.sys.stderr'
427-
with mock.patch(mock_path, new_callable=StringIO):
428-
handle('snap', cfg=cfg, cloud=None, log=self.logger, args=None)
428+
with self.allow_subp([CiTestCase.SUBP_SHELL_TRUE]):
429+
with mock.patch(mock_path, new_callable=StringIO):
430+
handle('snap', cfg=cfg, cloud=None, log=self.logger, args=None)
431+
429432
self.assertEqual('HI\nMOM\n', util.load_file(outfile))
430433

431434
@mock.patch('cloudinit.config.cc_snap.util.subp')

‎cloudinit/config/tests/test_ubuntu_advantage.py‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ def __init__(self, distro):
2323
class TestRunCommands(CiTestCase):
2424

2525
with_logs = True
26+
allowed_subp = [CiTestCase.SUBP_SHELL_TRUE]
2627

2728
def setUp(self):
2829
super(TestRunCommands, self).setUp()
@@ -234,8 +235,10 @@ def test_handle_runs_commands_provided(self, m_install):
234235
'ubuntu-advantage': {'commands': ['echo "HI" >> %s' % outfile,
235236
'echo "MOM" >> %s' % outfile]}}
236237
mock_path = '%s.sys.stderr' % MPATH
237-
with mock.patch(mock_path, new_callable=StringIO):
238-
handle('nomatter', cfg=cfg, cloud=None, log=self.logger, args=None)
238+
with self.allow_subp([CiTestCase.SUBP_SHELL_TRUE]):
239+
with mock.patch(mock_path, new_callable=StringIO):
240+
handle('nomatter', cfg=cfg, cloud=None, log=self.logger,
241+
args=None)
239242
self.assertEqual('HI\nMOM\n', util.load_file(outfile))
240243

241244

‎cloudinit/net/tests/test_init.py‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -199,6 +199,8 @@ def setUp(self):
199199
self.sysdir = self.tmp_dir() + '/'
200200
self.m_sys_path.return_value = self.sysdir
201201
self.addCleanup(sys_mock.stop)
202+
self.add_patch('cloudinit.net.util.is_container', 'm_is_container',
203+
return_value=False)
202204
self.add_patch('cloudinit.net.util.udevadm_settle', 'm_settle')
203205

204206
def test_generate_fallback_finds_connected_eth_with_mac(self):

‎cloudinit/sources/DataSourceAltCloud.py‎

Lines changed: 10 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -181,27 +181,18 @@ def user_data_rhevm(self):
181181

182182
# modprobe floppy
183183
try:
184-
cmd = CMD_PROBE_FLOPPY
185-
(cmd_out, _err) = util.subp(cmd)
186-
LOG.debug('Command: %s\nOutput%s', ' '.join(cmd), cmd_out)
184+
modprobe_floppy()
187185
except ProcessExecutionError as e:
188-
util.logexc(LOG, 'Failed command: %s\n%s', ' '.join(cmd), e)
189-
return False
190-
except OSError as e:
191-
util.logexc(LOG, 'Failed command: %s\n%s', ' '.join(cmd), e)
186+
util.logexc(LOG, 'Failed modprobe: %s', e)
192187
return False
193188

194189
floppy_dev = '/dev/fd0'
195190

196191
# udevadm settle for floppy device
197192
try:
198-
(cmd_out, _err) = util.udevadm_settle(exists=floppy_dev, timeout=5)
199-
LOG.debug('Command: %s\nOutput%s', ' '.join(cmd), cmd_out)
200-
except ProcessExecutionError as e:
201-
util.logexc(LOG, 'Failed command: %s\n%s', ' '.join(cmd), e)
202-
return False
203-
except OSError as e:
204-
util.logexc(LOG, 'Failed command: %s\n%s', ' '.join(cmd), e)
193+
util.udevadm_settle(exists=floppy_dev, timeout=5)
194+
except (ProcessExecutionError, OSError) as e:
195+
util.logexc(LOG, 'Failed udevadm_settle: %s\n', e)
205196
return False
206197

207198
try:
@@ -258,6 +249,11 @@ def user_data_vsphere(self):
258249
return False
259250

260251

252+
def modprobe_floppy():
253+
out, _err = util.subp(CMD_PROBE_FLOPPY)
254+
LOG.debug('Command: %s\nOutput%s', ' '.join(CMD_PROBE_FLOPPY), out)
255+
256+
261257
# Used to match classes to dependencies
262258
# Source DataSourceAltCloud does not really depend on networking.
263259
# In the future 'dsmode' like behavior can be added to offer user

‎cloudinit/sources/DataSourceSmartOS.py‎

Lines changed: 17 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -683,6 +683,18 @@ def jmc_client_factory(
683683
raise ValueError("Unknown value for smartos_type: %s" % smartos_type)
684684

685685

686+
def identify_file(content_f):
687+
cmd = ["file", "--brief", "--mime-type", content_f]
688+
f_type = None
689+
try:
690+
(f_type, _err) = util.subp(cmd)
691+
LOG.debug("script %s mime type is %s", content_f, f_type)
692+
except util.ProcessExecutionError as e:
693+
util.logexc(
694+
LOG, ("Failed to identify script type for %s" % content_f, e))
695+
return None if f_type is None else f_type.strip()
696+
697+
686698
def write_boot_content(content, content_f, link=None, shebang=False,
687699
mode=0o400):
688700
"""
@@ -715,18 +727,11 @@ def write_boot_content(content, content_f, link=None, shebang=False,
715727
util.write_file(content_f, content, mode=mode)
716728

717729
if shebang and not content.startswith("#!"):
718-
try:
719-
cmd = ["file", "--brief", "--mime-type", content_f]
720-
(f_type, _err) = util.subp(cmd)
721-
LOG.debug("script %s mime type is %s", content_f, f_type)
722-
if f_type.strip() == "text/plain":
723-
new_content = "\n".join(["#!/bin/bash", content])
724-
util.write_file(content_f, new_content, mode=mode)
725-
LOG.debug("added shebang to file %s", content_f)
726-
727-
except Exception as e:
728-
util.logexc(LOG, ("Failed to identify script type for %s" %
729-
content_f, e))
730+
f_type = identify_file(content_f)
731+
if f_type == "text/plain":
732+
util.write_file(
733+
content_f, "\n".join(["#!/bin/bash", content]), mode=mode)
734+
LOG.debug("added shebang to file %s", content_f)
730735

731736
if link:
732737
try:

‎cloudinit/tests/helpers.py‎

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,8 @@
3333
from cloudinit.sources import DataSourceNone
3434
from cloudinit import util
3535

36+
_real_subp = util.subp
37+
3638
# Used for skipping tests
3739
SkipTest = unittest2.SkipTest
3840
skipIf = unittest2.skipIf
@@ -143,6 +145,17 @@ class CiTestCase(TestCase):
143145
# Subclass overrides for specific test behavior
144146
# Whether or not a unit test needs logfile setup
145147
with_logs = False
148+
allowed_subp = False
149+
SUBP_SHELL_TRUE = "shell=true"
150+
151+
@contextmanager
152+
def allow_subp(self, allowed_subp):
153+
orig = self.allowed_subp
154+
try:
155+
self.allowed_subp = allowed_subp
156+
yield
157+
finally:
158+
self.allowed_subp = orig
146159

147160
def setUp(self):
148161
super(CiTestCase, self).setUp()
@@ -155,11 +168,41 @@ def setUp(self):
155168
handler.setFormatter(formatter)
156169
self.old_handlers = self.logger.handlers
157170
self.logger.handlers = [handler]
171+
if self.allowed_subp is True:
172+
util.subp = _real_subp
173+
else:
174+
util.subp = self._fake_subp
175+
176+
def _fake_subp(self, *args, **kwargs):
177+
if 'args' in kwargs:
178+
cmd = kwargs['args']
179+
else:
180+
cmd = args[0]
181+
182+
if not isinstance(cmd, six.string_types):
183+
cmd = cmd[0]
184+
pass_through = False
185+
if not isinstance(self.allowed_subp, (list, bool)):
186+
raise TypeError("self.allowed_subp supports list or bool.")
187+
if isinstance(self.allowed_subp, bool):
188+
pass_through = self.allowed_subp
189+
else:
190+
pass_through = (
191+
(cmd in self.allowed_subp) or
192+
(self.SUBP_SHELL_TRUE in self.allowed_subp and
193+
kwargs.get('shell')))
194+
if pass_through:
195+
return _real_subp(*args, **kwargs)
196+
raise Exception(
197+
"called subp. set self.allowed_subp=True to allow\n subp(%s)" %
198+
', '.join([str(repr(a)) for a in args] +
199+
["%s=%s" % (k, repr(v)) for k, v in kwargs.items()]))
158200

159201
def tearDown(self):
160202
if self.with_logs:
161203
# Remove the handler we setup
162204
logging.getLogger().handlers = self.old_handlers
205+
util.subp = _real_subp
163206
super(CiTestCase, self).tearDown()
164207

165208
def tmp_dir(self, dir=None, cleanup=True):

0 commit comments

Comments
 (0)