I really like this branch; some comments inline.
Diff comments:
> diff --git a/cloudinit/analyze/tests/test_dump.py
> b/cloudinit/analyze/tests/test_dump.py
> index f4c4284..7bd834c 100644
> --- a/cloudinit/analyze/tests/test_dump.py
> +++ b/cloudinit/analyze/tests/test_dump.py
> @@ -59,37 +40,22 @@ class TestParseTimestamp(CiTestCase):
> # convert stamp ourselves by adding the missing year value
> year = datetime.now().year
> dt = datetime.strptime(journal_stamp + " " + str(year), journal_fmt)
> - expected = float(dt.strftime('%s.%f'))
> - parsed = parse_timestamp(journal_stamp)
> -
> - # use date(1)
> - out, _ = subp(['date', '+%s.%6N', '-d', journal_stamp])
> - timestamp = out.strip()
> - date_ts = float(timestamp)
> -
> - self.assertEqual(expected, parsed)
> - self.assertEqual(expected, date_ts)
> - self.assertEqual(date_ts, parsed)
> + self.assertEqual(
> + float(dt.strftime('%s.%f')), parse_timestamp(journal_stamp))
>
> + @skipIf(not which("date"), "'date' command not available.")
Hrm, doesn't this mean that a platform could pass make check/tox without the
date command but at runtime it will fail?
> def test_parse_unexpected_timestamp_format_with_date_command(self):
> - """Dump sends unexpected timestamp formats to data for processing."""
> + """Dump sends unexpected timestamp formats to date for processing."""
> new_fmt = '%H:%M %m/%d %Y'
> new_stamp = '17:15 08/08'
> -
> # convert stamp ourselves by adding the missing year value
> year = datetime.now().year
> dt = datetime.strptime(new_stamp + " " + str(year), new_fmt)
> - expected = float(dt.strftime('%s.%f'))
> - parsed = parse_timestamp(new_stamp)
>
> # use date(1)
> - out, _ = subp(['date', '+%s.%6N', '-d', new_stamp])
> - timestamp = out.strip()
> - date_ts = float(timestamp)
> -
> - self.assertEqual(expected, parsed)
> - self.assertEqual(expected, date_ts)
> - self.assertEqual(date_ts, parsed)
> + with self.allow_subp(["date"]):
> + self.assertEqual(
> + float(dt.strftime('%s.%f')), parse_timestamp(new_stamp))
>
>
> class TestParseCILogLine(CiTestCase):
> diff --git a/cloudinit/cmd/tests/test_status.py
> b/cloudinit/cmd/tests/test_status.py
> index 37a8993..aded858 100644
> --- a/cloudinit/cmd/tests/test_status.py
> +++ b/cloudinit/cmd/tests/test_status.py
> @@ -39,7 +39,8 @@ class TestStatus(CiTestCase):
> ensure_file(self.disable_file) # Create the ignored disable file
> (is_disabled, reason) = wrap_and_call(
> 'cloudinit.cmd.status',
> - {'uses_systemd': False},
> + {'uses_systemd': False,
> + 'get_cmdline': "root=/dev/my-root not-important"},
This isn't strictly related to removing use of subp, but a different host leak
(ie, read from /proc) ?
> status._is_cloudinit_disabled, self.disable_file, self.paths)
> self.assertFalse(
> is_disabled, 'expected enabled cloud-init on sysvinit')
> diff --git a/cloudinit/sources/DataSourceAltCloud.py
> b/cloudinit/sources/DataSourceAltCloud.py
> index 24fd65f..8dc63dc 100644
> --- a/cloudinit/sources/DataSourceAltCloud.py
> +++ b/cloudinit/sources/DataSourceAltCloud.py
> @@ -181,27 +181,21 @@ class DataSourceAltCloud(sources.DataSource):
>
> # modprobe floppy
> try:
> - cmd = CMD_PROBE_FLOPPY
> - (cmd_out, _err) = util.subp(cmd)
> - LOG.debug('Command: %s\nOutput%s', ' '.join(cmd), cmd_out)
> + modprobe_floppy()
> except ProcessExecutionError as e:
> - util.logexc(LOG, 'Failed command: %s\n%s', ' '.join(cmd), e)
> + util.logexc(LOG, 'Failed modprobe: %s', e)
> return False
> except OSError as e:
> - util.logexc(LOG, 'Failed command: %s\n%s', ' '.join(cmd), e)
> + util.logexc(LOG, 'No modprobe: %s', e)
> return False
>
> floppy_dev = '/dev/fd0'
>
> # udevadm settle for floppy device
> try:
> - (cmd_out, _err) = util.udevadm_settle(exists=floppy_dev,
> timeout=5)
> - LOG.debug('Command: %s\nOutput%s', ' '.join(cmd), cmd_out)
> - except ProcessExecutionError as e:
> - util.logexc(LOG, 'Failed command: %s\n%s', ' '.join(cmd), e)
> - return False
> - except OSError as e:
> - util.logexc(LOG, 'Failed command: %s\n%s', ' '.join(cmd), e)
Also unrelated to the unittest changes, no?
> + util.udevadm_settle(exists=floppy_dev, timeout=5)
> + except (ProcessExecutionError, OSError) as e:
> + util.logexc(LOG, 'Failed udevadm_settle: %s\n', e)
> return False
>
> try:
> @@ -258,6 +252,11 @@ class DataSourceAltCloud(sources.DataSource):
> return False
>
>
> +def modprobe_floppy():
> + out, _err = util.subp(CMD_PROBE_FLOPPY)
> + LOG.debug('Command: %s\nOutput%s', ' '.join(CMD_PROBE_FLOPPY), out)
> +
> +
This is for ease of mocking, which makes sense. Is the above exception
handling changes removing unused code paths?
> # Used to match classes to dependencies
> # Source DataSourceAltCloud does not really depend on networking.
> # In the future 'dsmode' like behavior can be added to offer user
> diff --git a/tests/unittests/test_datasource/test_smartos.py
> b/tests/unittests/test_datasource/test_smartos.py
> index dca0b3d..860ee7d 100644
> --- a/tests/unittests/test_datasource/test_smartos.py
> +++ b/tests/unittests/test_datasource/test_smartos.py
> @@ -1066,7 +1094,11 @@ class TestSerialConcurrency(TestCase):
> there is only one session over a connection. In contrast, in the
> absence of proper locking multiple processes opening the same serial
> port can corrupt each others' exchanges with the metadata server.
> +
> + This takes on the order of 2 to 3 minutes to run.
We have this disabled by default?
> """
> + allowed_subp = ['mdata-get']
> +
> def setUp(self):
> self.mdata_proc =
> multiprocessing.Process(target=self.start_mdata_loop)
> self.mdata_proc.start()
--
https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+merge/354001
Your team cloud-init commiters is requested to review the proposed merge of
~smoser/cloud-init:fix/fix-unittest-use-of-subp into cloud-init:master.
_______________________________________________
Mailing list: https://launchpad.net/~cloud-init-dev
Post to : cloud-init-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~cloud-init-dev
More help : https://help.launchpad.net/ListHelp
