Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
22480002:
Use unittest framework in the Automated Installer Testing Framework. (Closed)
|
Created:
7 years, 4 months ago by sukolsak Modified:
7 years, 4 months ago CC:
chromium-reviews Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionUse unittest framework in the Automated Installer Testing Framework.
NOTRY=True
BUG=264859
TEST=
1) Uninstall Chrome.
2) Put mini_installer.exe in the same folder as test_installer.py.
3) Run "python test_installer.py config\config.config".
4) The script will install Chrome and then uninstall Chrome. At each state, it will check that a registry entry for Chrome exists (or doesn't exist). You should see output similar to the following:
"Test: clean -> install chrome -> chrome_installed -> uninstall chrome -> clean ... ok
----------------------------------------------------------------------
Ran 1 test in 12.345s"
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=217059
Patch Set 1 #
Total comments: 2
Patch Set 2 : Subclass failureException to add current state info. #Patch Set 3 : Try to fix 'old chunk mismatch' error in diff. #
Total comments: 9
Patch Set 4 : Change verifiers to classes. #
Total comments: 68
Patch Set 5 : Address mathp's comments. #
Total comments: 14
Patch Set 6 : Address comments of mathpq #
Total comments: 1
Messages
Total messages: 20 (0 generated)
https://codereview.chromium.org/22480002/diff/1/chrome/test/mini_installer/te... File chrome/test/mini_installer/test_installer.py (right): https://codereview.chromium.org/22480002/diff/1/chrome/test/mini_installer/te... chrome/test/mini_installer/test_installer.py:93: error_msg = "In state '%s', %s" % (state, e) It seems like the whole config struct is passed down to the verifier already (the self parameter contains the config) so the information needed to display this message is already available to the verifier code. Given this, do we need to catch the AssertionError here to add the state information?
https://codereview.chromium.org/22480002/diff/1/chrome/test/mini_installer/te... File chrome/test/mini_installer/test_installer.py (right): https://codereview.chromium.org/22480002/diff/1/chrome/test/mini_installer/te... chrome/test/mini_installer/test_installer.py:93: error_msg = "In state '%s', %s" % (state, e) On 2013/08/06 21:42:22, robertshield wrote: > It seems like the whole config struct is passed down to the verifier already > (the self parameter contains the config) so the information needed to display > this message is already available to the verifier code. > > Given this, do we need to catch the AssertionError here to add the state > information? True, but I would like to keep the assertions as simple as possible, e.g. testcase.AssertTrue(..., msg) instead of testcase.AssertTrue(..., testcase.current_state + ', ' + msg). It looks like failureException is exactly what we want. http://docs.python.org/2/library/unittest.html#unittest.TestCase.failureExcep...
lg, comments below. Cheers! Gab https://codereview.chromium.org/22480002/diff/15002/chrome/test/mini_installe... File chrome/test/mini_installer/test_installer.py (left): https://codereview.chromium.org/22480002/diff/15002/chrome/test/mini_installe... chrome/test/mini_installer/test_installer.py:143: # TODO(sukolsak): Quit the test early if VerifyState fails at any point. Bring this TODO in the new test code above. https://codereview.chromium.org/22480002/diff/15002/chrome/test/mini_installe... File chrome/test/mini_installer/test_installer.py (right): https://codereview.chromium.org/22480002/diff/15002/chrome/test/mini_installe... chrome/test/mini_installer/test_installer.py:73: return 'Test: %s' % (' -> '.join(self.test)) Add a method comment with an example of what this prints. https://codereview.chromium.org/22480002/diff/15002/chrome/test/mini_installe... chrome/test/mini_installer/test_installer.py:101: states.""" s/states/state for both 'states' in this sentence, right? https://codereview.chromium.org/22480002/diff/15002/chrome/test/mini_installe... File chrome/test/mini_installer/verifier.py (right): https://codereview.chromium.org/22480002/diff/15002/chrome/test/mini_installe... chrome/test/mini_installer/verifier.py:7: def Verify(property, testcase): Instead of having this method take a testcase (and a bunch of methods in registry_verifier doing so as well), how about making this a Verifier class and all other verifiers classes as well. A Verifier would be owned by each TestCase which would pass it a reference to itself. Each Verifier would own an instance of every verifiers and would also pass down a reference to the TestCase to these at construction time. I prefer that to having to modify a bunch of methods to take a |testcase| parameter. I don't like the cyclic dependency (TestCase -> Verifier; Verifier -> TestCase), but I don't see how to avoid it if we need the TestCase to be able to do fine grain asserts/fails in the verifiers. At least the ownership model is clear (TestCase -> Verifier -> SpecificVerifier) and the interaction model is synchronous so it's not really a dangerous cyclic dependency per se.
https://codereview.chromium.org/22480002/diff/15002/chrome/test/mini_installe... File chrome/test/mini_installer/test_installer.py (left): https://codereview.chromium.org/22480002/diff/15002/chrome/test/mini_installe... chrome/test/mini_installer/test_installer.py:143: # TODO(sukolsak): Quit the test early if VerifyState fails at any point. On 2013/08/07 13:03:34, gab wrote: > Bring this TODO in the new test code above. The assertions in verifiers will kill the test case. https://codereview.chromium.org/22480002/diff/15002/chrome/test/mini_installe... File chrome/test/mini_installer/test_installer.py (right): https://codereview.chromium.org/22480002/diff/15002/chrome/test/mini_installe... chrome/test/mini_installer/test_installer.py:73: return 'Test: %s' % (' -> '.join(self.test)) On 2013/08/07 13:03:34, gab wrote: > Add a method comment with an example of what this prints. Done. https://codereview.chromium.org/22480002/diff/15002/chrome/test/mini_installe... chrome/test/mini_installer/test_installer.py:101: states.""" On 2013/08/07 13:03:34, gab wrote: > s/states/state > > for both 'states' in this sentence, right? Done. https://codereview.chromium.org/22480002/diff/15002/chrome/test/mini_installe... File chrome/test/mini_installer/verifier.py (right): https://codereview.chromium.org/22480002/diff/15002/chrome/test/mini_installe... chrome/test/mini_installer/verifier.py:7: def Verify(property, testcase): On 2013/08/07 13:03:34, gab wrote: > Instead of having this method take a testcase (and a bunch of methods in > registry_verifier doing so as well), how about making this a Verifier class and > all other verifiers classes as well. > > A Verifier would be owned by each TestCase which would pass it a reference to > itself. > > Each Verifier would own an instance of every verifiers and would also pass down > a reference to the TestCase to these at construction time. > I prefer that to having to modify a bunch of methods to take a |testcase| > parameter. > > I don't like the cyclic dependency (TestCase -> Verifier; Verifier -> TestCase), > but I don't see how to avoid it if we need the TestCase to be able to do fine > grain asserts/fails in the verifiers. At least the ownership model is clear > (TestCase -> Verifier -> SpecificVerifier) and the interaction model is > synchronous so it's not really a dangerous cyclic dependency per se. I like to think of verifiers as a collection of static methods that don't have any state, so we don't have to worry about reference cycles and other things. But, like you said, it also comes at a cost of having to pass down |testcase| to every method. So, your suggestion works too.
lgtm w/ consideration of comment below for a follow-up CL. +mathp for readability review (Math, PTAL at my comment below, does the suggested paradigm make sense in python?) https://codereview.chromium.org/22480002/diff/24001/chrome/test/mini_installe... File chrome/test/mini_installer/verifier.py (right): https://codereview.chromium.org/22480002/diff/24001/chrome/test/mini_installe... chrome/test/mini_installer/verifier.py:18: self.registry_verifier = RegistryVerifier(testcase) How about a single dictionary of verifiers, e.g.: self.verifiers['RegistryEntries'] = RegistryVerifier(testcase) Every individual verifier would subclass some AbstractVerifier which would do two things: 1) Hold the testcase. 2) Have a Verify method to be overridden by the individual verifiers. That then makes Verify() below extremely simple (even as we add more verifiers). I'm happy to see this in a follow-up CL to keep this one down to introducing unittest.
https://codereview.chromium.org/22480002/diff/15002/chrome/test/mini_installe... File chrome/test/mini_installer/verifier.py (right): https://codereview.chromium.org/22480002/diff/15002/chrome/test/mini_installe... chrome/test/mini_installer/verifier.py:7: def Verify(property, testcase): On 2013/08/07 21:23:29, sukolsak wrote: > On 2013/08/07 13:03:34, gab wrote: > > Instead of having this method take a testcase (and a bunch of methods in > > registry_verifier doing so as well), how about making this a Verifier class > and > > all other verifiers classes as well. > > > > A Verifier would be owned by each TestCase which would pass it a reference to > > itself. > > > > Each Verifier would own an instance of every verifiers and would also pass > down > > a reference to the TestCase to these at construction time. > > I prefer that to having to modify a bunch of methods to take a |testcase| > > parameter. > > > > I don't like the cyclic dependency (TestCase -> Verifier; Verifier -> > TestCase), > > but I don't see how to avoid it if we need the TestCase to be able to do fine > > grain asserts/fails in the verifiers. At least the ownership model is clear > > (TestCase -> Verifier -> SpecificVerifier) and the interaction model is > > synchronous so it's not really a dangerous cyclic dependency per se. > > I like to think of verifiers as a collection of static methods that don't have > any state, so we don't have to worry about reference cycles and other things. > But, like you said, it also comes at a cost of having to pass down |testcase| to > every method. So, your suggestion works too. And yes, I agree, that's also how I saw them at first, but having to pass testcase around only in methods that need them (and potentially having to change methods signatures if we later want to assert in existing methods that don't take a testcase object is just plain ugly imo). A class is designed to hold this type of data and is a better fit here I think.
Initial comments. https://chromiumcodereview.appspot.com/22480002/diff/24001/chrome/test/mini_i... File chrome/test/mini_installer/registry_verifier.py (right): https://chromiumcodereview.appspot.com/22480002/diff/24001/chrome/test/mini_i... chrome/test/mini_installer/registry_verifier.py:16: self.testcase = testcase self._testcase to show it's private. https://chromiumcodereview.appspot.com/22480002/diff/24001/chrome/test/mini_i... chrome/test/mini_installer/registry_verifier.py:19: """Verifies that the current registry matches the specified criteria.""" can you describe |entries|, since it's a non-trivial dictionary? https://chromiumcodereview.appspot.com/22480002/diff/24001/chrome/test/mini_i... chrome/test/mini_installer/registry_verifier.py:23: def _RootKeyConstant(self, key): one-line docstring please https://chromiumcodereview.appspot.com/22480002/diff/24001/chrome/test/mini_i... chrome/test/mini_installer/registry_verifier.py:34: def _VerifyRegistryEntry(self, key, entry): I think you should rename entry to "expectation" https://chromiumcodereview.appspot.com/22480002/diff/24001/chrome/test/mini_i... chrome/test/mini_installer/registry_verifier.py:35: """Verifies that a registry entry exists or doesn't exist and has 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: ...""" https://chromiumcodereview.appspot.com/22480002/diff/24001/chrome/test/mini_i... chrome/test/mini_installer/registry_verifier.py:41: 'expected' a boolean indicating whether the registry entry exists. *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? https://chromiumcodereview.appspot.com/22480002/diff/24001/chrome/test/mini_i... chrome/test/mini_installer/registry_verifier.py:41: 'expected' a boolean indicating whether the registry entry exists. rename 'expected' to 'should_exist'? https://chromiumcodereview.appspot.com/22480002/diff/24001/chrome/test/mini_i... chrome/test/mini_installer/registry_verifier.py:46: A boolean indicating whether the registry entry matches the criteria. your method doesn't return anything, so you can remove this section. https://chromiumcodereview.appspot.com/22480002/diff/24001/chrome/test/mini_i... chrome/test/mini_installer/registry_verifier.py:48: expected = entry['expected'] remove this line, see below. https://chromiumcodereview.appspot.com/22480002/diff/24001/chrome/test/mini_i... chrome/test/mini_installer/registry_verifier.py:50: try: 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) https://chromiumcodereview.appspot.com/22480002/diff/24001/chrome/test/mini_i... chrome/test/mini_installer/registry_verifier.py:54: self.testcase.assertFalse(expected, 'Registry entry %s is missing' % key) - 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) https://chromiumcodereview.appspot.com/22480002/diff/24001/chrome/test/mini_i... chrome/test/mini_installer/registry_verifier.py:56: self.testcase.assertTrue(expected, 'Registry entry %s exists' % key) put a comment here: # The key exists, see that it matches the expectation # TODO(sukolsak): Verify the expected value. self.testcase.assertTrue(...) https://chromiumcodereview.appspot.com/22480002/diff/24001/chrome/test/mini_i... chrome/test/mini_installer/registry_verifier.py:57: if 'value' in entry: remove this from the CL and only leave the TODO? https://chromiumcodereview.appspot.com/22480002/diff/24001/chrome/test/mini_i... File chrome/test/mini_installer/test_installer.py (right): https://chromiumcodereview.appspot.com/22480002/diff/24001/chrome/test/mini_i... chrome/test/mini_installer/test_installer.py:51: self.verifier = Verifier(self) you are passing the whole object in the Verifier constructor?
https://chromiumcodereview.appspot.com/22480002/diff/24001/chrome/test/mini_i... File chrome/test/mini_installer/test_installer.py (right): https://chromiumcodereview.appspot.com/22480002/diff/24001/chrome/test/mini_i... chrome/test/mini_installer/test_installer.py:51: self.verifier = Verifier(self) On 2013/08/09 16:08:43, Mathieu Perreault wrote: > you are passing the whole object in the Verifier constructor? We would love to avoid this! Is there any way to instead statically assert without requiring the TestCase object to do say testcase.fail()...?
More comments :) https://chromiumcodereview.appspot.com/22480002/diff/24001/chrome/test/mini_i... File chrome/test/mini_installer/registry_verifier.py (right): https://chromiumcodereview.appspot.com/22480002/diff/24001/chrome/test/mini_i... chrome/test/mini_installer/registry_verifier.py:21: self._VerifyRegistryEntry(key, entry) you probably don't need a private method for the impl, you can put the whole implementation here. https://chromiumcodereview.appspot.com/22480002/diff/24001/chrome/test/mini_i... chrome/test/mini_installer/registry_verifier.py:32: self.testcase.fail('Unknown registry key') 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. https://chromiumcodereview.appspot.com/22480002/diff/24001/chrome/test/mini_i... File chrome/test/mini_installer/test_installer.py (right): https://chromiumcodereview.appspot.com/22480002/diff/24001/chrome/test/mini_i... chrome/test/mini_installer/test_installer.py:24: A state is a dictionary where each key is a verifier's name and the Change this to be: Attributes: state: ... actions: ... tests: ... https://chromiumcodereview.appspot.com/22480002/diff/24001/chrome/test/mini_i... chrome/test/mini_installer/test_installer.py:32: self.states = {} Is this used? https://chromiumcodereview.appspot.com/22480002/diff/24001/chrome/test/mini_i... chrome/test/mini_installer/test_installer.py:38: """Tests a test case in the config file.""" The guide says you should describe class attributes if they are public (i.e. not preceded by a _). I think there is no harm in having them private (you're not accessing the attributes from outside the class, right?). https://chromiumcodereview.appspot.com/22480002/diff/24001/chrome/test/mini_i... chrome/test/mini_installer/test_installer.py:48: super(InstallerTest, self).__init__() initialize self._current_state to None. It's best to declare every class attribute that you're going to use. https://chromiumcodereview.appspot.com/22480002/diff/24001/chrome/test/mini_i... chrome/test/mini_installer/test_installer.py:49: self.test = test therefore self._test self._config self._verifier https://chromiumcodereview.appspot.com/22480002/diff/24001/chrome/test/mini_i... chrome/test/mini_installer/test_installer.py:51: self.verifier = Verifier(self) On 2013/08/09 16:16:11, gab wrote: > On 2013/08/09 16:08:43, Mathieu Perreault wrote: > > you are passing the whole object in the Verifier constructor? > > We would love to avoid this! Is there any way to instead statically assert > without requiring the TestCase object to do say testcase.fail()...? How about throwing an exception with a useful message (a VerificationError let's say), that the caller below would intercept and fail() on the message. It would be much preferable to passing the whole testcase to verifier. This works because execution needs to stop when there is one fail (i.e. you're not collecting fails, if that makes sense). https://chromiumcodereview.appspot.com/22480002/diff/24001/chrome/test/mini_i... chrome/test/mini_installer/test_installer.py:55: test = self.test use self._test throughout https://chromiumcodereview.appspot.com/22480002/diff/24001/chrome/test/mini_i... chrome/test/mini_installer/test_installer.py:59: self.assertEqual(len(test) % 2, 1, 'The length of test array must be odd') -expectation should be 1st param self.assertEqual(1, len(self._test) % 2, '...') https://chromiumcodereview.appspot.com/22480002/diff/24001/chrome/test/mini_i... chrome/test/mini_installer/test_installer.py:66: for i in range(1, len(test), 2): Add a comment saying exactly what you're looping over by jumping by 2 at every iteration (is it states, actions?). Example: # Starting at index 1, we loop through pairs of (..., ...). https://chromiumcodereview.appspot.com/22480002/diff/24001/chrome/test/mini_i... chrome/test/mini_installer/test_installer.py:73: def __str__(self): nit: bring it up to be next to __init__? https://chromiumcodereview.appspot.com/22480002/diff/24001/chrome/test/mini_i... chrome/test/mini_installer/test_installer.py:76: This method returns a string created by joining state names and action names "This method returns" -> Returns: A string created... https://chromiumcodereview.appspot.com/22480002/diff/24001/chrome/test/mini_i... chrome/test/mini_installer/test_installer.py:84: Add a line saying: Overriden from unittest.TestCase. also in failureException https://chromiumcodereview.appspot.com/22480002/diff/24001/chrome/test/mini_i... chrome/test/mini_installer/test_installer.py:107: """Verifies that the current machine state matches the expected machine Make it fit on one line https://chromiumcodereview.appspot.com/22480002/diff/24001/chrome/test/mini_i... chrome/test/mini_installer/test_installer.py:115: # TODO(sukolsak): Need to figure how exactly we want to reset. nit: put this TODO in place of _RunResetCommand, above and remove this function. Once you add it, you'll have a better idea of what it does and be able to provide good doc for it. https://chromiumcodereview.appspot.com/22480002/diff/24001/chrome/test/mini_i... chrome/test/mini_installer/test_installer.py:185: def Test(config): rename to RunTests? Having test everywhere makes my head spin :)
Ah ok, I like that, so Verifiers can just raise exceptions and the main TestCase can then provide testcase failures if the underlying verifiers raised exceptions? That's nicer as it allows us to keep the verifiers as before, simple free functions (i.e., no Verifier class).
https://chromiumcodereview.appspot.com/22480002/diff/24001/chrome/test/mini_i... File chrome/test/mini_installer/registry_verifier.py (right): https://chromiumcodereview.appspot.com/22480002/diff/24001/chrome/test/mini_i... chrome/test/mini_installer/registry_verifier.py:7: class RegistryVerifier: probably want to use new-style objects https://chromiumcodereview.appspot.com/22480002/diff/24001/chrome/test/mini_i... File chrome/test/mini_installer/verifier.py (right): https://chromiumcodereview.appspot.com/22480002/diff/24001/chrome/test/mini_i... chrome/test/mini_installer/verifier.py:18: self.registry_verifier = RegistryVerifier(testcase) On 2013/08/08 02:07:08, gab wrote: > How about a single dictionary of verifiers, e.g.: > > self.verifiers['RegistryEntries'] = RegistryVerifier(testcase) > > Every individual verifier would subclass some AbstractVerifier which would do > two things: > > 1) Hold the testcase. > 2) Have a Verify method to be overridden by the individual verifiers. > > That then makes Verify() below extremely simple (even as we add more verifiers). > > I'm happy to see this in a follow-up CL to keep this one down to introducing > unittest. I agree with the notion of having Verifiers derive from an interface of some kind. IIRC Python either didn't have or had really ugly built in ways of specifying abstract interfaces, but something like class Verifier(object): def Verify(self): raise NotImplementedError("Complain") class RegistryVerifier(Verifier): def Verify(self): # Do stuff accomplishes the same. https://chromiumcodereview.appspot.com/22480002/diff/24001/chrome/test/mini_i... chrome/test/mini_installer/verifier.py:20: def Verify(self, property): If the comment above is followed, this would then NOT be a method of Verifier, but would instead be a standalone method that takes a given machine _state_, conjures up the needed verifiers and runs them to check whether the state is good.
https://chromiumcodereview.appspot.com/22480002/diff/24001/chrome/test/mini_i... File chrome/test/mini_installer/registry_verifier.py (right): https://chromiumcodereview.appspot.com/22480002/diff/24001/chrome/test/mini_i... chrome/test/mini_installer/registry_verifier.py:16: self.testcase = testcase On 2013/08/09 16:08:43, Mathieu Perreault wrote: > self._testcase to show it's private. I have removed testcase. https://chromiumcodereview.appspot.com/22480002/diff/24001/chrome/test/mini_i... chrome/test/mini_installer/registry_verifier.py:19: """Verifies that the current registry matches the specified criteria.""" On 2013/08/09 16:08:43, Mathieu Perreault wrote: > can you describe |entries|, since it's a non-trivial dictionary? Done. https://chromiumcodereview.appspot.com/22480002/diff/24001/chrome/test/mini_i... chrome/test/mini_installer/registry_verifier.py:21: self._VerifyRegistryEntry(key, entry) On 2013/08/09 17:25:15, Mathieu Perreault wrote: > you probably don't need a private method for the impl, you can put the whole > implementation here. I think it's much cleaner to put it in a separate method. https://chromiumcodereview.appspot.com/22480002/diff/24001/chrome/test/mini_i... chrome/test/mini_installer/registry_verifier.py:23: def _RootKeyConstant(self, key): On 2013/08/09 16:08:43, Mathieu Perreault wrote: > one-line docstring please Done. https://chromiumcodereview.appspot.com/22480002/diff/24001/chrome/test/mini_i... chrome/test/mini_installer/registry_verifier.py:32: self.testcase.fail('Unknown registry key') On 2013/08/09 17:25:15, Mathieu Perreault wrote: > 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. 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. https://chromiumcodereview.appspot.com/22480002/diff/24001/chrome/test/mini_i... chrome/test/mini_installer/registry_verifier.py:34: def _VerifyRegistryEntry(self, key, entry): On 2013/08/09 16:08:43, Mathieu Perreault wrote: > I think you should rename entry to "expectation" Done. https://chromiumcodereview.appspot.com/22480002/diff/24001/chrome/test/mini_i... chrome/test/mini_installer/registry_verifier.py:35: """Verifies that a registry entry exists or doesn't exist and has On 2013/08/09 16:08:43, Mathieu Perreault wrote: > 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: > ...""" Done. https://chromiumcodereview.appspot.com/22480002/diff/24001/chrome/test/mini_i... chrome/test/mini_installer/registry_verifier.py:41: 'expected' a boolean indicating whether the registry entry exists. On 2013/08/09 16:08:43, Mathieu Perreault wrote: > *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? Done. Yes. https://chromiumcodereview.appspot.com/22480002/diff/24001/chrome/test/mini_i... chrome/test/mini_installer/registry_verifier.py:41: 'expected' a boolean indicating whether the registry entry exists. On 2013/08/09 16:08:43, Mathieu Perreault wrote: > rename 'expected' to 'should_exist'? 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. https://chromiumcodereview.appspot.com/22480002/diff/24001/chrome/test/mini_i... chrome/test/mini_installer/registry_verifier.py:46: A boolean indicating whether the registry entry matches the criteria. On 2013/08/09 16:08:43, Mathieu Perreault wrote: > your method doesn't return anything, so you can remove this section. Done. https://chromiumcodereview.appspot.com/22480002/diff/24001/chrome/test/mini_i... chrome/test/mini_installer/registry_verifier.py:48: expected = entry['expected'] On 2013/08/09 16:08:43, Mathieu Perreault wrote: > remove this line, see below. Done. https://chromiumcodereview.appspot.com/22480002/diff/24001/chrome/test/mini_i... chrome/test/mini_installer/registry_verifier.py:50: try: On 2013/08/09 16:08:43, Mathieu Perreault wrote: > 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) Done. https://chromiumcodereview.appspot.com/22480002/diff/24001/chrome/test/mini_i... chrome/test/mini_installer/registry_verifier.py:54: self.testcase.assertFalse(expected, 'Registry entry %s is missing' % key) On 2013/08/09 16:08:43, Mathieu Perreault wrote: > - 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) Done. https://chromiumcodereview.appspot.com/22480002/diff/24001/chrome/test/mini_i... chrome/test/mini_installer/registry_verifier.py:56: self.testcase.assertTrue(expected, 'Registry entry %s exists' % key) On 2013/08/09 16:08:43, Mathieu Perreault wrote: > put a comment here: > > # The key exists, see that it matches the expectation > # TODO(sukolsak): Verify the expected value. > self.testcase.assertTrue(...) Done. https://chromiumcodereview.appspot.com/22480002/diff/24001/chrome/test/mini_i... chrome/test/mini_installer/registry_verifier.py:57: if 'value' in entry: On 2013/08/09 16:08:43, Mathieu Perreault wrote: > remove this from the CL and only leave the TODO? Done. https://chromiumcodereview.appspot.com/22480002/diff/24001/chrome/test/mini_i... File chrome/test/mini_installer/test_installer.py (right): https://chromiumcodereview.appspot.com/22480002/diff/24001/chrome/test/mini_i... chrome/test/mini_installer/test_installer.py:24: A state is a dictionary where each key is a verifier's name and the On 2013/08/09 17:25:15, Mathieu Perreault wrote: > Change this to be: > > Attributes: > state: ... > actions: ... > tests: ... Done. https://chromiumcodereview.appspot.com/22480002/diff/24001/chrome/test/mini_i... chrome/test/mini_installer/test_installer.py:32: self.states = {} On 2013/08/09 17:25:15, Mathieu Perreault wrote: > Is this used? It is used. https://chromiumcodereview.appspot.com/22480002/diff/24001/chrome/test/mini_i... chrome/test/mini_installer/test_installer.py:38: """Tests a test case in the config file.""" On 2013/08/09 17:25:15, Mathieu Perreault wrote: > The guide says you should describe class attributes if they are public (i.e. not > preceded by a _). I think there is no harm in having them private (you're not > accessing the attributes from outside the class, right?). Done. https://chromiumcodereview.appspot.com/22480002/diff/24001/chrome/test/mini_i... chrome/test/mini_installer/test_installer.py:48: super(InstallerTest, self).__init__() On 2013/08/09 17:25:15, Mathieu Perreault wrote: > initialize self._current_state to None. It's best to declare every class > attribute that you're going to use. Done. https://chromiumcodereview.appspot.com/22480002/diff/24001/chrome/test/mini_i... chrome/test/mini_installer/test_installer.py:49: self.test = test On 2013/08/09 17:25:15, Mathieu Perreault wrote: > therefore > > self._test > self._config > self._verifier Done. https://chromiumcodereview.appspot.com/22480002/diff/24001/chrome/test/mini_i... chrome/test/mini_installer/test_installer.py:55: test = self.test On 2013/08/09 17:25:15, Mathieu Perreault wrote: > use self._test throughout Done. https://chromiumcodereview.appspot.com/22480002/diff/24001/chrome/test/mini_i... chrome/test/mini_installer/test_installer.py:59: self.assertEqual(len(test) % 2, 1, 'The length of test array must be odd') On 2013/08/09 17:25:15, Mathieu Perreault wrote: > -expectation should be 1st param > > self.assertEqual(1, len(self._test) % 2, '...') Done. https://chromiumcodereview.appspot.com/22480002/diff/24001/chrome/test/mini_i... chrome/test/mini_installer/test_installer.py:66: for i in range(1, len(test), 2): On 2013/08/09 17:25:15, Mathieu Perreault wrote: > Add a comment saying exactly what you're looping over by jumping by 2 at every > iteration (is it states, actions?). > > Example: > # Starting at index 1, we loop through pairs of (..., ...). Done. https://chromiumcodereview.appspot.com/22480002/diff/24001/chrome/test/mini_i... chrome/test/mini_installer/test_installer.py:73: def __str__(self): On 2013/08/09 17:25:15, Mathieu Perreault wrote: > nit: bring it up to be next to __init__? Done. https://chromiumcodereview.appspot.com/22480002/diff/24001/chrome/test/mini_i... chrome/test/mini_installer/test_installer.py:76: This method returns a string created by joining state names and action names On 2013/08/09 17:25:15, Mathieu Perreault wrote: > "This method returns" -> > > Returns: > A string created... Done. https://chromiumcodereview.appspot.com/22480002/diff/24001/chrome/test/mini_i... chrome/test/mini_installer/test_installer.py:84: On 2013/08/09 17:25:15, Mathieu Perreault wrote: > Add a line saying: > > Overriden from unittest.TestCase. > > also in failureException Done. https://chromiumcodereview.appspot.com/22480002/diff/24001/chrome/test/mini_i... chrome/test/mini_installer/test_installer.py:107: """Verifies that the current machine state matches the expected machine On 2013/08/09 17:25:15, Mathieu Perreault wrote: > Make it fit on one line Done. https://chromiumcodereview.appspot.com/22480002/diff/24001/chrome/test/mini_i... chrome/test/mini_installer/test_installer.py:115: # TODO(sukolsak): Need to figure how exactly we want to reset. On 2013/08/09 17:25:15, Mathieu Perreault wrote: > nit: put this TODO in place of _RunResetCommand, above and remove this function. > Once you add it, you'll have a better idea of what it does and be able to > provide good doc for it. Done. https://chromiumcodereview.appspot.com/22480002/diff/24001/chrome/test/mini_i... chrome/test/mini_installer/test_installer.py:185: def Test(config): On 2013/08/09 17:25:15, Mathieu Perreault wrote: > rename to RunTests? Having test everywhere makes my head spin :) Done. https://chromiumcodereview.appspot.com/22480002/diff/24001/chrome/test/mini_i... File chrome/test/mini_installer/verifier.py (right): https://chromiumcodereview.appspot.com/22480002/diff/24001/chrome/test/mini_i... chrome/test/mini_installer/verifier.py:20: def Verify(self, property): On 2013/08/09 19:00:15, robertshield wrote: > If the comment above is followed, this would then NOT be a method of Verifier, > but would instead be a standalone method that takes a given machine _state_, > conjures up the needed verifiers and runs them to check whether the state is > good. Done.
Thank you for the comments. https://chromiumcodereview.appspot.com/22480002/diff/24001/chrome/test/mini_i... File chrome/test/mini_installer/test_installer.py (right): https://chromiumcodereview.appspot.com/22480002/diff/24001/chrome/test/mini_i... chrome/test/mini_installer/test_installer.py:51: self.verifier = Verifier(self) On 2013/08/09 17:25:15, Mathieu Perreault wrote: > On 2013/08/09 16:16:11, gab wrote: > > On 2013/08/09 16:08:43, Mathieu Perreault wrote: > > > you are passing the whole object in the Verifier constructor? > > > > We would love to avoid this! Is there any way to instead statically assert > > without requiring the TestCase object to do say testcase.fail()...? > > How about throwing an exception with a useful message (a VerificationError let's > say), that the caller below would intercept and fail() on the message. It would > be much preferable to passing the whole testcase to verifier. This works because > execution needs to stop when there is one fail (i.e. you're not collecting > fails, if that makes sense). I have removed |testcase| from verifier (and thus no need to make it a class) and made it throw an exception when an error occurs. I'm not sure if this addresses your comment. Could you take a look?
lgtm w/ comment below, love the new framework based on AssertionErrors instead of having to pass |testcase| around :). Cheers! Gab https://codereview.chromium.org/22480002/diff/37001/chrome/test/mini_installe... File chrome/test/mini_installer/config/chrome_installed.prop (right): https://codereview.chromium.org/22480002/diff/37001/chrome/test/mini_installe... chrome/test/mini_installer/config/chrome_installed.prop:4: {"exist": true} I prefer "exists" (as you suggested in reply to another comment I think). https://codereview.chromium.org/22480002/diff/37001/chrome/test/mini_installe... File chrome/test/mini_installer/registry_verifier.py (right): https://codereview.chromium.org/22480002/diff/37001/chrome/test/mini_installe... chrome/test/mini_installer/registry_verifier.py:36: (under 'exist') and optionally specifies an expected 'value' for the key. 'exists' https://codereview.chromium.org/22480002/diff/37001/chrome/test/mini_installe... chrome/test/mini_installer/registry_verifier.py:41: 'exist' a boolean indicating whether the registry entry should exist. 'exists'
lgtm with the few nits below. Thanks for your patience! https://chromiumcodereview.appspot.com/22480002/diff/37001/chrome/test/mini_i... File chrome/test/mini_installer/registry_verifier.py (right): https://chromiumcodereview.appspot.com/22480002/diff/37001/chrome/test/mini_i... chrome/test/mini_installer/registry_verifier.py:43: the key. nit: indent 4 more https://chromiumcodereview.appspot.com/22480002/diff/37001/chrome/test/mini_i... chrome/test/mini_installer/registry_verifier.py:47: # Query the Windows registry for the registry key. It will throw pack comment lines. Sorry if I didn't do it when I gave an example comment. https://chromiumcodereview.appspot.com/22480002/diff/37001/chrome/test/mini_i... File chrome/test/mini_installer/verifier.py (right): https://chromiumcodereview.appspot.com/22480002/diff/37001/chrome/test/mini_i... chrome/test/mini_installer/verifier.py:12: property: A property dictionary. can you describe what is a value in this dictionary? https://chromiumcodereview.appspot.com/22480002/diff/37001/chrome/test/mini_i... chrome/test/mini_installer/verifier.py:14: for verifier_name, value in property.iteritems(): perhaps rename value -> entries to be more consistent with the VerifyRegistryEntries signature.
Thanks for your reviews. Sorry I messed up the title of the latest CL. I meant to write "Address mathp, gab, and robertshield's comment". https://codereview.chromium.org/22480002/diff/24001/chrome/test/mini_installe... File chrome/test/mini_installer/registry_verifier.py (right): https://codereview.chromium.org/22480002/diff/24001/chrome/test/mini_installe... chrome/test/mini_installer/registry_verifier.py:7: class RegistryVerifier: On 2013/08/09 19:00:15, robertshield wrote: > probably want to use new-style objects We are using stand-alone methods now. https://codereview.chromium.org/22480002/diff/37001/chrome/test/mini_installe... File chrome/test/mini_installer/config/chrome_installed.prop (right): https://codereview.chromium.org/22480002/diff/37001/chrome/test/mini_installe... chrome/test/mini_installer/config/chrome_installed.prop:4: {"exist": true} On 2013/08/10 01:59:27, gab wrote: > I prefer "exists" (as you suggested in reply to another comment I think). Done. https://codereview.chromium.org/22480002/diff/37001/chrome/test/mini_installe... File chrome/test/mini_installer/registry_verifier.py (right): https://codereview.chromium.org/22480002/diff/37001/chrome/test/mini_installe... chrome/test/mini_installer/registry_verifier.py:36: (under 'exist') and optionally specifies an expected 'value' for the key. On 2013/08/10 01:59:27, gab wrote: > 'exists' Done. https://codereview.chromium.org/22480002/diff/37001/chrome/test/mini_installe... chrome/test/mini_installer/registry_verifier.py:41: 'exist' a boolean indicating whether the registry entry should exist. On 2013/08/10 01:59:27, gab wrote: > 'exists' Done. https://codereview.chromium.org/22480002/diff/37001/chrome/test/mini_installe... chrome/test/mini_installer/registry_verifier.py:43: the key. On 2013/08/12 17:37:26, Mathieu Perreault wrote: > nit: indent 4 more Done. https://codereview.chromium.org/22480002/diff/37001/chrome/test/mini_installe... chrome/test/mini_installer/registry_verifier.py:47: # Query the Windows registry for the registry key. It will throw On 2013/08/12 17:37:26, Mathieu Perreault wrote: > pack comment lines. Sorry if I didn't do it when I gave an example comment. Done. https://codereview.chromium.org/22480002/diff/37001/chrome/test/mini_installe... File chrome/test/mini_installer/verifier.py (right): https://codereview.chromium.org/22480002/diff/37001/chrome/test/mini_installe... chrome/test/mini_installer/verifier.py:12: property: A property dictionary. On 2013/08/12 17:37:26, Mathieu Perreault wrote: > can you describe what is a value in this dictionary? Done. https://codereview.chromium.org/22480002/diff/37001/chrome/test/mini_installe... chrome/test/mini_installer/verifier.py:14: for verifier_name, value in property.iteritems(): On 2013/08/12 17:37:26, Mathieu Perreault wrote: > perhaps rename value -> entries to be more consistent with the > VerifyRegistryEntries signature. Other verifiers that I am going to add will have different signatures.
lgtm https://codereview.chromium.org/22480002/diff/50001/chrome/test/mini_installe... File chrome/test/mini_installer/registry_verifier.py (right): https://codereview.chromium.org/22480002/diff/50001/chrome/test/mini_installe... chrome/test/mini_installer/registry_verifier.py:56: # TODO(sukolsak): Verify the expected value. Don't forget to do this in the next CL :-)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sukolsak@chromium.org/22480002/50001
Message was sent while issue was closed.
Change committed as 217059 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
