Use unittest framework in the Automated Installer Testing Framework.

chrome/test/mini_installer/registry_verifier.py

 # Copyright 2013 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 import _winreg
 
-import settings
+class RegistryVerifier:

[robertshield, 2013/08/09 19:00:15]

probably want to use new-style objects

[sukolsak, 2013/08/12 19:20:20]

We are using stand-alone methods now.

+  """Verifies that the current registry matches the specified criteria."""
 
+  def __init__(self, testcase):
+    """Constructor.
 
-def VerifyRegistryEntries(entries):
-  """Verifies that the current registry matches the specified criteria."""
-  for key, entry in entries.iteritems():
-    # TODO(sukolsak): Use unittest framework instead of prints.
-    if VerifyRegistryEntry(key, entry):
-      print 'Passed'
-    else:
-      print 'Failed'
+    Args:
+      testcase: A unittest.TestCase instance.
+    """
+    self.testcase = testcase

[Mathieu, 2013/08/09 16:08:43]

self._testcase to show it's private.

[sukolsak, 2013/08/09 21:12:38]

I have removed testcase.

 
+  def VerifyRegistryEntries(self, entries):
+    """Verifies that the current registry matches the specified criteria."""

[Mathieu, 2013/08/09 16:08:43]

can you describe |entries|, since it's a non-trivial dictionary?

[sukolsak, 2013/08/09 21:12:38]

Done.

+    for key, entry in entries.iteritems():
+      self._VerifyRegistryEntry(key, entry)

[Mathieu, 2013/08/09 17:25:15]

you probably don't need a private method for the impl, you can put the whole
implementation here.

[sukolsak, 2013/08/09 21:12:38]

I think it's much cleaner to put it in a separate method. 

 
-def RootKeyConstant(key):
-  if key == 'HKEY_CLASSES_ROOT':
-    return _winreg.HKEY_CLASSES_ROOT
-  if key == 'HKEY_CURRENT_USER':
-    return _winreg.HKEY_CURRENT_USER
-  if key == 'HKEY_LOCAL_MACHINE':
-    return _winreg.HKEY_LOCAL_MACHINE
-  if key == 'HKEY_USERS':
-    return _winreg.HKEY_USERS
-  # TODO(sukolsak): Use unittest framework instead of exceptions.
-  raise Exception('Unknown registry key')
+  def _RootKeyConstant(self, key):

[Mathieu, 2013/08/09 16:08:43]

one-line docstring please

[sukolsak, 2013/08/09 21:12:38]

Done.

+    if key == 'HKEY_CLASSES_ROOT':
+      return _winreg.HKEY_CLASSES_ROOT
+    if key == 'HKEY_CURRENT_USER':
+      return _winreg.HKEY_CURRENT_USER
+    if key == 'HKEY_LOCAL_MACHINE':
+      return _winreg.HKEY_LOCAL_MACHINE
+    if key == 'HKEY_USERS':
+      return _winreg.HKEY_USERS
+    self.testcase.fail('Unknown registry key')

[Mathieu, 2013/08/09 17:25:15]

Have this return an error instead (try throwing KeyError), and have a different
except block in VerifyRegistryEntries that will catch it and make the testcase
fail.

[sukolsak, 2013/08/09 21:12:38]

Changed to KeyError. I don't think we have to create an except block to catch it
because the unittest framework will catch it and fail the testcase anyway.

 
+  def _VerifyRegistryEntry(self, key, entry):

[Mathieu, 2013/08/09 16:08:43]

I think you should rename entry to "expectation"

[sukolsak, 2013/08/09 21:12:38]

Done.

+    """Verifies that a registry entry exists or doesn't exist and has

[Mathieu, 2013/08/09 16:08:43]

Has to be one line.

"""Verifies a registry key according to the |expectation|.

The |expectation| specifies whether or not the registry key should exist (under
'should_exist') and optionally specifies an expected 'value' for the key.

