cloud-init-dev team mailing list archive

[Scott Moser <ssmoser2+ubuntu@xxxxxxxxx>]

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.")

yes.  But what else are we to do here?  We do not want unit tests to depend on this program.

I think really the code should either just drop use of 'date' or at least support all "expected" cases without date, and then at runtime if it came on an unespected date format it could try and error if not available.

that isn't new here. the only thing new is avoiding unit test failure if 'date' is not present.

>      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"},

well, it is a higher level.
status.py:_is_cloudinit_disabled -> get_cmdline -> is_container -> subp(systemd-detect-virt)

mocking get_cmdline() is the right place as mocking '_is_cloudinit_disabled' in a test of '_is_cloudinit_disabled' would be kind of silly.

>              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)

These are probably not strictly needed.  but the change here is just really to not log successful output of udevadm_settle.

I also did drop the 'except OSError' path as it is not necessary any longer.
 http://paste.ubuntu.com/p/dTXVZSZnRk/

and subp has test cases that validate that (test_exception_has_out_err_are_bytes_if_decode_false, test_exception_has_out_err_are_bytes_if_decode_true).

> +            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)
> +
> +

i just simplified the above error handling on calls to modprobe_floppy also.

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

yes. It will 'skipUnless' it has write access to the serial device and is running on smartOS KVM platform.

>      """
> +    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.