Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
20578004:
Initial commit for the Automated Installer Testing Framework. (Closed)
|
Created:
7 years, 4 months ago by sukolsak Modified:
7 years, 4 months ago CC:
chromium-reviews, grt+watch_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionInitial commit for the Automated Installer Testing Framework. The design documentation is available at https://docs.google.com/a/google.com/document/d/1iK0INUnaTbeEMXHLWpyRch2eyp-Uxb7ckppO7X-onLY/
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. In other words, it will go from clean -> installed -> clean. At each state, it will check that a registry entry for Chrome exists (or doesn't exist). You should see "Passed" at each state.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=215940
Patch Set 1 #
Total comments: 38
Patch Set 2 : Fix format. #Patch Set 3 : Add data. #
Total comments: 9
Patch Set 4 : #
Total comments: 53
Patch Set 5 : #
Total comments: 6
Patch Set 6 : #
Total comments: 14
Patch Set 7 : #Patch Set 8 : #
Total comments: 6
Patch Set 9 : #
Total comments: 29
Patch Set 10 : #
Total comments: 2
Patch Set 11 : #Patch Set 12 : #Patch Set 13 : Use argparse instead of optparse. #
Total comments: 4
Patch Set 14 : #Patch Set 15 : Upload with NOTRY=True. #Messages
Total messages: 42 (0 generated)
Here is my first submission. Please take a look.
https://codereview.chromium.org/20578004/diff/1/chrome/installer/automation_t... File chrome/installer/automation_test/chrome_installed.prop (right): https://codereview.chromium.org/20578004/diff/1/chrome/installer/automation_t... chrome/installer/automation_test/chrome_installed.prop:1: { Suggest adding these files under a sub directory. automation_test/data or something similar https://codereview.chromium.org/20578004/diff/1/chrome/installer/automation_t... chrome/installer/automation_test/chrome_installed.prop:2: "RegistryEntries": { not sure, but it look like these might be tab characters? tabs are verboten in Chromium code and indents are usually two spaces. See http://www.chromium.org/developers/coding-style and specifically, http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Spaces... https://codereview.chromium.org/20578004/diff/1/chrome/installer/automation_t... File chrome/installer/automation_test/config.config (right): https://codereview.chromium.org/20578004/diff/1/chrome/installer/automation_t... chrome/installer/automation_test/config.config:5: ["chromeframe_installed", ["chrome_not_installed.prop", "chromeframe_installed.prop"]] is it possible to add a line feed after the second comma on this line, then reindent to make this more readable and fit in 80 chars, e.g.: "states": [ ["clean", ["chrome_not_installed.prop", "chromeframe_not_installed.prop"]], ["chrome_installed", ["chrome_installed.prop", "chromeframe_not_installed.prop"]], ["chromeframe_installed", ["chrome_not_installed.prop", "chromeframe_installed.prop"]] ... https://codereview.chromium.org/20578004/diff/1/chrome/installer/automation_t... File chrome/installer/automation_test/test_installer.py (right): https://codereview.chromium.org/20578004/diff/1/chrome/installer/automation_t... chrome/installer/automation_test/test_installer.py:22: assert(isinstance(current_property[key], dict) and \ \ not needed here https://codereview.chromium.org/20578004/diff/1/chrome/installer/automation_t... chrome/installer/automation_test/test_installer.py:25: dict(current_property[key].items() + value.items()) hanging indents (that continue a statement) are usually four spaces it would also be ok to write current_property[key] = dict( current_property[key.items() + value.items()) https://codereview.chromium.org/20578004/diff/1/chrome/installer/automation_t... chrome/installer/automation_test/test_installer.py:33: I believe it is two blank lines between top-level function definitions. https://codereview.chromium.org/20578004/diff/1/chrome/installer/automation_t... chrome/installer/automation_test/test_installer.py:69: + " & mini_installer.exe --chrome-frame --uninstall", shell=True) # FIXME indent 2 more https://codereview.chromium.org/20578004/diff/1/chrome/installer/automation_t... File chrome/installer/automation_test/verifier.py (right): https://codereview.chromium.org/20578004/diff/1/chrome/installer/automation_t... chrome/installer/automation_test/verifier.py:12: # TODO: implement other verifiers TODOs should be attributed: # TODO(sukolsak): implement...
Python is so sick! I still can't believe that it does all of this in barely 100 lines :). Please link to design doc and add a TEST= section specifying what should work as of this CL and how you verified that locally (i.e., how someone else could try this and confirm it works). (Reviewed on phone, apologies if some things don't make sense, this reviewing tool is way broken on mobile!) Cheers! Gab https://codereview.chromium.org/20578004/diff/1/chrome/installer/automation_t... File chrome/installer/automation_test/chrome_installed.prop (right): https://codereview.chromium.org/20578004/diff/1/chrome/installer/automation_t... chrome/installer/automation_test/chrome_installed.prop:1: { On 2013/07/26 19:32:23, robertshield wrote: > Suggest adding these files under a sub directory. automation_test/data or > something similar Right, I suggest a "config" directory instead of "data" though. I also think this all should go in chrome/installer/mini_installer/test instead of automation_test (all tests are automated ;)!) https://codereview.chromium.org/20578004/diff/1/chrome/installer/automation_t... chrome/installer/automation_test/chrome_installed.prop:2: "RegistryEntries": { On 2013/07/26 19:32:23, robertshield wrote: > not sure, but it look like these might be tab characters? tabs are verboten in > Chromium code and indents are usually two spaces. > > See http://www.chromium.org/developers/coding-style and specifically, > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Spaces... Presubmit scripts (also ran when uploading) should catch tabs if any. I know because this happened to me when my editor's config broke earlier today! https://codereview.chromium.org/20578004/diff/1/chrome/installer/automation_t... chrome/installer/automation_test/chrome_installed.prop:3: "HKEY_CURRENT_USER\\Software\\Google\\Update\\Clients\\{8A69D345-D564-463c-AFF1-A69D9E530F96}": {"expected": true} Prefer to terminate all lists/dicts in JSON with commas after the last item, this is the preferred style across chromium when the language allows it. The benefit is that you can add and remove items without worrying about fixing the comma of the last item... https://codereview.chromium.org/20578004/diff/1/chrome/installer/automation_t... File chrome/installer/automation_test/chromeframe_installed.prop (right): https://codereview.chromium.org/20578004/diff/1/chrome/installer/automation_t... chrome/installer/automation_test/chromeframe_installed.prop:1: { I thought we said we weren't doing chrome frame for the first pass? https://codereview.chromium.org/20578004/diff/1/chrome/installer/automation_t... File chrome/installer/automation_test/config.config (right): https://codereview.chromium.org/20578004/diff/1/chrome/installer/automation_t... chrome/installer/automation_test/config.config:1: { Start this file with a comment leading to the design doc, or perhaps a comment in the main py file if we can't add a comment in JSON. https://codereview.chromium.org/20578004/diff/1/chrome/installer/automation_t... File chrome/installer/automation_test/test_installer.py (right): https://codereview.chromium.org/20578004/diff/1/chrome/installer/automation_t... chrome/installer/automation_test/test_installer.py:1: # Copyright (c) 2012 The Chromium Authors. All rights reserved. 2013 https://codereview.chromium.org/20578004/diff/1/chrome/installer/automation_t... chrome/installer/automation_test/test_installer.py:25: dict(current_property[key].items() + value.items()) This is the overriding merge I like but Robert doesn't, right? :) This seems easier than one that adds yet doesn't override existing values :). https://codereview.chromium.org/20578004/diff/1/chrome/installer/automation_t... chrome/installer/automation_test/test_installer.py:33: On 2013/07/26 19:32:23, robertshield wrote: > I believe it is two blank lines between top-level function definitions. We should get someone with python readability to take a look after we agree with the fundamentals. https://codereview.chromium.org/20578004/diff/1/chrome/installer/automation_t... chrome/installer/automation_test/test_installer.py:59: print " * Current state: " + state What's the idea behind spaces + *? I guess it makes the output nicer? I suggest making that prefix a constant to make it clear what it achieves and avoid having to perfectly replicate it everywhere. https://codereview.chromium.org/20578004/diff/1/chrome/installer/automation_t... chrome/installer/automation_test/test_installer.py:68: subprocess.call("mini_installer.exe --chrome --uninstall" I don't actually think that works... I don't think the installer can uninstall another install. I would say simply don't add a way to reset for now while we figure how exactly we want to reset. https://codereview.chromium.org/20578004/diff/1/chrome/installer/automation_t... chrome/installer/automation_test/test_installer.py:74: print "Testing [ " + " -> ".join(test) + " ]" What does this print exactly? The format appears weird to me, or is this more python magic?! https://codereview.chromium.org/20578004/diff/1/chrome/installer/automation_t... chrome/installer/automation_test/test_installer.py:81: for i in range(1, len(test), 2): Perhaps assert that len(tests) is odd before running this loop with a comment shortly explaining the expected format? https://codereview.chromium.org/20578004/diff/1/chrome/installer/automation_t... File chrome/installer/automation_test/verifier.py (right): https://codereview.chromium.org/20578004/diff/1/chrome/installer/automation_t... chrome/installer/automation_test/verifier.py:1: # Copyright (c) 2012 The Chromium Authors. All rights reserved. 2013 https://codereview.chromium.org/20578004/diff/1/chrome/installer/automation_t... chrome/installer/automation_test/verifier.py:9: if verifier_name == "RegistryEntries": Is there some kind of switch statement for strings in python? If so, could be nice to use here https://codereview.chromium.org/20578004/diff/1/chrome/installer/automation_t... chrome/installer/automation_test/verifier.py:15: def VerifyRegistryEntries(entries): I would put individual verifiers in their own module our this file will get huge! https://codereview.chromium.org/20578004/diff/1/chrome/installer/automation_t... chrome/installer/automation_test/verifier.py:35: print " - " + key Constantify/document arbitrary formatting spaces https://codereview.chromium.org/20578004/diff/1/chrome/installer/automation_t... chrome/installer/automation_test/verifier.py:42: reg_key = _winreg.OpenKey(RootKeyConstant(root_key), \ Why do you need a backslash here? (Sorry if this is obvious I don't know python much) https://codereview.chromium.org/20578004/diff/1/chrome/installer/automation_t... chrome/installer/automation_test/verifier.py:47: # TODO: implement value Name your TODO, i.e., TODO(sukolsak)
https://codereview.chromium.org/20578004/diff/1/chrome/installer/automation_t... File chrome/installer/automation_test/chrome_installed.prop (right): https://codereview.chromium.org/20578004/diff/1/chrome/installer/automation_t... chrome/installer/automation_test/chrome_installed.prop:3: "HKEY_CURRENT_USER\\Software\\Google\\Update\\Clients\\{8A69D345-D564-463c-AFF1-A69D9E530F96}": {"expected": true} On 2013/07/26 20:39:48, gab wrote: > Prefer to terminate all lists/dicts in JSON with commas after the last item, > this is the preferred style across chromium when the language allows it. > > The benefit is that you can add and remove items without worrying about fixing > the comma of the last item... The JSON spec does not allow trailing commas.
https://codereview.chromium.org/20578004/diff/1/chrome/installer/automation_t... File chrome/installer/automation_test/chrome_installed.prop (right): https://codereview.chromium.org/20578004/diff/1/chrome/installer/automation_t... chrome/installer/automation_test/chrome_installed.prop:3: "HKEY_CURRENT_USER\\Software\\Google\\Update\\Clients\\{8A69D345-D564-463c-AFF1-A69D9E530F96}": {"expected": true} On 2013/07/26 21:31:03, sukolsak wrote: > On 2013/07/26 20:39:48, gab wrote: > > Prefer to terminate all lists/dicts in JSON with commas after the last item, > > this is the preferred style across chromium when the language allows it. > > > > The benefit is that you can add and remove items without worrying about fixing > > the comma of the last item... > > The JSON spec does not allow trailing commas. Ah :(, does the python parser allow them anyways? I think most of our existing JSON-like formats use trailing commas.
This looks pretty cool. Some preliminary comments below. https://codereview.chromium.org/20578004/diff/1/chrome/installer/automation_t... File chrome/installer/automation_test/chrome_installed.prop (right): https://codereview.chromium.org/20578004/diff/1/chrome/installer/automation_t... chrome/installer/automation_test/chrome_installed.prop:1: { On 2013/07/26 20:39:48, gab wrote: > On 2013/07/26 19:32:23, robertshield wrote: > > Suggest adding these files under a sub directory. automation_test/data or > > something similar > > Right, I suggest a "config" directory instead of "data" though. > > I also think this all should go in chrome/installer/mini_installer/test instead > of automation_test (all tests are automated ;)!) Judging by the current directory structure under src/chrome, I think src/chrome/test/mini_installer is more appropriate than src/chrome/SOMETHING/test. Is there any overlap between the goals of this project and those of chrome/test/install_test? Can anything there be reused? https://codereview.chromium.org/20578004/diff/1/chrome/installer/automation_t... chrome/installer/automation_test/chrome_installed.prop:1: { Consider giving these files a .json suffix so that editors know the format. https://codereview.chromium.org/20578004/diff/1/chrome/installer/automation_t... chrome/installer/automation_test/chrome_installed.prop:3: "HKEY_CURRENT_USER\\Software\\Google\\Update\\Clients\\{8A69D345-D564-463c-AFF1-A69D9E530F96}": {"expected": true} On 2013/07/29 20:40:26, gab wrote: > I think most of our existing JSON-like formats use trailing commas. Maybe you're thinking of our GYP files, which are python rather than JSON. I believe that Chrome's JSON parser will tell you off if you try to slip in a trailing comma (been there, done that, wasted 20 minutes trying to figure out why my master_preferences were being ignored). https://codereview.chromium.org/20578004/diff/10001/chrome/installer/automati... File chrome/installer/automation_test/data/chrome_installed.prop (right): https://codereview.chromium.org/20578004/diff/10001/chrome/installer/automati... chrome/installer/automation_test/data/chrome_installed.prop:2: "RegistryEntries": { two-space indent, no tabs, etc... https://codereview.chromium.org/20578004/diff/10001/chrome/installer/automati... File chrome/installer/automation_test/test_installer.py (right): https://codereview.chromium.org/20578004/diff/10001/chrome/installer/automati... chrome/installer/automation_test/test_installer.py:1: # Copyright (c) 2012 The Chromium Authors. All rights reserved. the new-style notice is (note: no "(c)" and use the current year): Copyright 2013 The Chromium Authors. All rights reserved. please update all files accordingly, including the data files. https://codereview.chromium.org/20578004/diff/10001/chrome/installer/automati... chrome/installer/automation_test/test_installer.py:19: # Merge the new Property object into the current Property object i don't have python readability, so take this with a grain of salt: i think our python style calls for use of doc strings here rather than comments. please read maruel's wonderful chromium-dev post about python style: https://groups.google.com/a/chromium.org/d/topic/chromium-dev/2BeUNscpeB4/dis... if you haven't already seen it, this might be useful: http://google-styleguide.googlecode.com/svn/trunk/pyguide.html, but keep with chromium's formatting rules. https://codereview.chromium.org/20578004/diff/10001/chrome/installer/automati... chrome/installer/automation_test/test_installer.py:32: def ParseProperty(property_filename, dir): "dir" is the name of a built-in function. please use "directory" here and elsewhere instead. https://codereview.chromium.org/20578004/diff/10001/chrome/installer/automati... chrome/installer/automation_test/test_installer.py:33: property_file = open(os.path.join(dir, property_filename), "r") use the "with" statement when opening files here and elsewhere, a la: with open(os.path.join(dir, property_filename), "r") as property_file: return json.load(property_file) https://codereview.chromium.org/20578004/diff/10001/chrome/installer/automati... chrome/installer/automation_test/test_installer.py:79: subprocess.call("mini_installer.exe --chrome --uninstall" this isn't a true chrome uninstall command line, and may not be correct depending on how chrome is installed on the machine. is this just to show what might be done here, or is this intended to work?
https://codereview.chromium.org/20578004/diff/10001/chrome/installer/automati... File chrome/installer/automation_test/test_installer.py (right): https://codereview.chromium.org/20578004/diff/10001/chrome/installer/automati... chrome/installer/automation_test/test_installer.py:79: subprocess.call("mini_installer.exe --chrome --uninstall" On 2013/07/30 03:31:38, grt wrote: > this isn't a true chrome uninstall command line, and may not be correct > depending on how chrome is installed on the machine. is this just to show what > might be done here, or is this intended to work? As I suggested on a previous patch set, I think we should just not add RunResetCommand() for now (maybe add a TODO a the top of Test() below instead) until we have figured out how we want to handle machine reset (not required to get something going imo).
https://codereview.chromium.org/20578004/diff/1/chrome/installer/automation_t... File chrome/installer/automation_test/chrome_installed.prop (right): https://codereview.chromium.org/20578004/diff/1/chrome/installer/automation_t... chrome/installer/automation_test/chrome_installed.prop:1: { On 2013/07/30 03:31:38, grt (no reviews Aug 3-11) wrote: > On 2013/07/26 20:39:48, gab wrote: > > On 2013/07/26 19:32:23, robertshield wrote: > > > Suggest adding these files under a sub directory. automation_test/data or > > > something similar > > > > Right, I suggest a "config" directory instead of "data" though. > > > > I also think this all should go in chrome/installer/mini_installer/test > instead > > of automation_test (all tests are automated ;)!) > > Judging by the current directory structure under src/chrome, I think > src/chrome/test/mini_installer is more appropriate than > src/chrome/SOMETHING/test. > > Is there any overlap between the goals of this project and those of > chrome/test/install_test? Can anything there be reused? Makes sense, there already seems to be chrome/test/mini_installer_test; those are the old mini_installer tests that are completely broken (and thus never used) and the new framework will completely replace them. We should probably just get rid of this first and then put the new framework's code there. WDYT?
Thanks for the comments. https://codereview.chromium.org/20578004/diff/1/chrome/installer/automation_t... File chrome/installer/automation_test/chrome_installed.prop (right): https://codereview.chromium.org/20578004/diff/1/chrome/installer/automation_t... chrome/installer/automation_test/chrome_installed.prop:1: { On 2013/07/30 03:31:38, grt (no reviews Aug 3-11) wrote: > Consider giving these files a .json suffix so that editors know the format. I use .prop and .config suffixes in the spec to distinguish between property files and config files. Would it be confusing if all the data files have .json suffix? https://codereview.chromium.org/20578004/diff/1/chrome/installer/automation_t... File chrome/installer/automation_test/test_installer.py (right): https://codereview.chromium.org/20578004/diff/1/chrome/installer/automation_t... chrome/installer/automation_test/test_installer.py:74: print "Testing [ " + " -> ".join(test) + " ]" On 2013/07/26 20:39:48, gab wrote: > What does this print exactly? The format appears weird to me, or is this more > python magic?! It joins the strings in the array with " -> ". So, it basically prints "state -> action -> state -> ... -> state". I will make it clearer. https://codereview.chromium.org/20578004/diff/1/chrome/installer/automation_t... File chrome/installer/automation_test/verifier.py (right): https://codereview.chromium.org/20578004/diff/1/chrome/installer/automation_t... chrome/installer/automation_test/verifier.py:9: if verifier_name == "RegistryEntries": On 2013/07/26 20:39:48, gab wrote: > Is there some kind of switch statement for strings in python? If so, could be > nice to use here Python doesn't have a switch statement. https://codereview.chromium.org/20578004/diff/10001/chrome/installer/automati... File chrome/installer/automation_test/test_installer.py (right): https://codereview.chromium.org/20578004/diff/10001/chrome/installer/automati... chrome/installer/automation_test/test_installer.py:1: # Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2013/07/30 03:31:38, grt (no reviews Aug 3-11) wrote: > the new-style notice is (note: no "(c)" and use the current year): > Copyright 2013 The Chromium Authors. All rights reserved. > please update all files accordingly, including the data files. Should we put the notice in the data files? The data files are in JSON format, which doesn't allow comments.
Gotta run, didn't have time to look at the .py files; will look next time I'm online. Cheers! Gab https://codereview.chromium.org/20578004/diff/1/chrome/installer/automation_t... File chrome/installer/automation_test/chrome_installed.prop (right): https://codereview.chromium.org/20578004/diff/1/chrome/installer/automation_t... chrome/installer/automation_test/chrome_installed.prop:1: { On 2013/07/30 15:40:32, sukolsak wrote: > On 2013/07/30 03:31:38, grt (no reviews Aug 3-11) wrote: > > Consider giving these files a .json suffix so that editors know the format. > > I use .prop and .config suffixes in the spec to distinguish between property > files and config files. Would it be confusing if all the data files have .json > suffix? I prefer separate extensions to separate what we expect to find in those files. .json is too generic imo (i.e., the parser, after parsing the JSON expects specific things within the JSON, so it's more than just JSON) https://codereview.chromium.org/20578004/diff/10001/chrome/installer/automati... File chrome/installer/automation_test/test_installer.py (right): https://codereview.chromium.org/20578004/diff/10001/chrome/installer/automati... chrome/installer/automation_test/test_installer.py:1: # Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2013/07/30 15:40:32, sukolsak wrote: > On 2013/07/30 03:31:38, grt (no reviews Aug 3-11) wrote: > > the new-style notice is (note: no "(c)" and use the current year): > > Copyright 2013 The Chromium Authors. All rights reserved. > > please update all files accordingly, including the data files. > > Should we put the notice in the data files? The data files are in JSON format, > which doesn't allow comments. Ya, I don't think we can, but it's pure data, not code, so I think it's fine... https://codereview.chromium.org/20578004/diff/27001/chrome/test/mini_installe... File chrome/test/mini_installer_test/data/chrome_installed.prop (right): https://codereview.chromium.org/20578004/diff/27001/chrome/test/mini_installe... chrome/test/mini_installer_test/data/chrome_installed.prop:3: "HKEY_CURRENT_USER\\Software\\Google\\Update\\Clients\\{8A69D345-D564-463c-AFF1-A69D9E530F96}": {"expected": true} Wrap {"expected": true} to the next line, 4 extra spaces in, to try to respect the 80 columns rule as best as possible (I don't think breaking down the key's path itself is necessary). Same comment for all other such json files. https://codereview.chromium.org/20578004/diff/27001/chrome/test/mini_installe... File chrome/test/mini_installer_test/data/config.config (right): https://codereview.chromium.org/20578004/diff/27001/chrome/test/mini_installe... chrome/test/mini_installer_test/data/config.config:8: ["uninstall chrome", "mini_installer.exe --chrome --uninstall"] This should do the real uninstall imo, i.e. run whatever Chrome registered as its uninstall command @ "HKEY_CURRENT_USER\\Software\\Google\\Update\\Clients\\{8A69D345-D564-463c-AFF1-A69D9E530F96}\\(UninstallCommand + UninstallArguments)" Perhaps the command here should point to a script that knows how to do that.
Second pass. Please ping me if anything is unclear. Cheers! Gab https://codereview.chromium.org/20578004/diff/27001/chrome/test/mini_installe... File chrome/test/mini_installer_test/data/chrome_installed.prop (right): https://codereview.chromium.org/20578004/diff/27001/chrome/test/mini_installe... chrome/test/mini_installer_test/data/chrome_installed.prop:1: { As suggested earlier, how about putting these in a config/ directory rather than data/? This is more inline with what these files actually are imo.. https://codereview.chromium.org/20578004/diff/27001/chrome/test/mini_installe... File chrome/test/mini_installer_test/data/config.config (right): https://codereview.chromium.org/20578004/diff/27001/chrome/test/mini_installe... chrome/test/mini_installer_test/data/config.config:8: ["uninstall chrome", "mini_installer.exe --chrome --uninstall"] On 2013/07/30 20:44:14, gab wrote: > This should do the real uninstall imo, i.e. run whatever Chrome registered as > its uninstall command @ > "HKEY_CURRENT_USER\\Software\\Google\\Update\\Clients\\{8A69D345-D564-463c-AFF1-A69D9E530F96}\\(UninstallCommand > + UninstallArguments)" > > Perhaps the command here should point to a script that knows how to do that. For now I'm fine with this being a script that does the wrong thing with a TODO, e.g. uninstall.py: # TODO(sukolsak): This should read the uninstall command from the registry and run that instead. subprocess.call("mini_installer.exe --chrome --uninstall", Shell=True) I think the script will eventually need to take at least a product key (the product to be uninstalled); e.g., {8A69D345-D564-463c-AFF1-A69D9E530F96} so that it can read the proper uninstall command from the registry, but the above with a TODO should be roughly fine for this version (and then I guess the command in the config for now becomes "python uninstall.py"). Does that make sense? https://codereview.chromium.org/20578004/diff/27001/chrome/test/mini_installe... File chrome/test/mini_installer_test/registry_verifier.py (right): https://codereview.chromium.org/20578004/diff/27001/chrome/test/mini_installe... chrome/test/mini_installer_test/registry_verifier.py:30: expected = entry["expected"] Does this raise an exception if "expected" hasn't been specified? It probably should somehow fail given this is a mandatory value for registry entries. https://codereview.chromium.org/20578004/diff/27001/chrome/test/mini_installe... chrome/test/mini_installer_test/registry_verifier.py:33: print "exists...", Don't these prints need the PRINT_VERIFIER_PREFIX as well? https://codereview.chromium.org/20578004/diff/27001/chrome/test/mini_installe... chrome/test/mini_installer_test/registry_verifier.py:39: sub_key, 0, _winreg.KEY_READ) Indent to delimiter ('(') or 4 spaces in, not a mix of both, see http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Indent... https://codereview.chromium.org/20578004/diff/27001/chrome/test/mini_installe... chrome/test/mini_installer_test/registry_verifier.py:40: if expected == False: Doesn't if !expected work here? https://codereview.chromium.org/20578004/diff/27001/chrome/test/mini_installe... chrome/test/mini_installer_test/registry_verifier.py:47: return expected == False !expected ? I don't know python much, can we not assume |expected| is a bool here perhaps and thus need to do ==False? https://codereview.chromium.org/20578004/diff/27001/chrome/test/mini_installe... File chrome/test/mini_installer_test/settings.py (right): https://codereview.chromium.org/20578004/diff/27001/chrome/test/mini_installe... chrome/test/mini_installer_test/settings.py:6: PRINT_COMMAND_PREFIX = " * Command: " nit: align '=' with others https://codereview.chromium.org/20578004/diff/27001/chrome/test/mini_installe... chrome/test/mini_installer_test/settings.py:7: PRINT_STATE_PREFIX = " * State: " Why is this a single space less than the command prefix? https://codereview.chromium.org/20578004/diff/27001/chrome/test/mini_installe... chrome/test/mini_installer_test/settings.py:8: PRINT_VERIFIER_PREFIX = " - Verify that " To go inline with the rest of our coding style, how about having 2 spaces indent (i.e., 4 here and 2 for command/state?) https://codereview.chromium.org/20578004/diff/27001/chrome/test/mini_installe... File chrome/test/mini_installer_test/test_installer.py (right): https://codereview.chromium.org/20578004/diff/27001/chrome/test/mini_installe... chrome/test/mini_installer_test/test_installer.py:10: Read the design documentation at http://goo.gl/Q0rGM6 s/Read/For more details take a look at https://codereview.chromium.org/20578004/diff/27001/chrome/test/mini_installe... chrome/test/mini_installer_test/test_installer.py:42: isinstance(value, dict)) fix indent (i.e., wrap to '(' above) https://codereview.chromium.org/20578004/diff/27001/chrome/test/mini_installe... chrome/test/mini_installer_test/test_installer.py:44: current_property[key].items() + value.items()) Does this merge (i.e., override) properties with the same name? Either way, add a comment stating exactly what this does. https://codereview.chromium.org/20578004/diff/27001/chrome/test/mini_installe... chrome/test/mini_installer_test/test_installer.py:47: def ParseProperty(property_filename, directory): I suggest flipping the arguments here since native system calls take directory first than filename; it's probably more intuitive for experienced python developers to have directory first than filename... https://codereview.chromium.org/20578004/diff/27001/chrome/test/mini_installe... chrome/test/mini_installer_test/test_installer.py:59: with open(property_path, "r") as property_file: Any failure handling required here? Or will an exception be bubbled back up and kill the test (that's ok) if it happens? https://codereview.chromium.org/20578004/diff/27001/chrome/test/mini_installe... chrome/test/mini_installer_test/test_installer.py:63: def ParseProperties(property_filenames, directory): Same comment here about argument order https://codereview.chromium.org/20578004/diff/27001/chrome/test/mini_installe... chrome/test/mini_installer_test/test_installer.py:93: with open(config_filename, "r") as config_file: Does config_filename need to be absolute? Either way the comments should specify that. https://codereview.chromium.org/20578004/diff/27001/chrome/test/mini_installe... chrome/test/mini_installer_test/test_installer.py:99: directory) fix indent https://codereview.chromium.org/20578004/diff/27001/chrome/test/mini_installe... chrome/test/mini_installer_test/test_installer.py:143: VerifyState(config, current_state) Add a TODO to quit the test early if VerifyState fails at any point here or below (any later failure has no meaning if a previous state is incorrect). https://codereview.chromium.org/20578004/diff/27001/chrome/test/mini_installe... File chrome/test/mini_installer_test/verifier.py (right): https://codereview.chromium.org/20578004/diff/27001/chrome/test/mini_installe... chrome/test/mini_installer_test/verifier.py:14: pass Fail the test in the "else" case. i.e., if someone writes a .prop file with a typo in one of the properties, the test should fail instead of silently ignoring the miss-typed property. You will likely want to use the python "unittest" framework to report failures (instead of basic prints), I believe our test infrastructure already handles parsing failures from the python unittest framework. For this first CL prints are ok I'd say, but please switch to the "unittest" framework in a follow-up CL.
General comment on the reviewing process before I take another look: When reviewers add comments inline, please respond to each one individually, even if the response is simply to change a line of code. For comments that don't need an elaborate response (which is usually most of them), the code review UI provides a "Done" button that serves as a simple ACK. Having each comment acknowledged and responded to lets reviewers know that their feedback has been integrated. Thanks!
https://codereview.chromium.org/20578004/diff/1/chrome/installer/automation_t... File chrome/installer/automation_test/chrome_installed.prop (right): https://codereview.chromium.org/20578004/diff/1/chrome/installer/automation_t... chrome/installer/automation_test/chrome_installed.prop:1: { On 2013/07/30 15:11:43, gab wrote: > On 2013/07/30 03:31:38, grt (no reviews Aug 3-11) wrote: > > On 2013/07/26 20:39:48, gab wrote: > > > On 2013/07/26 19:32:23, robertshield wrote: > > > > Suggest adding these files under a sub directory. automation_test/data or > > > > something similar > > > > > > Right, I suggest a "config" directory instead of "data" though. > > > > > > I also think this all should go in chrome/installer/mini_installer/test > > instead > > > of automation_test (all tests are automated ;)!) > > > > Judging by the current directory structure under src/chrome, I think > > src/chrome/test/mini_installer is more appropriate than > > src/chrome/SOMETHING/test. > > > > Is there any overlap between the goals of this project and those of > > chrome/test/install_test? Can anything there be reused? > > Makes sense, there already seems to be chrome/test/mini_installer_test; those > are the old mini_installer tests that are completely broken (and thus never > used) and the new framework will completely replace them. > > We should probably just get rid of this first and then put the new framework's > code there. > > WDYT? I don't like the redundancy of chrome/test/mini_installer_test. I think chrome/test/mini_installer is much better. https://codereview.chromium.org/20578004/diff/1/chrome/installer/automation_t... chrome/installer/automation_test/chrome_installed.prop:1: { On 2013/07/30 20:44:14, gab wrote: > On 2013/07/30 15:40:32, sukolsak wrote: > > On 2013/07/30 03:31:38, grt (no reviews Aug 3-11) wrote: > > > Consider giving these files a .json suffix so that editors know the format. > > > > I use .prop and .config suffixes in the spec to distinguish between property > > files and config files. Would it be confusing if all the data files have .json > > suffix? > > I prefer separate extensions to separate what we expect to find in those files. > > .json is too generic imo (i.e., the parser, after parsing the JSON expects > specific things within the JSON, so it's more than just JSON) SGTM. https://codereview.chromium.org/20578004/diff/27001/chrome/test/mini_installe... File chrome/test/mini_installer_test/data/chrome_installed.prop (right): https://codereview.chromium.org/20578004/diff/27001/chrome/test/mini_installe... chrome/test/mini_installer_test/data/chrome_installed.prop:3: "HKEY_CURRENT_USER\\Software\\Google\\Update\\Clients\\{8A69D345-D564-463c-AFF1-A69D9E530F96}": {"expected": true} Strictly speaking, the presence/absence of the registry value named "pv" within the Clients key is the thing that indicates to Google Update that a product is installed. I think the framework should make the same test. Ultimately, the framework should verify that the version number present in the "pv" registry value is the one that was expected given the install/update operation that preceded the verification. Please at least put a TODO somewhere that this should be fixed down the road. https://codereview.chromium.org/20578004/diff/27001/chrome/test/mini_installe... File chrome/test/mini_installer_test/data/config.config (right): https://codereview.chromium.org/20578004/diff/27001/chrome/test/mini_installe... chrome/test/mini_installer_test/data/config.config:8: ["uninstall chrome", "mini_installer.exe --chrome --uninstall"] On 2013/07/31 13:45:16, gab wrote: > On 2013/07/30 20:44:14, gab wrote: > > This should do the real uninstall imo, i.e. run whatever Chrome registered as > > its uninstall command @ > > > "HKEY_CURRENT_USER\\Software\\Google\\Update\\Clients\\{8A69D345-D564-463c-AFF1-A69D9E530F96}\\(UninstallCommand > > + UninstallArguments)" > > > > Perhaps the command here should point to a script that knows how to do that. > > For now I'm fine with this being a script that does the wrong thing with a TODO, > e.g. uninstall.py: > > # TODO(sukolsak): This should read the uninstall command from the registry and > run that instead. > subprocess.call("mini_installer.exe --chrome --uninstall", Shell=True) > > > I think the script will eventually need to take at least a product key (the > product to be uninstalled); e.g., {8A69D345-D564-463c-AFF1-A69D9E530F96} so that > it can read the proper uninstall command from the registry, but the above with a > TODO should be roughly fine for this version (and then I guess the command in > the config for now becomes "python uninstall.py"). > > Does that make sense? Note that the command to run to uninstall the MSI is unrelated to the ordinary Chrome uninstall commands. If the infrastructure will (eventually) support installing/testing/uninstalling the MSI, then this needs to be taken into account somehow. https://codereview.chromium.org/20578004/diff/27001/chrome/test/mini_installe... chrome/test/mini_installer_test/data/config.config:8: ["uninstall chrome", "mini_installer.exe --chrome --uninstall"] The correct uninstall command here is "--uninstall --multi-install --chrome" since the corresponding install command include "--multi-install".
https://codereview.chromium.org/20578004/diff/27001/chrome/test/mini_installe... File chrome/test/mini_installer_test/data/chrome_installed.prop (right): https://codereview.chromium.org/20578004/diff/27001/chrome/test/mini_installe... chrome/test/mini_installer_test/data/chrome_installed.prop:3: "HKEY_CURRENT_USER\\Software\\Google\\Update\\Clients\\{8A69D345-D564-463c-AFF1-A69D9E530F96}": {"expected": true} On 2013/07/31 17:20:05, grt (no reviews Aug 3-11) wrote: > Strictly speaking, the presence/absence of the registry value named "pv" within > the Clients key is the thing that indicates to Google Update that a product is > installed. I think the framework should make the same test. > > Ultimately, the framework should verify that the version number present in the > "pv" registry value is the one that was expected given the install/update > operation that preceded the verification. > > Please at least put a TODO somewhere that this should be fixed down the road. Eventually the goal is to have ALL the registry values listed here; so I think it's ok for now to leave it as an implied TODO that this list will grow (since we can't put comments in JSON anyways...).
Thank you for the comments. https://codereview.chromium.org/20578004/diff/27001/chrome/test/mini_installe... File chrome/test/mini_installer_test/data/chrome_installed.prop (right): https://codereview.chromium.org/20578004/diff/27001/chrome/test/mini_installe... chrome/test/mini_installer_test/data/chrome_installed.prop:1: { On 2013/07/31 13:45:16, gab wrote: > As suggested earlier, how about putting these in a config/ directory rather than > data/? > > This is more inline with what these files actually are imo.. Done. https://codereview.chromium.org/20578004/diff/27001/chrome/test/mini_installe... chrome/test/mini_installer_test/data/chrome_installed.prop:3: "HKEY_CURRENT_USER\\Software\\Google\\Update\\Clients\\{8A69D345-D564-463c-AFF1-A69D9E530F96}": {"expected": true} On 2013/07/30 20:44:14, gab wrote: > Wrap {"expected": true} to the next line, 4 extra spaces in, to try to respect > the 80 columns rule as best as possible (I don't think breaking down the key's > path itself is necessary). > > Same comment for all other such json files. Done. https://codereview.chromium.org/20578004/diff/27001/chrome/test/mini_installe... File chrome/test/mini_installer_test/data/config.config (right): https://codereview.chromium.org/20578004/diff/27001/chrome/test/mini_installe... chrome/test/mini_installer_test/data/config.config:8: ["uninstall chrome", "mini_installer.exe --chrome --uninstall"] On 2013/07/31 13:45:16, gab wrote: > On 2013/07/30 20:44:14, gab wrote: > > This should do the real uninstall imo, i.e. run whatever Chrome registered as > > its uninstall command @ > > > "HKEY_CURRENT_USER\\Software\\Google\\Update\\Clients\\{8A69D345-D564-463c-AFF1-A69D9E530F96}\\(UninstallCommand > > + UninstallArguments)" > > > > Perhaps the command here should point to a script that knows how to do that. > > For now I'm fine with this being a script that does the wrong thing with a TODO, > e.g. uninstall.py: > > # TODO(sukolsak): This should read the uninstall command from the registry and > run that instead. > subprocess.call("mini_installer.exe --chrome --uninstall", Shell=True) > > > I think the script will eventually need to take at least a product key (the > product to be uninstalled); e.g., {8A69D345-D564-463c-AFF1-A69D9E530F96} so that > it can read the proper uninstall command from the registry, but the above with a > TODO should be roughly fine for this version (and then I guess the command in > the config for now becomes "python uninstall.py"). > > Does that make sense? Done. https://codereview.chromium.org/20578004/diff/27001/chrome/test/mini_installe... chrome/test/mini_installer_test/data/config.config:8: ["uninstall chrome", "mini_installer.exe --chrome --uninstall"] On 2013/07/31 17:20:05, grt (no reviews Aug 3-11) wrote: > The correct uninstall command here is "--uninstall --multi-install --chrome" > since the corresponding install command include "--multi-install". Done. https://codereview.chromium.org/20578004/diff/27001/chrome/test/mini_installe... File chrome/test/mini_installer_test/registry_verifier.py (right): https://codereview.chromium.org/20578004/diff/27001/chrome/test/mini_installe... chrome/test/mini_installer_test/registry_verifier.py:30: expected = entry["expected"] On 2013/07/31 13:45:16, gab wrote: > Does this raise an exception if "expected" hasn't been specified? It probably > should somehow fail given this is a mandatory value for registry entries. Python will raise the KeyError exception when the key is not in the dictionary. https://codereview.chromium.org/20578004/diff/27001/chrome/test/mini_installe... chrome/test/mini_installer_test/registry_verifier.py:33: print "exists...", On 2013/07/31 13:45:16, gab wrote: > Don't these prints need the PRINT_VERIFIER_PREFIX as well? No. I want it to be on the same line. The output will look like this: - Verify that HKEY\a exists... Passed - Verify that HKEY\b doesn't exist... Passed https://codereview.chromium.org/20578004/diff/27001/chrome/test/mini_installe... chrome/test/mini_installer_test/registry_verifier.py:39: sub_key, 0, _winreg.KEY_READ) On 2013/07/31 13:45:16, gab wrote: > Indent to delimiter ('(') or 4 spaces in, not a mix of both, see > http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Indent... Done. https://codereview.chromium.org/20578004/diff/27001/chrome/test/mini_installe... chrome/test/mini_installer_test/registry_verifier.py:40: if expected == False: On 2013/07/31 13:45:16, gab wrote: > Doesn't > > if !expected > > work here? Done. https://codereview.chromium.org/20578004/diff/27001/chrome/test/mini_installe... chrome/test/mini_installer_test/registry_verifier.py:47: return expected == False On 2013/07/31 13:45:16, gab wrote: > !expected > > ? I don't know python much, can we not assume |expected| is a bool here perhaps > and thus need to do ==False? Done. https://codereview.chromium.org/20578004/diff/27001/chrome/test/mini_installe... File chrome/test/mini_installer_test/settings.py (right): https://codereview.chromium.org/20578004/diff/27001/chrome/test/mini_installe... chrome/test/mini_installer_test/settings.py:6: PRINT_COMMAND_PREFIX = " * Command: " On 2013/07/31 13:45:16, gab wrote: > nit: align '=' with others Done. https://codereview.chromium.org/20578004/diff/27001/chrome/test/mini_installe... chrome/test/mini_installer_test/settings.py:7: PRINT_STATE_PREFIX = " * State: " On 2013/07/31 13:45:16, gab wrote: > Why is this a single space less than the command prefix? I think they are both 4 spaces in. https://codereview.chromium.org/20578004/diff/27001/chrome/test/mini_installe... chrome/test/mini_installer_test/settings.py:8: PRINT_VERIFIER_PREFIX = " - Verify that " On 2013/07/31 13:45:16, gab wrote: > To go inline with the rest of our coding style, how about having 2 spaces indent > (i.e., 4 here and 2 for command/state?) Done. https://codereview.chromium.org/20578004/diff/27001/chrome/test/mini_installe... File chrome/test/mini_installer_test/test_installer.py (right): https://codereview.chromium.org/20578004/diff/27001/chrome/test/mini_installe... chrome/test/mini_installer_test/test_installer.py:10: Read the design documentation at http://goo.gl/Q0rGM6 On 2013/07/31 13:45:16, gab wrote: > s/Read/For more details take a look at Done. https://codereview.chromium.org/20578004/diff/27001/chrome/test/mini_installe... chrome/test/mini_installer_test/test_installer.py:42: isinstance(value, dict)) On 2013/07/31 13:45:16, gab wrote: > fix indent (i.e., wrap to '(' above) Done. https://codereview.chromium.org/20578004/diff/27001/chrome/test/mini_installe... chrome/test/mini_installer_test/test_installer.py:44: current_property[key].items() + value.items()) On 2013/07/31 13:45:16, gab wrote: > Does this merge (i.e., override) properties with the same name? > > Either way, add a comment stating exactly what this does. Yes. Comment added. https://codereview.chromium.org/20578004/diff/27001/chrome/test/mini_installe... chrome/test/mini_installer_test/test_installer.py:47: def ParseProperty(property_filename, directory): On 2013/07/31 13:45:16, gab wrote: > I suggest flipping the arguments here since native system calls take directory > first than filename; it's probably more intuitive for experienced python > developers to have directory first than filename... Done. https://codereview.chromium.org/20578004/diff/27001/chrome/test/mini_installe... chrome/test/mini_installer_test/test_installer.py:59: with open(property_path, "r") as property_file: On 2013/07/31 13:45:16, gab wrote: > Any failure handling required here? Or will an exception be bubbled back up and > kill the test (that's ok) if it happens? The exception IOError will be bubbled back up and kill the test. https://codereview.chromium.org/20578004/diff/27001/chrome/test/mini_installe... chrome/test/mini_installer_test/test_installer.py:63: def ParseProperties(property_filenames, directory): On 2013/07/31 13:45:16, gab wrote: > Same comment here about argument order Done. https://codereview.chromium.org/20578004/diff/27001/chrome/test/mini_installe... chrome/test/mini_installer_test/test_installer.py:93: with open(config_filename, "r") as config_file: On 2013/07/31 13:45:16, gab wrote: > Does config_filename need to be absolute? > > Either way the comments should specify that. It can be relative or absolute. I think the open operator in most major languages does this. So this is trivial. https://codereview.chromium.org/20578004/diff/27001/chrome/test/mini_installe... chrome/test/mini_installer_test/test_installer.py:99: directory) On 2013/07/31 13:45:16, gab wrote: > fix indent Done. https://codereview.chromium.org/20578004/diff/27001/chrome/test/mini_installe... chrome/test/mini_installer_test/test_installer.py:143: VerifyState(config, current_state) On 2013/07/31 13:45:16, gab wrote: > Add a TODO to quit the test early if VerifyState fails at any point here or > below (any later failure has no meaning if a previous state is incorrect). Done. https://codereview.chromium.org/20578004/diff/27001/chrome/test/mini_installe... File chrome/test/mini_installer_test/verifier.py (right): https://codereview.chromium.org/20578004/diff/27001/chrome/test/mini_installe... chrome/test/mini_installer_test/verifier.py:14: pass On 2013/07/31 13:45:16, gab wrote: > Fail the test in the "else" case. i.e., if someone writes a .prop file with a > typo in one of the properties, the test should fail instead of silently ignoring > the miss-typed property. > > You will likely want to use the python "unittest" framework to report failures > (instead of basic prints), I believe our test infrastructure already handles > parsing failures from the python unittest framework. > > For this first CL prints are ok I'd say, but please switch to the "unittest" > framework in a follow-up CL. Done.
On 2013/07/31 13:52:14, robertshield wrote: > General comment on the reviewing process before I take another look: > > When reviewers add comments inline, please respond to each one individually, > even if the response is simply to change a line of code. For comments that don't > need an elaborate response (which is usually most of them), the code review UI > provides a "Done" button that serves as a simple ACK. > > Having each comment acknowledged and responded to lets reviewers know that their > feedback has been integrated. > > Thanks! Sorry about that. I was not familiar with the process. I will do it next time.
https://codereview.chromium.org/20578004/diff/37002/chrome/test/mini_installe... File chrome/test/mini_installer/test_installer.py (right): https://codereview.chromium.org/20578004/diff/37002/chrome/test/mini_installe... chrome/test/mini_installer/test_installer.py:21: """Describes the machine states, actions, and test cases.""" Please provide here a very brief description of a state, an action and a test case for those who don't read the design doc. https://codereview.chromium.org/20578004/diff/37002/chrome/test/mini_installe... chrome/test/mini_installer/test_installer.py:30: """Merges the new Property object into the current Property object Is this actually a "Property" object, or is it rather a dict returned by json.load? If the latter, perhaps this should be called MergeDicts https://codereview.chromium.org/20578004/diff/37002/chrome/test/mini_installe... chrome/test/mini_installer/test_installer.py:114: print settings.PRINT_STATE_PREFIX + state These are good to have for now, but when this starts to come together it will not be desired for a passing test to spew lots of logs to stdio. Consider adding a TODO() to think of ways of preserving the log when the test fails but not printing this stuff when the test passes.
https://codereview.chromium.org/20578004/diff/37002/chrome/test/mini_installe... File chrome/test/mini_installer/test_installer.py (right): https://codereview.chromium.org/20578004/diff/37002/chrome/test/mini_installe... chrome/test/mini_installer/test_installer.py:21: """Describes the machine states, actions, and test cases.""" On 2013/07/31 18:28:53, robertshield wrote: > Please provide here a very brief description of a state, an action and a test > case for those who don't read the design doc. Done. https://codereview.chromium.org/20578004/diff/37002/chrome/test/mini_installe... chrome/test/mini_installer/test_installer.py:30: """Merges the new Property object into the current Property object On 2013/07/31 18:28:53, robertshield wrote: > Is this actually a "Property" object, or is it rather a dict returned by > json.load? If the latter, perhaps this should be called MergeDicts It is a Property object. It is also a dictionary. In the spec, we define a Property object to be a dictionary whose keys are verifiers' names. https://codereview.chromium.org/20578004/diff/37002/chrome/test/mini_installe... chrome/test/mini_installer/test_installer.py:114: print settings.PRINT_STATE_PREFIX + state On 2013/07/31 18:28:53, robertshield wrote: > These are good to have for now, but when this starts to come together it will > not be desired for a passing test to spew lots of logs to stdio. Consider adding > a TODO() to think of ways of preserving the log when the test fails but not > printing this stuff when the test passes. Done.
Looks great! We'll also need to add an OWNERS file in chrome\test\mini_installer. For now this should contain me, grt, and robertshield I'd say; see chrome\installer\OWNERS for an example. Cheers! Gab https://codereview.chromium.org/20578004/diff/27001/chrome/test/mini_installe... File chrome/test/mini_installer_test/registry_verifier.py (right): https://codereview.chromium.org/20578004/diff/27001/chrome/test/mini_installe... chrome/test/mini_installer_test/registry_verifier.py:33: print "exists...", On 2013/07/31 18:20:30, sukolsak wrote: > On 2013/07/31 13:45:16, gab wrote: > > Don't these prints need the PRINT_VERIFIER_PREFIX as well? > > No. I want it to be on the same line. The output will look like this: > > - Verify that HKEY\a exists... Passed > - Verify that HKEY\b doesn't exist... Passed Ah ok my bad, assumed print forced an automatic new line. https://codereview.chromium.org/20578004/diff/27001/chrome/test/mini_installe... File chrome/test/mini_installer_test/settings.py (right): https://codereview.chromium.org/20578004/diff/27001/chrome/test/mini_installe... chrome/test/mini_installer_test/settings.py:7: PRINT_STATE_PREFIX = " * State: " On 2013/07/31 18:20:30, sukolsak wrote: > On 2013/07/31 13:45:16, gab wrote: > > Why is this a single space less than the command prefix? > > I think they are both 4 spaces in. Ah, my bad, it's the equality signs that aren't aligned, please align them. https://codereview.chromium.org/20578004/diff/27001/chrome/test/mini_installe... File chrome/test/mini_installer_test/test_installer.py (right): https://codereview.chromium.org/20578004/diff/27001/chrome/test/mini_installe... chrome/test/mini_installer_test/test_installer.py:44: current_property[key].items() + value.items()) On 2013/07/31 18:20:30, sukolsak wrote: > On 2013/07/31 13:45:16, gab wrote: > > Does this merge (i.e., override) properties with the same name? > > > > Either way, add a comment stating exactly what this does. > > Yes. Comment added. Sounds good, can you make it clear in the spec then that we are going with the merge strategy? https://codereview.chromium.org/20578004/diff/48001/chrome/test/mini_installe... File chrome/test/mini_installer/registry_verifier.py (right): https://codereview.chromium.org/20578004/diff/48001/chrome/test/mini_installe... chrome/test/mini_installer/registry_verifier.py:12: print "Passed" Add TODO to use unittest framework instead of print here https://codereview.chromium.org/20578004/diff/48001/chrome/test/mini_installe... chrome/test/mini_installer/registry_verifier.py:26: raise Exception("Unknown registry key") Add TODO to use unittest framework here. https://codereview.chromium.org/20578004/diff/48001/chrome/test/mini_installe... chrome/test/mini_installer/registry_verifier.py:31: print settings.PRINT_VERIFIER_PREFIX + key, Add TODO to remove these prints, i.e.: # TODO(sukolsak): Debug prints to be removed later. https://codereview.chromium.org/20578004/diff/48001/chrome/test/mini_installe... chrome/test/mini_installer/registry_verifier.py:41: return False Add a comment that the code should only continue if the key exists as a WindowsError is otherwise raised (it's fairly obvious now, but it might be less obvious once the code below expands to handle "value" and the WindowsError handling is a few more lines down). https://codereview.chromium.org/20578004/diff/48001/chrome/test/mini_installe... File chrome/test/mini_installer/settings.py (left): https://codereview.chromium.org/20578004/diff/48001/chrome/test/mini_installe... chrome/test/mini_installer/settings.py:1: #!/bin/bash You can upload with --similarity=99 to get rid of copy-detection (copy detection is useful when you actually move a file and slightly modify it, but in your case the default threshold is considering this a copy of some random file which makes the diff kind of weird!). git cl upload --similarity=99 (I'm not sure 100 works, but it probably does... anyways 99 should be sufficient!) https://codereview.chromium.org/20578004/diff/48001/chrome/test/mini_installe... File chrome/test/mini_installer/verifier.py (right): https://codereview.chromium.org/20578004/diff/48001/chrome/test/mini_installe... chrome/test/mini_installer/verifier.py:7: nit: remove extra empty line https://codereview.chromium.org/20578004/diff/48001/chrome/test/mini_installe... chrome/test/mini_installer/verifier.py:14: raise Exception("Unknown verifier") Add to TODO that this should use unittest framework instead of exceptions.
https://codereview.chromium.org/20578004/diff/27001/chrome/test/mini_installe... File chrome/test/mini_installer_test/settings.py (right): https://codereview.chromium.org/20578004/diff/27001/chrome/test/mini_installe... chrome/test/mini_installer_test/settings.py:7: PRINT_STATE_PREFIX = " * State: " On 2013/08/01 20:30:41, gab wrote: > On 2013/07/31 18:20:30, sukolsak wrote: > > On 2013/07/31 13:45:16, gab wrote: > > > Why is this a single space less than the command prefix? > > > > I think they are both 4 spaces in. > > Ah, my bad, it's the equality signs that aren't aligned, please align them. And I see that you've down that already in the latest patchset :) -- woot!
https://codereview.chromium.org/20578004/diff/27001/chrome/test/mini_installe... File chrome/test/mini_installer_test/test_installer.py (right): https://codereview.chromium.org/20578004/diff/27001/chrome/test/mini_installe... chrome/test/mini_installer_test/test_installer.py:44: current_property[key].items() + value.items()) On 2013/08/01 20:30:41, gab wrote: > On 2013/07/31 18:20:30, sukolsak wrote: > > On 2013/07/31 13:45:16, gab wrote: > > > Does this merge (i.e., override) properties with the same name? > > > > > > Either way, add a comment stating exactly what this does. > > > > Yes. Comment added. > > Sounds good, can you make it clear in the spec then that we are going with the > merge strategy? Done. https://codereview.chromium.org/20578004/diff/48001/chrome/test/mini_installe... File chrome/test/mini_installer/registry_verifier.py (right): https://codereview.chromium.org/20578004/diff/48001/chrome/test/mini_installe... chrome/test/mini_installer/registry_verifier.py:12: print "Passed" On 2013/08/01 20:30:41, gab wrote: > Add TODO to use unittest framework instead of print here Done. https://codereview.chromium.org/20578004/diff/48001/chrome/test/mini_installe... chrome/test/mini_installer/registry_verifier.py:26: raise Exception("Unknown registry key") On 2013/08/01 20:30:41, gab wrote: > Add TODO to use unittest framework here. Done. https://codereview.chromium.org/20578004/diff/48001/chrome/test/mini_installe... chrome/test/mini_installer/registry_verifier.py:31: print settings.PRINT_VERIFIER_PREFIX + key, On 2013/08/01 20:30:41, gab wrote: > Add TODO to remove these prints, i.e.: > > # TODO(sukolsak): Debug prints to be removed later. Done. https://codereview.chromium.org/20578004/diff/48001/chrome/test/mini_installe... chrome/test/mini_installer/registry_verifier.py:41: return False On 2013/08/01 20:30:41, gab wrote: > Add a comment that the code should only continue if the key exists as a > WindowsError is otherwise raised (it's fairly obvious now, but it might be less > obvious once the code below expands to handle "value" and the WindowsError > handling is a few more lines down). Fixed by reducing the scope of try/except to the OpenKey statement only. https://codereview.chromium.org/20578004/diff/48001/chrome/test/mini_installe... File chrome/test/mini_installer/settings.py (left): https://codereview.chromium.org/20578004/diff/48001/chrome/test/mini_installe... chrome/test/mini_installer/settings.py:1: #!/bin/bash On 2013/08/01 20:30:41, gab wrote: > You can upload with --similarity=99 to get rid of copy-detection (copy detection > is useful when you actually move a file and slightly modify it, but in your case > the default threshold is considering this a copy of some random file which makes > the diff kind of weird!). > > git cl upload --similarity=99 (I'm not sure 100 works, but it probably does... > anyways 99 should be sufficient!) Thanks. I will do it next time. https://codereview.chromium.org/20578004/diff/48001/chrome/test/mini_installe... File chrome/test/mini_installer/verifier.py (right): https://codereview.chromium.org/20578004/diff/48001/chrome/test/mini_installe... chrome/test/mini_installer/verifier.py:7: On 2013/08/01 20:30:41, gab wrote: > nit: remove extra empty line Done. https://codereview.chromium.org/20578004/diff/48001/chrome/test/mini_installe... chrome/test/mini_installer/verifier.py:14: raise Exception("Unknown verifier") On 2013/08/01 20:30:41, gab wrote: > Add to TODO that this should use unittest framework instead of exceptions. Done.
lgtm w/ changes below :), exciting! https://codereview.chromium.org/20578004/diff/66001/chrome/test/mini_installe... File chrome/test/mini_installer/test_installer.py (right): https://codereview.chromium.org/20578004/diff/66001/chrome/test/mini_installe... chrome/test/mini_installer/test_installer.py:12: import optparse nit: Fix alphabetical order of imports https://codereview.chromium.org/20578004/diff/66001/chrome/test/mini_installe... chrome/test/mini_installer/test_installer.py:166: parser = optparse.OptionParser(usage, description="Test the installer.") Use parser.add_option, see https://code.google.com/p/chromium/codesearch#chromium/src/chrome/tools/build... for an example. https://codereview.chromium.org/20578004/diff/66001/chrome/test/mini_installe... chrome/test/mini_installer/test_installer.py:167: (options, args) = parser.parse_args() Do not even store |args|, see https://code.google.com/p/chromium/codesearch#chromium/src/chrome/tools/build...
LGTM once Gab's final comments are addressed
https://codereview.chromium.org/20578004/diff/66001/chrome/test/mini_installe... File chrome/test/mini_installer/test_installer.py (right): https://codereview.chromium.org/20578004/diff/66001/chrome/test/mini_installe... chrome/test/mini_installer/test_installer.py:12: import optparse On 2013/08/05 15:43:48, gab wrote: > nit: Fix alphabetical order of imports Done. https://codereview.chromium.org/20578004/diff/66001/chrome/test/mini_installe... chrome/test/mini_installer/test_installer.py:166: parser = optparse.OptionParser(usage, description="Test the installer.") On 2013/08/05 15:43:48, gab wrote: > Use parser.add_option, see > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/tools/build... > for an example. add_option doesn't support required arguments. The Python doc recommends using positional arguments for required arguments instead of adding options. http://docs.python.org/2/library/optparse.html#what-are-options-for
+mathp for python readability review :) -- (neither me, nor robert, have been python readbility...) Thanks Math! Gab https://codereview.chromium.org/20578004/diff/66001/chrome/test/mini_installe... File chrome/test/mini_installer/test_installer.py (right): https://codereview.chromium.org/20578004/diff/66001/chrome/test/mini_installe... chrome/test/mini_installer/test_installer.py:166: parser = optparse.OptionParser(usage, description="Test the installer.") On 2013/08/05 18:03:51, sukolsak wrote: > On 2013/08/05 15:43:48, gab wrote: > > Use parser.add_option, see > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/tools/build... > > for an example. > > add_option doesn't support required arguments. The Python doc recommends using > positional arguments for required arguments instead of adding options. > http://docs.python.org/2/library/optparse.html#what-are-options-for I see then the error message below should be a more verbose help message detailing what the expected syntax is (does optparse support some kind of generic help message; e.g., either triggered via -h/--help or forced when incoming command-line is incorrect?).
On 2013/08/05 18:13:05, gab wrote: > +mathp for python readability review :) -- (neither me, nor robert, have been > python readbility...) s/have been python readbility/have been python certified or s/have been python readbility/have python readability :) > > Thanks Math! > Gab > > https://codereview.chromium.org/20578004/diff/66001/chrome/test/mini_installe... > File chrome/test/mini_installer/test_installer.py (right): > > https://codereview.chromium.org/20578004/diff/66001/chrome/test/mini_installe... > chrome/test/mini_installer/test_installer.py:166: parser = > optparse.OptionParser(usage, description="Test the installer.") > On 2013/08/05 18:03:51, sukolsak wrote: > > On 2013/08/05 15:43:48, gab wrote: > > > Use parser.add_option, see > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/tools/build... > > > for an example. > > > > add_option doesn't support required arguments. The Python doc recommends using > > positional arguments for required arguments instead of adding options. > > http://docs.python.org/2/library/optparse.html#what-are-options-for > > I see then the error message below should be a more verbose help message > detailing what the expected syntax is (does optparse support some kind of > generic help message; e.g., either triggered via -h/--help or forced when > incoming command-line is incorrect?).
Hi, A few comments about how this can be simplified. Have a look. https://codereview.chromium.org/20578004/diff/79001/chrome/test/mini_installe... File chrome/test/mini_installer/registry_verifier.py (right): https://codereview.chromium.org/20578004/diff/79001/chrome/test/mini_installe... chrome/test/mini_installer/registry_verifier.py:10: def VerifyRegistryEntries(entries): You should have a docstring to document public functions. A one-liner is sufficient if the function is simple (obvious param, no return value), but if it's more complicated, you should describe the params, their type, and the returned value. Look for Python Style Guide, internally. Also, try pylint <path/to/file...> https://codereview.chromium.org/20578004/diff/79001/chrome/test/mini_installe... chrome/test/mini_installer/registry_verifier.py:28: # TODO(sukolsak: Use unittest framework instead of exceptions. nit: missing closing parens https://codereview.chromium.org/20578004/diff/79001/chrome/test/mini_installe... chrome/test/mini_installer/registry_verifier.py:33: expected = entry["expected"] as mentioned above, for non-obvious params such as |entry|, a description is encouraged. https://codereview.chromium.org/20578004/diff/79001/chrome/test/mini_installe... File chrome/test/mini_installer/test_installer.py (right): https://codereview.chromium.org/20578004/diff/79001/chrome/test/mini_installe... chrome/test/mini_installer/test_installer.py:5: """This script tests the installer with a series of test cases specified in You're doing it right below. One-line description, then one blank line, and the more detailed description. https://codereview.chromium.org/20578004/diff/79001/chrome/test/mini_installe... chrome/test/mini_installer/test_installer.py:37: def MergeProperties(current_property, new_property): Googling quickly for merging dicts, I found: http://stackoverflow.com/questions/38987/how-can-i-merge-union-two-python-dic... Would it work here? https://codereview.chromium.org/20578004/diff/79001/chrome/test/mini_installe... chrome/test/mini_installer/test_installer.py:38: """Merges the new Property object into the current Property object nit: period. https://codereview.chromium.org/20578004/diff/79001/chrome/test/mini_installe... chrome/test/mini_installer/test_installer.py:38: """Merges the new Property object into the current Property object This makes it seem like Property is a class. How about "property dictionary"? If you agree, change throughout the file. https://codereview.chromium.org/20578004/diff/79001/chrome/test/mini_installe... chrome/test/mini_installer/test_installer.py:56: def ParseProperty(directory, property_filename): I don't think you need this function. See below. https://codereview.chromium.org/20578004/diff/79001/chrome/test/mini_installe... chrome/test/mini_installer/test_installer.py:72: def ParseProperties(directory, property_filenames): could you inline ParseProperty in ParseProperties? also note my rename of the function. And see above for whether or not you need a full MergeProperties function. def ParsePropertyFiles(directory, filenames): """... usual comment...""" current_property = {} for filename in filenames: path = os.path.join(directory, filename) new_property = json.load(open(path)) MergeProperties(current_property, new_property) return current_property https://codereview.chromium.org/20578004/diff/79001/chrome/test/mini_installe... chrome/test/mini_installer/test_installer.py:103: config_data = json.load(config_file) as before, I would just have config_data = json.load(open(config_file)) Also, you're not catching the exception if the file is not found or the JSON parsing fails (it happens a lot). https://codereview.chromium.org/20578004/diff/79001/chrome/test/mini_installe... chrome/test/mini_installer/test_installer.py:106: for state_name, state_property_filenames in config_data["states"]: single quotes, throughout all your files. https://codereview.chromium.org/20578004/diff/79001/chrome/test/mini_installe... chrome/test/mini_installer/test_installer.py:111: config.tests = config_data["tests"] could you bring this at the top after the creation of |config|?
https://codereview.chromium.org/20578004/diff/79001/chrome/test/mini_installe... File chrome/test/mini_installer/test_installer.py (right): https://codereview.chromium.org/20578004/diff/79001/chrome/test/mini_installe... chrome/test/mini_installer/test_installer.py:103: config_data = json.load(config_file) On 2013/08/05 18:46:13, Mathieu Perreault wrote: > as before, I would just have > config_data = json.load(open(config_file)) > > Also, you're not catching the exception if the file is not found or the JSON > parsing fails (it happens a lot). The idea was that the exception would bubble up the stack and make the test fail. Or is it bad enough that this would make the test flaky?
It should be fine in this case. I was saying it happens a lot, but I rather meant "make sure you know the cases where this can happen" On Aug 5, 2013 3:06 PM, <gab@chromium.org> wrote: > > https://codereview.chromium.**org/20578004/diff/79001/** > chrome/test/mini_installer/**test_installer.py<https://codereview.chromium.org/20578004/diff/79001/chrome/test/mini_installer/test_installer.py> > File chrome/test/mini_installer/**test_installer.py (right): > > https://codereview.chromium.**org/20578004/diff/79001/** > chrome/test/mini_installer/**test_installer.py#newcode103<https://codereview.chromium.org/20578004/diff/79001/chrome/test/mini_installer/test_installer.py#newcode103> > chrome/test/mini_installer/**test_installer.py:103: config_data = > json.load(config_file) > On 2013/08/05 18:46:13, Mathieu Perreault wrote: > >> as before, I would just have >> config_data = json.load(open(config_file)) >> > > Also, you're not catching the exception if the file is not found or >> > the JSON > >> parsing fails (it happens a lot). >> > > The idea was that the exception would bubble up the stack and make the > test fail. Or is it bad enough that this would make the test flaky? > > https://codereview.chromium.**org/20578004/<https://codereview.chromium.org/2... >
Thanks for the comments. https://codereview.chromium.org/20578004/diff/79001/chrome/test/mini_installe... File chrome/test/mini_installer/registry_verifier.py (right): https://codereview.chromium.org/20578004/diff/79001/chrome/test/mini_installe... chrome/test/mini_installer/registry_verifier.py:10: def VerifyRegistryEntries(entries): On 2013/08/05 18:46:13, Mathieu Perreault wrote: > You should have a docstring to document public functions. A one-liner is > sufficient if the function is simple (obvious param, no return value), but if > it's more complicated, you should describe the params, their type, and the > returned value. > > Look for Python Style Guide, internally. > > Also, try > > pylint <path/to/file...> Done. https://codereview.chromium.org/20578004/diff/79001/chrome/test/mini_installe... chrome/test/mini_installer/registry_verifier.py:28: # TODO(sukolsak: Use unittest framework instead of exceptions. On 2013/08/05 18:46:13, Mathieu Perreault wrote: > nit: missing closing parens Done. https://codereview.chromium.org/20578004/diff/79001/chrome/test/mini_installe... chrome/test/mini_installer/registry_verifier.py:33: expected = entry["expected"] On 2013/08/05 18:46:13, Mathieu Perreault wrote: > as mentioned above, for non-obvious params such as |entry|, a description is > encouraged. Done. https://codereview.chromium.org/20578004/diff/79001/chrome/test/mini_installe... File chrome/test/mini_installer/test_installer.py (right): https://codereview.chromium.org/20578004/diff/79001/chrome/test/mini_installe... chrome/test/mini_installer/test_installer.py:37: def MergeProperties(current_property, new_property): On 2013/08/05 18:46:13, Mathieu Perreault wrote: > Googling quickly for merging dicts, I found: > http://stackoverflow.com/questions/38987/how-can-i-merge-union-two-python-dic... > > Would it work here? No, it wouldn't work here. We are doing it differently from the one on stackoverflow. The design doc explicitly specifies how to merge dictionaries. For example, the result from merging {a: 0, b: {x: 1, y: 2}} and {b: {z: 3}} should be {a: 0, b: {x: 1, y: 2, z: 3}}, not {a: 0, b: {z: 3}}. Anyway, I have added a comment clarifying that. https://codereview.chromium.org/20578004/diff/79001/chrome/test/mini_installe... chrome/test/mini_installer/test_installer.py:38: """Merges the new Property object into the current Property object On 2013/08/05 18:46:13, Mathieu Perreault wrote: > nit: period. Done. https://codereview.chromium.org/20578004/diff/79001/chrome/test/mini_installe... chrome/test/mini_installer/test_installer.py:38: """Merges the new Property object into the current Property object On 2013/08/05 18:46:13, Mathieu Perreault wrote: > This makes it seem like Property is a class. How about "property dictionary"? If > you agree, change throughout the file. Done. https://codereview.chromium.org/20578004/diff/79001/chrome/test/mini_installe... chrome/test/mini_installer/test_installer.py:56: def ParseProperty(directory, property_filename): On 2013/08/05 18:46:13, Mathieu Perreault wrote: > I don't think you need this function. See below. Done. https://codereview.chromium.org/20578004/diff/79001/chrome/test/mini_installe... chrome/test/mini_installer/test_installer.py:72: def ParseProperties(directory, property_filenames): On 2013/08/05 18:46:13, Mathieu Perreault wrote: > could you inline ParseProperty in ParseProperties? > > also note my rename of the function. And see above for whether or not you need a > full MergeProperties function. > > def ParsePropertyFiles(directory, filenames): > """... usual comment...""" > current_property = {} > for filename in filenames: > path = os.path.join(directory, filename) > new_property = json.load(open(path)) > MergeProperties(current_property, new_property) > return current_property Done. https://codereview.chromium.org/20578004/diff/79001/chrome/test/mini_installe... chrome/test/mini_installer/test_installer.py:106: for state_name, state_property_filenames in config_data["states"]: On 2013/08/05 18:46:13, Mathieu Perreault wrote: > single quotes, throughout all your files. Done. https://codereview.chromium.org/20578004/diff/79001/chrome/test/mini_installe... chrome/test/mini_installer/test_installer.py:111: config.tests = config_data["tests"] On 2013/08/05 18:46:13, Mathieu Perreault wrote: > could you bring this at the top after the creation of |config|? Done.
Thanks for addressing the comments! I've added a few nits, but overall lgtm. https://codereview.chromium.org/20578004/diff/79001/chrome/test/mini_installe... File chrome/test/mini_installer/test_installer.py (right): https://codereview.chromium.org/20578004/diff/79001/chrome/test/mini_installe... chrome/test/mini_installer/test_installer.py:37: def MergeProperties(current_property, new_property): On 2013/08/05 20:34:46, sukolsak wrote: > On 2013/08/05 18:46:13, Mathieu Perreault wrote: > > Googling quickly for merging dicts, I found: > > > http://stackoverflow.com/questions/38987/how-can-i-merge-union-two-python-dic... > > > > Would it work here? > > No, it wouldn't work here. We are doing it differently from the one on > stackoverflow. The design doc explicitly specifies how to merge dictionaries. > For example, the result from merging {a: 0, b: {x: 1, y: 2}} and {b: {z: 3}} > should be {a: 0, b: {x: 1, y: 2, z: 3}}, not {a: 0, b: {z: 3}}. Anyway, I have > added a comment clarifying that. Thanks for clarifying! https://codereview.chromium.org/20578004/diff/79001/chrome/test/mini_installe... chrome/test/mini_installer/test_installer.py:38: """Merges the new Property object into the current Property object On 2013/08/05 18:46:13, Mathieu Perreault wrote: > This makes it seem like Property is a class. How about "property dictionary"? If > you agree, change throughout the file. Please also change the references to "Property object" throughout the file to be property dictionary. https://codereview.chromium.org/20578004/diff/90001/chrome/test/mini_installe... File chrome/test/mini_installer/registry_verifier.py (right): https://codereview.chromium.org/20578004/diff/90001/chrome/test/mini_installe... chrome/test/mini_installer/registry_verifier.py:52: print 'doesn\'t exist...', optional nit: you can use double quotes when there is a single quote inside the string.
https://codereview.chromium.org/20578004/diff/79001/chrome/test/mini_installe... File chrome/test/mini_installer/test_installer.py (right): https://codereview.chromium.org/20578004/diff/79001/chrome/test/mini_installe... chrome/test/mini_installer/test_installer.py:38: """Merges the new Property object into the current Property object On 2013/08/05 21:03:26, Mathieu Perreault wrote: > On 2013/08/05 18:46:13, Mathieu Perreault wrote: > > This makes it seem like Property is a class. How about "property dictionary"? > If > > you agree, change throughout the file. > > Please also change the references to "Property object" throughout the file to be > property dictionary. Would changing references to "Property object" in the comments be enough, or should I change variable names as well, e.g. current_property to current_property_dictionary? (Personally, I don't think including data types in variable names is a good idea.) In the spec, we use Property objects to refer to dictionaries with certain keys for the sake of readability. This probably doesn't make sense in Python where objects and dictionaries are not the same thing. https://codereview.chromium.org/20578004/diff/90001/chrome/test/mini_installe... File chrome/test/mini_installer/registry_verifier.py (right): https://codereview.chromium.org/20578004/diff/90001/chrome/test/mini_installe... chrome/test/mini_installer/registry_verifier.py:52: print 'doesn\'t exist...', On 2013/08/05 21:03:26, Mathieu Perreault wrote: > optional nit: you can use double quotes when there is a single quote inside the > string. Done.
still lgtm https://codereview.chromium.org/20578004/diff/79001/chrome/test/mini_installe... File chrome/test/mini_installer/test_installer.py (right): https://codereview.chromium.org/20578004/diff/79001/chrome/test/mini_installe... chrome/test/mini_installer/test_installer.py:38: """Merges the new Property object into the current Property object On 2013/08/05 21:57:00, sukolsak wrote: > On 2013/08/05 21:03:26, Mathieu Perreault wrote: > > On 2013/08/05 18:46:13, Mathieu Perreault wrote: > > > This makes it seem like Property is a class. How about "property > dictionary"? > > If > > > you agree, change throughout the file. > > > > Please also change the references to "Property object" throughout the file to > be > > property dictionary. > > Would changing references to "Property object" in the comments be enough, or > should I change variable names as well, e.g. current_property to > current_property_dictionary? (Personally, I don't think including data types in > variable names is a good idea.) In the spec, we use Property objects to refer to > dictionaries with certain keys for the sake of readability. This probably > doesn't make sense in Python where objects and dictionaries are not the same > thing. Just the comments is fine. Generally, you want to keep the variable names short. You can also call it "property object", although like I said my personal preferences is dictionary. You can also ignore me if you're using this convention elsewhere.
Thanks Math (see ping below for 1 more question for you)! Gab https://codereview.chromium.org/20578004/diff/79001/chrome/test/mini_installe... File chrome/test/mini_installer/test_installer.py (right): https://codereview.chromium.org/20578004/diff/79001/chrome/test/mini_installe... chrome/test/mini_installer/test_installer.py:103: config_data = json.load(config_file) On 2013/08/05 19:06:20, gab wrote: > On 2013/08/05 18:46:13, Mathieu Perreault wrote: > > as before, I would just have > > config_data = json.load(open(config_file)) > > > > Also, you're not catching the exception if the file is not found or the JSON > > parsing fails (it happens a lot). > > The idea was that the exception would bubble up the stack and make the test > fail. Or is it bad enough that this would make the test flaky? ping mathp https://codereview.chromium.org/20578004/diff/115001/chrome/test/mini_install... File chrome/test/mini_installer/test_installer.py (right): https://codereview.chromium.org/20578004/diff/115001/chrome/test/mini_install... chrome/test/mini_installer/test_installer.py:42: override earlier values in the second level. Link to http://goo.gl/uE0RoR for details here. (hopefully this is not a bad sign that I got the most awesome goo.gl link ever "you error" :)!) https://codereview.chromium.org/20578004/diff/115001/chrome/test/mini_install... chrome/test/mini_installer/test_installer.py:154: parser.add_argument('config_filename', help='the config file') optional nit: From ParseConfigFile() I gather that this can be a relative path; should we mention this here? i.e. help='The relative/absolute path to the config file.' ?
lgtm :) https://codereview.chromium.org/20578004/diff/79001/chrome/test/mini_installe... File chrome/test/mini_installer/test_installer.py (right): https://codereview.chromium.org/20578004/diff/79001/chrome/test/mini_installe... chrome/test/mini_installer/test_installer.py:103: config_data = json.load(config_file) On 2013/08/06 14:12:56, gab wrote: > On 2013/08/05 19:06:20, gab wrote: > > On 2013/08/05 18:46:13, Mathieu Perreault wrote: > > > as before, I would just have > > > config_data = json.load(open(config_file)) > > > > > > Also, you're not catching the exception if the file is not found or the JSON > > > parsing fails (it happens a lot). > > > > The idea was that the exception would bubble up the stack and make the test > > fail. Or is it bad enough that this would make the test flaky? > > ping mathp It's fine. I was just mentioning this to make sure you cover all the cases gracefully. In the context of a test, you don't want to silently fail so your thinking is perfect.
https://codereview.chromium.org/20578004/diff/115001/chrome/test/mini_install... File chrome/test/mini_installer/test_installer.py (right): https://codereview.chromium.org/20578004/diff/115001/chrome/test/mini_install... chrome/test/mini_installer/test_installer.py:42: override earlier values in the second level. On 2013/08/06 14:12:56, gab wrote: > Link to http://goo.gl/uE0RoR for details here. > > (hopefully this is not a bad sign that I got the most awesome goo.gl link ever > "you error" :)!) Done. https://codereview.chromium.org/20578004/diff/115001/chrome/test/mini_install... chrome/test/mini_installer/test_installer.py:154: parser.add_argument('config_filename', help='the config file') On 2013/08/06 14:12:56, gab wrote: > optional nit: From ParseConfigFile() I gather that this can be a relative path; > should we mention this here? > > i.e. help='The relative/absolute path to the config file.' > > ? Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sukolsak@chromium.org/20578004/101002
Retried try job too often on mac_rel for step(s) remoting_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
On 2013/08/06 16:38:58, I haz the power (commit-bot) wrote: > Retried try job too often on mac_rel for step(s) remoting_unittests > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu... FYI, this commit has no chance of breaking any existing test (and doesn't yet introduce new ones). Thus, the only test that matters is the presubmit check (that enforces some style and files rules and should already have passed when you uploaded unless you explicitly disabled it on upload). In this case you can add NOTRY=True on the line above BUG=... in the CL description for the CQ to know it can only do the presubmit check and commit when the tree opens (http://build.chromium.org) without running all the other tests. Cheers! Gab
On 2013/08/06 17:25:57, gab wrote: > On 2013/08/06 16:38:58, I haz the power (commit-bot) wrote: > > Retried try job too often on mac_rel for step(s) remoting_unittests > > > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu... This also makes me think, we should put all of this code in a win/ directory; perhaps chrome/test/win/mini_installer? I think the CQ is smart enough to only tests relevant platforms if it 100% sure the the files touched can only affect a single platform (i.e., all under win/ or *_win.cc), this will help later commits by only requiring Windows tests and avoiding irrelevant flakes on other platforms. Robert, WDYT?
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sukolsak@chromium.org/20578004/141001
Message was sent while issue was closed.
Change committed as 215940 |
