Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2831020:
A qa tool for checking that the browser stays open after installing extensions (Closed)
|
Created:
10 years, 6 months ago by alyssad Modified:
9 years, 7 months ago Reviewers:
Nirnimesh CC:
chromium-reviews, Aaron Boodman, Erik does not do reviews, pam+watch_chromium.org, Paweł Hajdan Jr. Base URL:
http://src.chromium.org/svn/trunk/src/ Visibility:
Public. |
DescriptionA qa-tool to check that the browser does not crash after extensions are installed.
Patch Set 1 : '' #Patch Set 2 : '' #
Total comments: 20
Patch Set 3 : '' #
Total comments: 4
Patch Set 4 : '' #
Total comments: 14
Patch Set 5 : '' #Patch Set 6 : '' #Messages
Total messages: 9 (0 generated)
http://codereview.chromium.org/2831020/diff/5001/6001 File chrome/test/qa_tools/extensions.py (right): http://codereview.chromium.org/2831020/diff/5001/6001#newcode32 chrome/test/qa_tools/extensions.py:32: except ImportError: Now that I think of it, it might be fine to put this script along with the other test scripts in chrome/test/functional so that we can avoid this all this ugly import kludge. http://codereview.chromium.org/2831020/diff/5001/6001#newcode37 chrome/test/qa_tools/extensions.py:37: import logging All system imports should go together, followed by pyauto imports http://codereview.chromium.org/2831020/diff/5001/6001#newcode38 chrome/test/qa_tools/extensions.py:38: import string This is unused http://codereview.chromium.org/2831020/diff/5001/6001#newcode41 chrome/test/qa_tools/extensions.py:41: extensions_dir = "" # The directory which holds the list of extensions No global vars please. Put them inside your class as member vars http://codereview.chromium.org/2831020/diff/5001/6001#newcode47 chrome/test/qa_tools/extensions.py:47: def testExtensions(self): need a better name. it doesn't really test extensions. It merely checks for crashes. testExtensionCrashes? http://codereview.chromium.org/2831020/diff/5001/6001#newcode54 chrome/test/qa_tools/extensions.py:54: top_urls = open(urls_file).readlines() You might want to strip off the '\n' at the end of each url http://codereview.chromium.org/2831020/diff/5001/6001#newcode55 chrome/test/qa_tools/extensions.py:55: assert len(top_urls) >= num_urls_to_visit http://codereview.chromium.org/2831020/diff/5001/6001#newcode67 chrome/test/qa_tools/extensions.py:67: for url in top_urls[0:num_urls_to_visit]: '0' is redundant. top_urls[:num_urls_to_visit] works http://codereview.chromium.org/2831020/diff/5001/6001#newcode71 chrome/test/qa_tools/extensions.py:71: Need 2 blank lines http://codereview.chromium.org/2831020/diff/5001/6001#newcode74 chrome/test/qa_tools/extensions.py:74: extensions_dir = sys.argv[1] This arg mangling looks ugly. Let's hardcode the names above for now, and add a TODO to provide a way in pyauto to pass args to a test
http://codereview.chromium.org/2831020/diff/5001/6001 File chrome/test/qa_tools/extensions.py (right): http://codereview.chromium.org/2831020/diff/5001/6001#newcode32 chrome/test/qa_tools/extensions.py:32: except ImportError: On 2010/06/23 00:37:15, Nirnimesh wrote: > Now that I think of it, it might be fine to put this script along with the other > test scripts in chrome/test/functional so that we can avoid this all this ugly > import kludge. Done. http://codereview.chromium.org/2831020/diff/5001/6001#newcode37 chrome/test/qa_tools/extensions.py:37: import logging On 2010/06/23 00:37:15, Nirnimesh wrote: > All system imports should go together, followed by pyauto imports Done. http://codereview.chromium.org/2831020/diff/5001/6001#newcode38 chrome/test/qa_tools/extensions.py:38: import string On 2010/06/23 00:37:15, Nirnimesh wrote: > This is unused Done. http://codereview.chromium.org/2831020/diff/5001/6001#newcode41 chrome/test/qa_tools/extensions.py:41: extensions_dir = "" # The directory which holds the list of extensions On 2010/06/23 00:37:15, Nirnimesh wrote: > No global vars please. Put them inside your class as member vars Done. http://codereview.chromium.org/2831020/diff/5001/6001#newcode47 chrome/test/qa_tools/extensions.py:47: def testExtensions(self): On 2010/06/23 00:37:15, Nirnimesh wrote: > need a better name. it doesn't really test extensions. It merely checks for > crashes. testExtensionCrashes? Done. http://codereview.chromium.org/2831020/diff/5001/6001#newcode54 chrome/test/qa_tools/extensions.py:54: top_urls = open(urls_file).readlines() On 2010/06/23 00:37:15, Nirnimesh wrote: > You might want to strip off the '\n' at the end of each url Done. http://codereview.chromium.org/2831020/diff/5001/6001#newcode55 chrome/test/qa_tools/extensions.py:55: On 2010/06/23 00:37:15, Nirnimesh wrote: > assert len(top_urls) >= num_urls_to_visit Do I want to assert this? I guess I assumed the user could provide a file with any number of urls. If <= 100 urls are given, then all of them will be visited. Otherwise, only the first 100 will be visited. Is this behavior okay? http://codereview.chromium.org/2831020/diff/5001/6001#newcode67 chrome/test/qa_tools/extensions.py:67: for url in top_urls[0:num_urls_to_visit]: On 2010/06/23 00:37:15, Nirnimesh wrote: > '0' is redundant. top_urls[:num_urls_to_visit] works Done. http://codereview.chromium.org/2831020/diff/5001/6001#newcode71 chrome/test/qa_tools/extensions.py:71: On 2010/06/23 00:37:15, Nirnimesh wrote: > Need 2 blank lines Done. http://codereview.chromium.org/2831020/diff/5001/6001#newcode74 chrome/test/qa_tools/extensions.py:74: extensions_dir = sys.argv[1] On 2010/06/23 00:37:15, Nirnimesh wrote: > This arg mangling looks ugly. Let's hardcode the names above for now, and add a > TODO to provide a way in pyauto to pass args to a test Done.
http://codereview.chromium.org/2831020/diff/9001/2002 File chrome/test/functional/extensions.py (right): http://codereview.chromium.org/2831020/diff/9001/2002#newcode28 chrome/test/functional/extensions.py:28: #TODO: provide a way in pyauto to pass args to a test and take these as args Need a space after '#' char http://codereview.chromium.org/2831020/diff/9001/2002#newcode29 chrome/test/functional/extensions.py:29: extensions_dir = "extensions" # The directory of extensions prefer single quotes.
This python script is not run by the try-bots. You can ignore the try pass.
http://codereview.chromium.org/2831020/diff/9001/2002 File chrome/test/functional/extensions.py (right): http://codereview.chromium.org/2831020/diff/9001/2002#newcode28 chrome/test/functional/extensions.py:28: #TODO: provide a way in pyauto to pass args to a test and take these as args On 2010/06/23 17:02:41, Nirnimesh wrote: > Need a space after '#' char Done. http://codereview.chromium.org/2831020/diff/9001/2002#newcode29 chrome/test/functional/extensions.py:29: extensions_dir = "extensions" # The directory of extensions On 2010/06/23 17:02:41, Nirnimesh wrote: > prefer single quotes. Done.
http://codereview.chromium.org/2831020/diff/14001/12002 File chrome/test/functional/extensions.py (right): http://codereview.chromium.org/2831020/diff/14001/12002#newcode10 chrome/test/functional/extensions.py:10: Usage: python extensions.py You use logging.debug below. With this command, you won't be able to list out anything. You probably need: python extensions.py -v http://codereview.chromium.org/2831020/diff/14001/12002#newcode23 chrome/test/functional/extensions.py:23: import pyautolib pyautolib is internal. Do not import it. Everything in pyautolib is exposed to pyauto. so pyauto.FilePath should work below. http://codereview.chromium.org/2831020/diff/14001/12002#newcode29 chrome/test/functional/extensions.py:29: extensions_dir = 'extensions' # The directory of extensions member vars should be suffixed with '_' http://codereview.chromium.org/2831020/diff/14001/12002#newcode30 chrome/test/functional/extensions.py:30: urls_file = 'urls.txt' # The file which holds a list of urls to visit You should probably gracefully assert somewhere that 'extensions' and 'urls.txt' exist. http://codereview.chromium.org/2831020/diff/14001/12002#newcode37 chrome/test/functional/extensions.py:37: extensions = [os.path.join(os.path.abspath(self.extensions_dir), file) There could be other files in that directory. (Ex: mac often creates .DS_Store file in any folder which you open with Finder even once). We need only .crx files. How about glob.glob(os.path.join(self.extensions_dir, '*.crx')) http://codereview.chromium.org/2831020/diff/14001/12002#newcode49 chrome/test/functional/extensions.py:49: self.InstallExtension(pyautolib.FilePath(extension), False) pyauto.FilePath http://codereview.chromium.org/2831020/diff/14001/12002#newcode54 chrome/test/functional/extensions.py:54: self.assertEqual(1, self.GetBrowserWindowCount()) 3rd argument (string) could be specified, which could contain all the extensions used in this batch. So, it'll be dumped when the assert fails
http://codereview.chromium.org/2831020/diff/14001/12002 File chrome/test/functional/extensions.py (right): http://codereview.chromium.org/2831020/diff/14001/12002#newcode10 chrome/test/functional/extensions.py:10: Usage: python extensions.py On 2010/06/23 18:01:29, Nirnimesh wrote: > You use logging.debug below. With this command, you won't be able to list out > anything. You probably need: python extensions.py -v Done. http://codereview.chromium.org/2831020/diff/14001/12002#newcode23 chrome/test/functional/extensions.py:23: import pyautolib On 2010/06/23 18:01:29, Nirnimesh wrote: > pyautolib is internal. Do not import it. Everything in pyautolib is exposed to > pyauto. so pyauto.FilePath should work below. Done. http://codereview.chromium.org/2831020/diff/14001/12002#newcode29 chrome/test/functional/extensions.py:29: extensions_dir = 'extensions' # The directory of extensions On 2010/06/23 18:01:29, Nirnimesh wrote: > member vars should be suffixed with '_' Done. http://codereview.chromium.org/2831020/diff/14001/12002#newcode30 chrome/test/functional/extensions.py:30: urls_file = 'urls.txt' # The file which holds a list of urls to visit On 2010/06/23 18:01:29, Nirnimesh wrote: > You should probably gracefully assert somewhere that 'extensions' and 'urls.txt' > exist. Done. http://codereview.chromium.org/2831020/diff/14001/12002#newcode37 chrome/test/functional/extensions.py:37: extensions = [os.path.join(os.path.abspath(self.extensions_dir), file) On 2010/06/23 18:01:29, Nirnimesh wrote: > There could be other files in that directory. (Ex: mac often creates .DS_Store > file in any folder which you open with Finder even once). We need only .crx > files. > > How about glob.glob(os.path.join(self.extensions_dir, '*.crx')) Done. Much cleaner. http://codereview.chromium.org/2831020/diff/14001/12002#newcode49 chrome/test/functional/extensions.py:49: self.InstallExtension(pyautolib.FilePath(extension), False) On 2010/06/23 18:01:29, Nirnimesh wrote: > pyauto.FilePath Done. http://codereview.chromium.org/2831020/diff/14001/12002#newcode54 chrome/test/functional/extensions.py:54: self.assertEqual(1, self.GetBrowserWindowCount()) On 2010/06/23 18:01:29, Nirnimesh wrote: > 3rd argument (string) could be specified, which could contain all the extensions > used in this batch. So, it'll be dumped when the assert fails Done.
LGTM. I'll check this in. |