Args:
..."""

[sukolsak, 2013/08/09 21:12:38]

Done.

+    the specified value.
 
-def VerifyRegistryEntry(key, entry):
-  """Verifies that a registry entry exists or doesn't exist and has
-  the specified value.
+    Args:
+      key: Name of the registry key.
+      entry: A dictionary with the following keys and values:
+          'expected' a boolean indicating whether the registry entry exists.

[Mathieu, 2013/08/09 16:08:43]

*registry entry should exist.

Unless I'm not understanding correct, 'expected' is whether the entry should
exist, right, not whether or not it actually exists?

[Mathieu, 2013/08/09 16:08:43]

rename 'expected' to 'should_exist'?

[sukolsak, 2013/08/09 21:12:38]

Done. Yes.

[sukolsak, 2013/08/09 21:12:38]

I have renamed it to 'exist' (or should it be 'exists'?) because it being in the
'expectation' dictionary should already imply that it's just an expectation.

+          'value' (optional) a string representing the value of the registry
+          entry.
 
-  Args:
-    key: Name of the registry key.
-    entry: A dictionary with the following keys and values:
-      'expected' a boolean indicating whether the registry entry exists.
-      'value' (optional) a string representing the value of the registry entry.
-
-  Returns:
-    A boolean indicating whether the registry entry matches the criteria.
-  """
-  expected = entry['expected']
-  # TODO(sukolsak): Debug prints to be removed later.
-  print settings.PRINT_VERIFIER_PREFIX + key,
-  if expected:
-    print 'exists...',
-  else:
-    print "doesn't exist...",
-  root_key, sub_key = key.split('\\', 1)
-  try:
-    reg_key = _winreg.OpenKey(RootKeyConstant(root_key),
-                              sub_key, 0, _winreg.KEY_READ)
-  except WindowsError:
-    return not expected
-  if not expected:
-    return False
-  if 'value' in entry:
-    # TODO(sukolsak): implement value
-    pass
-  return True
+    Returns:
+      A boolean indicating whether the registry entry matches the criteria.

[Mathieu, 2013/08/09 16:08:43]

your method doesn't return anything, so you can remove this section.

[sukolsak, 2013/08/09 21:12:38]

Done.

+    """
+    expected = entry['expected']

[Mathieu, 2013/08/09 16:08:43]

remove this line, see below.

[sukolsak, 2013/08/09 21:12:38]

Done.

+    root_key, sub_key = key.split('\\', 1)
+    try:

[Mathieu, 2013/08/09 16:08:43]

Add a comment inside the try block to describe what's going on.

Also, if you're not using the returned value for now, use _.

try:
  # Query the Windows registry for the registry key. Will throw
  # a WindowsError if the key doesn't exist.
  _ = _winreg.OpenKey(self._RootKeyConstant(root_key), sub_key, 0,
                      _winreg.KEY_READ)

[sukolsak, 2013/08/09 21:12:38]

Done.

+      reg_key = _winreg.OpenKey(self._RootKeyConstant(root_key), sub_key, 0,
+                                _winreg.KEY_READ)
+    except WindowsError:
+      self.testcase.assertFalse(expected, 'Registry entry %s is missing' % key)

[Mathieu, 2013/08/09 16:08:43]

- Add a comment.
- keep expectation['should_exist']. It reads better and reader doesn't have to
go back to the top of the method.

except WindowsError:
  # Key doesn't exist. See that it matches the expectation.
  self.testcase.assertFalse(expectation['should_exist'], 'Registry entry %s is
missing' % key)

[sukolsak, 2013/08/09 21:12:38]

Done.

+      return
+    self.testcase.assertTrue(expected, 'Registry entry %s exists' % key)

[Mathieu, 2013/08/09 16:08:43]

put a comment here:

# The key exists, see that it matches the expectation
# TODO(sukolsak): Verify the expected value.
self.testcase.assertTrue(...)

[sukolsak, 2013/08/09 21:12:38]

Done.

+    if 'value' in entry:

[Mathieu, 2013/08/09 16:08:43]

remove this from the CL and only leave the TODO?

[sukolsak, 2013/08/09 21:12:38]

Done.

+      # TODO(sukolsak): implement value
+      pass