Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23523045:
Clean the machine before running commands in the mini_installer test framework. (Closed)
|
Created:
7 years, 3 months ago by sukolsak Modified:
7 years, 3 months ago CC:
chromium-reviews Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionClean the machine before running commands in the mini_installer test framework.
Clean the machine by uninstalling Chrome (if it's installed) for now. This allows the test slave to work properly. Eventually, we should read the clean state from the config file and clean the machine according to it.
NOTRY=True
BUG=264859
TEST=
1) Install Chrome at user level (if it's not installed already.)
2) Build Chrome with Release mode and make sure that mini_installer.exe is created.
3) Go to src\chrome\test\mini_installer
4) Run "python test_installer.py config\config.config --build-dir=<build-dir> --target=Release" where <build-dir> is the path to main build directory (the parent of the Release directory). The test should pass.
5) Repeat steps 1-4, but install Chrome at system level instead. The test should still pass.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=223458
Patch Set 1 #
Total comments: 4
Patch Set 2 : Address robertshield's comment. #
Total comments: 8
Patch Set 3 : Address gab and grt's comments. #
Total comments: 13
Patch Set 4 : Address gab's comments. #
Total comments: 10
Patch Set 5 : Address gab's comments. #
Total comments: 12
Patch Set 6 : Address gab's comments. #
Total comments: 2
Patch Set 7 : Address gab's comment. #
Total comments: 2
Patch Set 8 : Address mathp's comment. #
Messages
Total messages: 23 (0 generated)
Could you please take a look at this patch?
lgtm https://codereview.chromium.org/23523045/diff/1/chrome/test/mini_installer/te... File chrome/test/mini_installer/test_installer.py (right): https://codereview.chromium.org/23523045/diff/1/chrome/test/mini_installer/te... chrome/test/mini_installer/test_installer.py:124: # the machine according to it. Also, consider adding a TODO to handle SxS installs (what canary does). https://codereview.chromium.org/23523045/diff/1/chrome/test/mini_installer/te... chrome/test/mini_installer/test_installer.py:126: '--chrome-long-name="$CHROME_LONG_NAME" & ' Just to double check: it is my understanding that if either of these fail, then they will print an informative error message to stdout or stderr. Is that correct?
Thanks. https://codereview.chromium.org/23523045/diff/1/chrome/test/mini_installer/te... File chrome/test/mini_installer/test_installer.py (right): https://codereview.chromium.org/23523045/diff/1/chrome/test/mini_installer/te... chrome/test/mini_installer/test_installer.py:124: # the machine according to it. On 2013/09/10 20:59:31, robertshield wrote: > Also, consider adding a TODO to handle SxS installs (what canary does). Done. https://codereview.chromium.org/23523045/diff/1/chrome/test/mini_installer/te... chrome/test/mini_installer/test_installer.py:126: '--chrome-long-name="$CHROME_LONG_NAME" & ' On 2013/09/10 20:59:31, robertshield wrote: > Just to double check: it is my understanding that if either of these fail, then > they will print an informative error message to stdout or stderr. Is that > correct? They will print an error message to stderr. For example, if Chrome is not installed, they will complain that they can't find the registry key for the uninstall command. The error message is not really useful because our goal is to remove everything. We just give the uninstaller a chance to do a preliminary cleanup before we do it ourselves.
https://codereview.chromium.org/23523045/diff/5001/chrome/test/mini_installer... File chrome/test/mini_installer/test_installer.py (right): https://codereview.chromium.org/23523045/diff/5001/chrome/test/mini_installer... chrome/test/mini_installer/test_installer.py:72: self._RunCleanCommand() This shouldn't always be run by default imo... as I remember grt saying, if you have Chrome installed on your machine and you run this script unknowingly it shouldn't delete your Chrome from your system. It's ok for it to warn you though (popup or stderr). It's also ok to have a flag that would force cleaning without prompting (and the bots would always use that flag); but, again, it's not okay to risk deleting Chrome from the machines of any random dev trying this script. https://codereview.chromium.org/23523045/diff/5001/chrome/test/mini_installer... chrome/test/mini_installer/test_installer.py:131: script_dir = os.path.dirname(os.path.abspath(__file__)) Since we now need |script_dir| in more than one place, how about making it a member of InstallerTest? https://codereview.chromium.org/23523045/diff/5001/chrome/test/mini_installer... chrome/test/mini_installer/test_installer.py:133: stderr=open(os.devnull, 'w')) So we trash all stderr output :(... why? If it's only to avoid the case where there is no install present, perhaps uninstall_chrome.py could have an option to silently return if no install is found instead?
i find that the path resolver is mis-named since it replaces variables in any kind of string rather than just in paths (case in point: this CL uses it to substitute an argument to a command line switch). in a separate CL, please rename it to something that doesn't include "path" in the name. maybe ExpandVariables or something like that would be good. thanks. https://codereview.chromium.org/23523045/diff/5001/chrome/test/mini_installer... File chrome/test/mini_installer/test_installer.py (right): https://codereview.chromium.org/23523045/diff/5001/chrome/test/mini_installer... chrome/test/mini_installer/test_installer.py:72: self._RunCleanCommand() On 2013/09/10 22:19:05, gab wrote: > This shouldn't always be run by default imo... as I remember grt saying, if you > have Chrome installed on your machine and you run this script unknowingly it > shouldn't delete your Chrome from your system. > > It's ok for it to warn you though (popup or stderr). > > It's also ok to have a flag that would force cleaning without prompting (and the > bots would always use that flag); but, again, it's not okay to risk deleting > Chrome from the machines of any random dev trying this script. I like the idea of: 1) having the test fail if it finds that the machine isn't in the proper state 2) having a command line argument that puts the machine in the proper state if needed 3) having the bot configured to always use the above command line argument grt would be sad if he ran this and it nuked his chromes. :-)
Thank you for your review. On 2013/09/11 03:01:40, grt wrote: > i find that the path resolver is mis-named since it replaces variables in any > kind of string rather than just in paths (case in point: this CL uses it to > substitute an argument to a command line switch). in a separate CL, please > rename it to something that doesn't include "path" in the name. maybe > ExpandVariables or something like that would be good. thanks. I will do. https://codereview.chromium.org/23523045/diff/5001/chrome/test/mini_installer... File chrome/test/mini_installer/test_installer.py (right): https://codereview.chromium.org/23523045/diff/5001/chrome/test/mini_installer... chrome/test/mini_installer/test_installer.py:72: self._RunCleanCommand() On 2013/09/10 22:19:05, gab wrote: > This shouldn't always be run by default imo... as I remember grt saying, if you > have Chrome installed on your machine and you run this script unknowingly it > shouldn't delete your Chrome from your system. > > It's ok for it to warn you though (popup or stderr). > > It's also ok to have a flag that would force cleaning without prompting (and the > bots would always use that flag); but, again, it's not okay to risk deleting > Chrome from the machines of any random dev trying this script. Done. https://codereview.chromium.org/23523045/diff/5001/chrome/test/mini_installer... chrome/test/mini_installer/test_installer.py:72: self._RunCleanCommand() On 2013/09/11 03:01:41, grt wrote: > On 2013/09/10 22:19:05, gab wrote: > > This shouldn't always be run by default imo... as I remember grt saying, if > you > > have Chrome installed on your machine and you run this script unknowingly it > > shouldn't delete your Chrome from your system. > > > > It's ok for it to warn you though (popup or stderr). > > > > It's also ok to have a flag that would force cleaning without prompting (and > the > > bots would always use that flag); but, again, it's not okay to risk deleting > > Chrome from the machines of any random dev trying this script. > > I like the idea of: > 1) having the test fail if it finds that the machine isn't in the proper state > 2) having a command line argument that puts the machine in the proper state if > needed > 3) having the bot configured to always use the above command line argument > > grt would be sad if he ran this and it nuked his chromes. :-) Done. Checking whether Chrome is installed (and is installed properly) before deciding whether to have the test fail is kind of tricky. So, I choose to always warn the user that the script will remove Chrome, whether Chrome is currently installed or not, and ask for confirmation. https://codereview.chromium.org/23523045/diff/5001/chrome/test/mini_installer... chrome/test/mini_installer/test_installer.py:131: script_dir = os.path.dirname(os.path.abspath(__file__)) On 2013/09/10 22:19:05, gab wrote: > Since we now need |script_dir| in more than one place, how about making it a > member of InstallerTest? |script_dir| doesn't seem like a state of an object to me. And we use it only twice. What do you think? https://codereview.chromium.org/23523045/diff/5001/chrome/test/mini_installer... chrome/test/mini_installer/test_installer.py:133: stderr=open(os.devnull, 'w')) On 2013/09/10 22:19:05, gab wrote: > So we trash all stderr output :(... why? If it's only to avoid the case where > there is no install present, perhaps uninstall_chrome.py could have an option to > silently return if no install is found instead? Done.
Suggestions in comments below. Also, just to be clear: this cleaning is insufficient in all cases where chrome has been uninstalled, but left pieces behind... I really think we need to be reading what the clean state is and delete everything on the system to get to a clean state. I do think that it can be worth it to try to first run the uninstaller if Chrome is still fully installed on the system (e.g. the test aborted early), but a second pass to cleanup leftover debris will also be required imo. For that reason I'm ok with doing this as a first pass, but please keep in mind that the second part is very much needed. I'm also ok with not prompting the user to remove debris from the system (i.e., once we know there is no Chrome on the system (e.g., we removed it after user approval or there was none already)); it's ok to silently remove debris left by previous Chromes on that user's machine imo). Cheers! Gab https://codereview.chromium.org/23523045/diff/9001/chrome/test/mini_installer... File chrome/test/mini_installer/test_installer.py (right): https://codereview.chromium.org/23523045/diff/9001/chrome/test/mini_installer... chrome/test/mini_installer/test_installer.py:235: prompt = ('Warning: This script will uninstall Chrome or Chromium if they ' Instead of always prompting how about an option on uninstall_chrome.py to prompt if it does find an install? e.g., --interactive which would only be passed to uninstall_chrome.py if !force_clean ? https://codereview.chromium.org/23523045/diff/9001/chrome/test/mini_installer... File chrome/test/mini_installer/uninstall_chrome.py (right): https://codereview.chromium.org/23523045/diff/9001/chrome/test/mini_installer... chrome/test/mini_installer/uninstall_chrome.py:23: parser.add_option('--silent', action='store_true', dest='silent', How about: --no-error-if-absent or something more verbose like that? To avoid confusion between this and --interactive option proposed above. https://codereview.chromium.org/23523045/diff/9001/chrome/test/mini_installer... chrome/test/mini_installer/uninstall_chrome.py:41: return 1 I'd say we should return 0 here.
On 2013/09/13 15:06:19, gab wrote: > Suggestions in comments below. > > Also, just to be clear: this cleaning is insufficient in all cases where chrome > has been uninstalled, but left pieces behind... > > I really think we need to be reading what the clean state is and delete > everything on the system to get to a clean state. > > I do think that it can be worth it to try to first run the uninstaller if Chrome > is still fully installed on the system (e.g. the test aborted early), but a > second pass to cleanup leftover debris will also be required imo. > > For that reason I'm ok with doing this as a first pass, but please keep in mind > that the second part is very much needed. Yes, that's why I put a TODO in the code to "Read the clean state from the config file and clean the machine according to it." https://codereview.chromium.org/23523045/diff/9001/chrome/test/mini_installer... File chrome/test/mini_installer/test_installer.py (right): https://codereview.chromium.org/23523045/diff/9001/chrome/test/mini_installer... chrome/test/mini_installer/test_installer.py:235: prompt = ('Warning: This script will uninstall Chrome or Chromium if they ' On 2013/09/13 15:06:19, gab wrote: > Instead of always prompting how about an option on uninstall_chrome.py to prompt > if it does find an install? > > e.g., --interactive > > which would only be passed to uninstall_chrome.py if !force_clean > > ? Then we will only warn the user only when the registry key for uninstalling Chrome exists. What if the registry key is missing, but everything else (chrome.exe, browsing data, etc.) is still there? Are we really OK with removing them without warning the user? https://codereview.chromium.org/23523045/diff/9001/chrome/test/mini_installer... File chrome/test/mini_installer/uninstall_chrome.py (right): https://codereview.chromium.org/23523045/diff/9001/chrome/test/mini_installer... chrome/test/mini_installer/uninstall_chrome.py:23: parser.add_option('--silent', action='store_true', dest='silent', On 2013/09/13 15:06:19, gab wrote: > How about: --no-error-if-absent or something more verbose like that? > > To avoid confusion between this and --interactive option proposed above. Will do. https://codereview.chromium.org/23523045/diff/9001/chrome/test/mini_installer... chrome/test/mini_installer/uninstall_chrome.py:41: return 1 On 2013/09/13 15:06:19, gab wrote: > I'd say we should return 0 here. Will do.
lgtm from me. i defer to gab.
https://codereview.chromium.org/23523045/diff/9001/chrome/test/mini_installer... File chrome/test/mini_installer/test_installer.py (right): https://codereview.chromium.org/23523045/diff/9001/chrome/test/mini_installer... chrome/test/mini_installer/test_installer.py:235: prompt = ('Warning: This script will uninstall Chrome or Chromium if they ' On 2013/09/13 15:59:33, sukolsak wrote: > On 2013/09/13 15:06:19, gab wrote: > > Instead of always prompting how about an option on uninstall_chrome.py to > prompt > > if it does find an install? > > > > e.g., --interactive > > > > which would only be passed to uninstall_chrome.py if !force_clean > > > > ? > > Then we will only warn the user only when the registry key for uninstalling > Chrome exists. What if the registry key is missing, but everything else > (chrome.exe, browsing data, etc.) is still there? Are we really OK with removing > them without warning the user? I'd say yes, I think we shouldn't delete a real/full Chrome install on the user's machine. However if Chrome is not fully installed (i.e. not visible in Control Panel/Programs & Features -- which is what the UninstallString triggers among other things), I think it's fine to clean the machine for the user (as if we abort he may not even know how to clean his machine...). So the final design imo should be: 1) If there is a Chrome installed on the system, prompt the user to remove it (or do it automatically if --force-clean). 2) Unless there was a failure with the uninstall in step 1 (user denying the prompt is considered failure; the uninstall string not being present is not): clean up everything that's left behind. https://codereview.chromium.org/23523045/diff/9001/chrome/test/mini_installer... File chrome/test/mini_installer/uninstall_chrome.py (right): https://codereview.chromium.org/23523045/diff/9001/chrome/test/mini_installer... chrome/test/mini_installer/uninstall_chrome.py:23: parser.add_option('--silent', action='store_true', dest='silent', On 2013/09/13 15:59:33, sukolsak wrote: > On 2013/09/13 15:06:19, gab wrote: > > How about: --no-error-if-absent or something more verbose like that? > > > > To avoid confusion between this and --interactive option proposed above. > > Will do. I meant in this CL... https://codereview.chromium.org/23523045/diff/9001/chrome/test/mini_installer... chrome/test/mini_installer/uninstall_chrome.py:41: return 1 On 2013/09/13 15:59:33, sukolsak wrote: > On 2013/09/13 15:06:19, gab wrote: > > I'd say we should return 0 here. > > Will do. I meant in this CL...
https://codereview.chromium.org/23523045/diff/9001/chrome/test/mini_installer... File chrome/test/mini_installer/test_installer.py (right): https://codereview.chromium.org/23523045/diff/9001/chrome/test/mini_installer... chrome/test/mini_installer/test_installer.py:235: prompt = ('Warning: This script will uninstall Chrome or Chromium if they ' On 2013/09/13 20:12:55, gab wrote: > On 2013/09/13 15:59:33, sukolsak wrote: > > On 2013/09/13 15:06:19, gab wrote: > > > Instead of always prompting how about an option on uninstall_chrome.py to > > prompt > > > if it does find an install? > > > > > > e.g., --interactive > > > > > > which would only be passed to uninstall_chrome.py if !force_clean > > > > > > ? > > > > Then we will only warn the user only when the registry key for uninstalling > > Chrome exists. What if the registry key is missing, but everything else > > (chrome.exe, browsing data, etc.) is still there? Are we really OK with > removing > > them without warning the user? > > I'd say yes, I think we shouldn't delete a real/full Chrome install on the > user's machine. However if Chrome is not fully installed (i.e. not visible in > Control Panel/Programs & Features -- which is what the UninstallString triggers > among other things), I think it's fine to clean the machine for the user (as if > we abort he may not even know how to clean his machine...). > > So the final design imo should be: > > 1) If there is a Chrome installed on the system, prompt the user to remove it > (or do it automatically if --force-clean). > 2) Unless there was a failure with the uninstall in step 1 (user denying the > prompt is considered failure; the uninstall string not being present is not): > clean up everything that's left behind. Done. https://codereview.chromium.org/23523045/diff/9001/chrome/test/mini_installer... File chrome/test/mini_installer/uninstall_chrome.py (right): https://codereview.chromium.org/23523045/diff/9001/chrome/test/mini_installer... chrome/test/mini_installer/uninstall_chrome.py:23: parser.add_option('--silent', action='store_true', dest='silent', On 2013/09/13 20:12:55, gab wrote: > On 2013/09/13 15:59:33, sukolsak wrote: > > On 2013/09/13 15:06:19, gab wrote: > > > How about: --no-error-if-absent or something more verbose like that? > > > > > > To avoid confusion between this and --interactive option proposed above. > > > > Will do. > > I meant in this CL... Done. Yes, I was waiting for your reply. https://codereview.chromium.org/23523045/diff/9001/chrome/test/mini_installer... chrome/test/mini_installer/uninstall_chrome.py:41: return 1 On 2013/09/13 20:12:55, gab wrote: > On 2013/09/13 15:59:33, sukolsak wrote: > > On 2013/09/13 15:06:19, gab wrote: > > > I'd say we should return 0 here. > > > > Will do. > > I meant in this CL... > Done.
I like this a lot; last comments below. Cheers! Gab https://codereview.chromium.org/23523045/diff/9001/chrome/test/mini_installer... File chrome/test/mini_installer/uninstall_chrome.py (right): https://codereview.chromium.org/23523045/diff/9001/chrome/test/mini_installer... chrome/test/mini_installer/uninstall_chrome.py:23: parser.add_option('--silent', action='store_true', dest='silent', On 2013/09/13 23:11:14, sukolsak wrote: > On 2013/09/13 20:12:55, gab wrote: > > On 2013/09/13 15:59:33, sukolsak wrote: > > > On 2013/09/13 15:06:19, gab wrote: > > > > How about: --no-error-if-absent or something more verbose like that? > > > > > > > > To avoid confusion between this and --interactive option proposed above. > > > > > > Will do. > > > > I meant in this CL... > > Done. Yes, I was waiting for your reply. Ah ok, my bad :)! https://codereview.chromium.org/23523045/diff/25001/chrome/test/mini_installe... File chrome/test/mini_installer/test_installer.py (right): https://codereview.chromium.org/23523045/diff/25001/chrome/test/mini_installe... chrome/test/mini_installer/test_installer.py:87: RunCleanCommand(True) We should only need to do this if the test case failed (otherwise we should always be clean since the final state is the clean state). https://codereview.chromium.org/23523045/diff/25001/chrome/test/mini_installe... chrome/test/mini_installer/test_installer.py:141: for chrome_long_name in ['Google Chrome', 'Chromium']: I don't like having to delete both brands; imo we should only clean the brand we are testing. It is true however that this is required to have a fully clean state for a single shared registry value (App Paths for chrome.exe)... But in practice I think it's the TODO step that should clean the shared value (i.e., it's rule being: if the uninstall was successful, I shall clean everything remaining I know of -- this includes the shared registry value). Cleaning the shared registry value silently is not wrong imo; only one chrome.exe can own it, and thus any chrome.exe can't rely on it (all it does is it allows you to enter commands like "start chrome.exe" without providing a full path -- it's kind of like adding chrome.exe to %PATH%). Removing it even if the tested chrome doesn't own it isn't a big deal imo. ** tl;dr; Only clean the brand you are testing here.** (and get the long name from the PathResolver) https://codereview.chromium.org/23523045/diff/25001/chrome/test/mini_installe... File chrome/test/mini_installer/uninstall_chrome.py (right): https://codereview.chromium.org/23523045/diff/25001/chrome/test/mini_installe... chrome/test/mini_installer/uninstall_chrome.py:24: default=False, help='Ask before uninstalling Chrome') Add '.' at the end of help string. https://codereview.chromium.org/23523045/diff/25001/chrome/test/mini_installe... chrome/test/mini_installer/uninstall_chrome.py:28: 'is absent') Add '.' at the end of help string. https://codereview.chromium.org/23523045/diff/25001/chrome/test/mini_installe... chrome/test/mini_installer/uninstall_chrome.py:49: prompt = ('Warning: This will uninstall %s. Do you want to continue? (y/N) ' s/This will uninstall %s/This will uninstall %s at %s where the 2nd %s is: 'system-level' if options.system_level else 'user-level'
Thank you for your review. https://codereview.chromium.org/23523045/diff/25001/chrome/test/mini_installe... File chrome/test/mini_installer/test_installer.py (right): https://codereview.chromium.org/23523045/diff/25001/chrome/test/mini_installe... chrome/test/mini_installer/test_installer.py:87: RunCleanCommand(True) On 2013/09/16 13:59:58, gab wrote: > We should only need to do this if the test case failed (otherwise we should > always be clean since the final state is the clean state). Done. https://codereview.chromium.org/23523045/diff/25001/chrome/test/mini_installe... chrome/test/mini_installer/test_installer.py:141: for chrome_long_name in ['Google Chrome', 'Chromium']: On 2013/09/16 13:59:58, gab wrote: > I don't like having to delete both brands; imo we should only clean the brand we > are testing. It is true however that this is required to have a fully clean > state for a single shared registry value (App Paths for chrome.exe)... > > But in practice I think it's the TODO step that should clean the shared value > (i.e., it's rule being: if the uninstall was successful, I shall clean > everything remaining I know of -- this includes the shared registry value). > > Cleaning the shared registry value silently is not wrong imo; only one > chrome.exe can own it, and thus any chrome.exe can't rely on it (all it does is > it allows you to enter commands like "start chrome.exe" without providing a full > path -- it's kind of like adding chrome.exe to %PATH%). Removing it even if the > tested chrome doesn't own it isn't a big deal imo. > > > ** tl;dr; Only clean the brand you are testing here.** > (and get the long name from the PathResolver) Done. https://codereview.chromium.org/23523045/diff/25001/chrome/test/mini_installe... File chrome/test/mini_installer/uninstall_chrome.py (right): https://codereview.chromium.org/23523045/diff/25001/chrome/test/mini_installe... chrome/test/mini_installer/uninstall_chrome.py:24: default=False, help='Ask before uninstalling Chrome') On 2013/09/16 13:59:58, gab wrote: > Add '.' at the end of help string. Done. https://codereview.chromium.org/23523045/diff/25001/chrome/test/mini_installe... chrome/test/mini_installer/uninstall_chrome.py:28: 'is absent') On 2013/09/16 13:59:58, gab wrote: > Add '.' at the end of help string. Done. https://codereview.chromium.org/23523045/diff/25001/chrome/test/mini_installe... chrome/test/mini_installer/uninstall_chrome.py:49: prompt = ('Warning: This will uninstall %s. Do you want to continue? (y/N) ' On 2013/09/16 13:59:58, gab wrote: > s/This will uninstall %s/This will uninstall %s at %s > > where the 2nd %s is: > 'system-level' if options.system_level else 'user-level' Done.
Tweaks below, I'll take a look at the full diff again. https://codereview.chromium.org/23523045/diff/30001/chrome/test/mini_installe... File chrome/test/mini_installer/test_installer.py (right): https://codereview.chromium.org/23523045/diff/30001/chrome/test/mini_installe... chrome/test/mini_installer/test_installer.py:84: self._was_successful = True Add a comment stating why the test is successful if it made it here (i.e., RunCommand throws an exception on failure?). https://codereview.chromium.org/23523045/diff/30001/chrome/test/mini_installe... chrome/test/mini_installer/test_installer.py:87: """Cleans up the machine if the test case fails or raises an exception.""" Isn't "fail" and "raised an exception" the same thing? I'd cut this comment after "fails." https://codereview.chromium.org/23523045/diff/30001/chrome/test/mini_installe... File chrome/test/mini_installer/uninstall_chrome.py (right): https://codereview.chromium.org/23523045/diff/30001/chrome/test/mini_installe... chrome/test/mini_installer/uninstall_chrome.py:51: options.system_level else 'user-level')) nit: Fix ident (i.e. options.system_level is a continuation of the statement that starts with 'system_level' above; it should thus indented 4 spaces more than that (which doesn't fit) -- so you'll need to bring 'system_level'... down to this line
Looked at full-diff, looks fine, more comments below. https://codereview.chromium.org/23523045/diff/30001/chrome/test/mini_installe... File chrome/test/mini_installer/test_installer.py (right): https://codereview.chromium.org/23523045/diff/30001/chrome/test/mini_installe... chrome/test/mini_installer/test_installer.py:88: if not self._was_successful: Rename |_was_successful| to something like |clean_on_teardown| since that's its only purpose (and I think it's better that it not be interpreted too generally). https://codereview.chromium.org/23523045/diff/30001/chrome/test/mini_installe... chrome/test/mini_installer/test_installer.py:129: raise Exception('Command %s returned non-zero exit status %s' % ( Why go from self.fail to throwing an exception? Maybe we already talked about this, but I don't see a hint in the CL description to why this was changed.
https://codereview.chromium.org/23523045/diff/30001/chrome/test/mini_installe... File chrome/test/mini_installer/uninstall_chrome.py (right): https://codereview.chromium.org/23523045/diff/30001/chrome/test/mini_installe... chrome/test/mini_installer/uninstall_chrome.py:51: options.system_level else 'user-level')) On 2013/09/16 18:33:45, gab wrote: > nit: Fix ident (i.e. options.system_level is a continuation of the statement > that starts with 'system_level' above; it should thus indented 4 spaces more > than that (which doesn't fit) -- so you'll need to bring 'system_level'... down > to this line e.g., '(y/N) ' % (options.chrome_long_name, 'system-level' if options.system_level else 'user-level'))
https://codereview.chromium.org/23523045/diff/30001/chrome/test/mini_installe... File chrome/test/mini_installer/test_installer.py (right): https://codereview.chromium.org/23523045/diff/30001/chrome/test/mini_installe... chrome/test/mini_installer/test_installer.py:84: self._was_successful = True On 2013/09/16 18:33:45, gab wrote: > Add a comment stating why the test is successful if it made it here (i.e., > RunCommand throws an exception on failure?). Done. https://codereview.chromium.org/23523045/diff/30001/chrome/test/mini_installe... chrome/test/mini_installer/test_installer.py:87: """Cleans up the machine if the test case fails or raises an exception.""" On 2013/09/16 18:33:45, gab wrote: > Isn't "fail" and "raised an exception" the same thing? I'd cut this comment > after "fails." Done. Python unittest makes a distinction between failures and errors. It counts a test as failure if the test raises an AssertionError and as error if it raises any other exceptions. But this also makes sense if we regard anything that is not successful as failure. https://codereview.chromium.org/23523045/diff/30001/chrome/test/mini_installe... chrome/test/mini_installer/test_installer.py:88: if not self._was_successful: On 2013/09/16 18:41:22, gab wrote: > Rename |_was_successful| to something like |clean_on_teardown| since that's its > only purpose (and I think it's better that it not be interpreted too generally). Done. https://codereview.chromium.org/23523045/diff/30001/chrome/test/mini_installe... chrome/test/mini_installer/test_installer.py:129: raise Exception('Command %s returned non-zero exit status %s' % ( On 2013/09/16 18:41:22, gab wrote: > Why go from self.fail to throwing an exception? > > Maybe we already talked about this, but I don't see a hint in the CL description > to why this was changed. Because RunCommand is now a global function. I changed it so that RunCleanCommand could be called outside the InstallerTest class. What self.fail does is actually just throwing an Exception. https://codereview.chromium.org/23523045/diff/30001/chrome/test/mini_installe... File chrome/test/mini_installer/uninstall_chrome.py (right): https://codereview.chromium.org/23523045/diff/30001/chrome/test/mini_installe... chrome/test/mini_installer/uninstall_chrome.py:51: options.system_level else 'user-level')) On 2013/09/16 20:04:14, gab wrote: > On 2013/09/16 18:33:45, gab wrote: > > nit: Fix ident (i.e. options.system_level is a continuation of the statement > > that starts with 'system_level' above; it should thus indented 4 spaces more > > than that (which doesn't fit) -- so you'll need to bring 'system_level'... > down > > to this line > > e.g., > > '(y/N) ' % (options.chrome_long_name, > 'system-level' if > options.system_level else 'user-level')) Done.
lgtm, thanks! https://codereview.chromium.org/23523045/diff/30001/chrome/test/mini_installe... File chrome/test/mini_installer/test_installer.py (right): https://codereview.chromium.org/23523045/diff/30001/chrome/test/mini_installe... chrome/test/mini_installer/test_installer.py:129: raise Exception('Command %s returned non-zero exit status %s' % ( On 2013/09/16 22:02:02, sukolsak wrote: > On 2013/09/16 18:41:22, gab wrote: > > Why go from self.fail to throwing an exception? > > > > Maybe we already talked about this, but I don't see a hint in the CL > description > > to why this was changed. > > Because RunCommand is now a global function. I changed it so that > RunCleanCommand could be called outside the InstallerTest class. What self.fail > does is actually just throwing an Exception. Ah I see, man python is so subtle for that, unindent by 2 spaces and all of a sudden you have a global function...! Sorry I hadn't noticed that. https://codereview.chromium.org/23523045/diff/34001/chrome/test/mini_installe... File chrome/test/mini_installer/test_installer.py (right): https://codereview.chromium.org/23523045/diff/34001/chrome/test/mini_installe... chrome/test/mini_installer/test_installer.py:84: # If we make it here, it means that the test is successful, because If the test makes it here, it means it was successful... (i.e. try to avoid using "we", people frown upon it in Chromium...)
Thanks. https://codereview.chromium.org/23523045/diff/34001/chrome/test/mini_installe... File chrome/test/mini_installer/test_installer.py (right): https://codereview.chromium.org/23523045/diff/34001/chrome/test/mini_installe... chrome/test/mini_installer/test_installer.py:84: # If we make it here, it means that the test is successful, because On 2013/09/16 22:15:50, gab wrote: > If the test makes it here, it means it was successful... > > (i.e. try to avoid using "we", people frown upon it in Chromium...) Done.
lgtm https://codereview.chromium.org/23523045/diff/40001/chrome/test/mini_installe... File chrome/test/mini_installer/test_installer.py (right): https://codereview.chromium.org/23523045/diff/40001/chrome/test/mini_installe... chrome/test/mini_installer/test_installer.py:148: for level_option in ['', '--system-level']: very optional nit: tuples are more memory efficient than lists. Here and in general, consider using them when immutable suffices.
Thank you. https://codereview.chromium.org/23523045/diff/40001/chrome/test/mini_installe... File chrome/test/mini_installer/test_installer.py (right): https://codereview.chromium.org/23523045/diff/40001/chrome/test/mini_installe... chrome/test/mini_installer/test_installer.py:148: for level_option in ['', '--system-level']: On 2013/09/16 22:32:12, Mathieu Perreault wrote: > very optional nit: tuples are more memory efficient than lists. Here and in > general, consider using them when immutable suffices. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sukolsak@chromium.org/23523045/49001
Message was sent while issue was closed.
Change committed as 223458 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
