Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10834239:
Configuration file for Chrome perf and endure tests. (Closed)
|
Created:
8 years, 4 months ago by fdeng1 Modified:
8 years, 4 months ago CC:
chromium-reviews, dennis_jeffrey, anantha, dyu1, Nirnimesh Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionConfiguration file for Chrome perf and endure tests.
Make perf.py and perf_endure.py accept a configuration file in which
users can specify custom google account credentials and google account url.
They can also specify custom urls for google webapps(Gmail/Docs/Plus).
BUG=None
TEST=Ran perf_endure.py, perf.py with and without configuration file.
NOTRY=True
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=152224
Patch Set 1 : #
Total comments: 13
Patch Set 2 : Configuration work for both perf.py and perf_endure.py #
Total comments: 26
Patch Set 3 : Address comments #
Total comments: 34
Patch Set 4 : Address Dennis comments #
Total comments: 8
Patch Set 5 : Address Dennis Comments #Patch Set 6 : merge with original/master #
Total comments: 5
Messages
Total messages: 15 (0 generated)
Hi Dennis, Nirnimesh, Here is an idea of making chrome endure tests use credentials and alternative urls from a configuration file. Please take a look and let me know any thoughts. Thanks, Fang https://chromiumcodereview.appspot.com/10834239/diff/2001/chrome/test/functio... File chrome/test/functional/perf_endure.py (right): https://chromiumcodereview.appspot.com/10834239/diff/2001/chrome/test/functio... chrome/test/functional/perf_endure.py:735: """ Since we allow user to use their own google account. And their own account doesn't have the "latency" dom element. As such, gmail tests will hang to wait for that dom element to appear and then fail. We need some way to know whether latency dom element is available for the current account. We can let the user specify it in configuration file or we can choose to only record latency when it runs with a particular account(but concerns are that gmail team may also have some other accounts which have latency indicator). Other ideas?
https://chromiumcodereview.appspot.com/10834239/diff/2001/chrome/test/functio... File chrome/test/functional/endure_config.txt (right): https://chromiumcodereview.appspot.com/10834239/diff/2001/chrome/test/functio... chrome/test/functional/endure_config.txt:1: # Configuration file for Chrome Endure tests. eventually it would be nice to extend the use of this credentials file to the other perf tests in perf.py, since some of those also use the private tests info file to get login credentials. However, I think adding it to Chrome Endure is a good first step. https://chromiumcodereview.appspot.com/10834239/diff/2001/chrome/test/functio... chrome/test/functional/endure_config.txt:13: # "google_account_url": "https://my-google-account-server", when might the user need to customize this URL? Is it worthwhile making this URL customizable? https://chromiumcodereview.appspot.com/10834239/diff/2001/chrome/test/functio... File chrome/test/functional/perf_endure.py (right): https://chromiumcodereview.appspot.com/10834239/diff/2001/chrome/test/functio... chrome/test/functional/perf_endure.py:547: return {} if the file doesn't exist, let's return a dictionary here that is full with all default information. That way, the Gmail/Docs/Plus tests below don't need to check if the config information exists. They'll be guaranteed to always get config information. https://chromiumcodereview.appspot.com/10834239/diff/2001/chrome/test/functio... chrome/test/functional/perf_endure.py:566: """ if this function successfully finds credentials to use, we should log a message indicating whether we're using credentials from the config file or from the private credentials file. https://chromiumcodereview.appspot.com/10834239/diff/2001/chrome/test/functio... chrome/test/functional/perf_endure.py:574: private_file = os.path.join(pyauto.PyUITest.DataDir(), 'pyauto_private', can we just use the superclass's _LoginToGoogleAccount function here? We could do something like this: if the private_tests_info file exists: call the superclass _LoginToGoogleAccount else: raise a runtime error https://chromiumcodereview.appspot.com/10834239/diff/2001/chrome/test/functio... chrome/test/functional/perf_endure.py:735: """ On 2012/08/09 04:01:58, fdeng1 wrote: > Since we allow user to use their own google account. And their own account > doesn't have the "latency" dom element. As such, gmail tests will hang to wait > for that dom element to appear and then fail. We need some way to know whether > latency dom element is available for the current account. We can let the user > specify it in configuration file or we can choose to only record latency when it > runs with a particular account(but concerns are that gmail team may also have > some other accounts which have latency indicator). Other ideas? Good point. It would be good to have the code here dynamically check whether the latency is available on the webpage and only record it if so. The user should not have to worry about it. They'll just see the latency results if they're available, or else they won't see those results.
https://chromiumcodereview.appspot.com/10834239/diff/2001/chrome/test/functio... File chrome/test/functional/endure_config.txt (right): https://chromiumcodereview.appspot.com/10834239/diff/2001/chrome/test/functio... chrome/test/functional/endure_config.txt:1: # Configuration file for Chrome Endure tests. I tried to make the config file also effective for perf.py I renamed it to perf.cfg. I did it by modifying _LoginToGoogleAccount() in perf.py. Would you please let me know how you think about it? On 2012/08/09 17:29:04, dennis_jeffrey wrote: > eventually it would be nice to extend the use of this credentials file to the > other perf tests in perf.py, since some of those also use the private tests info > file to get login credentials. However, I think adding it to Chrome Endure is a > good first step. https://chromiumcodereview.appspot.com/10834239/diff/2001/chrome/test/functio... chrome/test/functional/endure_config.txt:13: # "google_account_url": "https://my-google-account-server", I remember Loreena said they have a different gmail server and authentication server. I am not sure about how they do authentications. We can ask her whether this is what they want on Monday. On 2012/08/09 17:29:04, dennis_jeffrey wrote: > when might the user need to customize this URL? Is it worthwhile making this > URL customizable? https://chromiumcodereview.appspot.com/10834239/diff/2001/chrome/test/functio... File chrome/test/functional/perf_endure.py (right): https://chromiumcodereview.appspot.com/10834239/diff/2001/chrome/test/functio... chrome/test/functional/perf_endure.py:547: return {} On 2012/08/09 17:29:04, dennis_jeffrey wrote: > if the file doesn't exist, let's return a dictionary here that is full with all > default information. That way, the Gmail/Docs/Plus tests below don't need to > check if the config information exists. They'll be guaranteed to always get > config information. Done. https://chromiumcodereview.appspot.com/10834239/diff/2001/chrome/test/functio... chrome/test/functional/perf_endure.py:566: """ Added a log message for each case. On 2012/08/09 17:29:04, dennis_jeffrey wrote: > if this function successfully finds credentials to use, we should log a message > indicating whether we're using credentials from the config file or from the > private credentials file. https://chromiumcodereview.appspot.com/10834239/diff/2001/chrome/test/functio... chrome/test/functional/perf_endure.py:574: private_file = os.path.join(pyauto.PyUITest.DataDir(), 'pyauto_private', I tried to make config file also effective for perf.py. So I put the code in the superclass's _LoginToGoogleAccount. On 2012/08/09 17:29:04, dennis_jeffrey wrote: > can we just use the superclass's _LoginToGoogleAccount function here? We could > do something like this: > > if the private_tests_info file exists: > call the superclass _LoginToGoogleAccount > else: > raise a runtime error https://chromiumcodereview.appspot.com/10834239/diff/2001/chrome/test/functio... chrome/test/functional/perf_endure.py:735: """ How would a test know the latency is available or not? Other than waiting for it to appear until it times out, is there another way? On 2012/08/09 17:29:04, dennis_jeffrey wrote: > On 2012/08/09 04:01:58, fdeng1 wrote: > > Since we allow user to use their own google account. And their own account > > doesn't have the "latency" dom element. As such, gmail tests will hang to wait > > for that dom element to appear and then fail. We need some way to know whether > > latency dom element is available for the current account. We can let the user > > specify it in configuration file or we can choose to only record latency when > it > > runs with a particular account(but concerns are that gmail team may also have > > some other accounts which have latency indicator). Other ideas? > > Good point. It would be good to have the code here dynamically check whether > the latency is available on the webpage and only record it if so. The user > should not have to worry about it. They'll just see the latency results if > they're available, or else they won't see those results. https://chromiumcodereview.appspot.com/10834239/diff/4002/chrome/test/functio... File chrome/test/functional/perf.cfg (right): https://chromiumcodereview.appspot.com/10834239/diff/4002/chrome/test/functio... chrome/test/functional/perf.cfg:14: # # Settings bellow are only effective for perf_endure.py Do we want to allow perf.py to use alternative gmail/plus/docs urls? https://chromiumcodereview.appspot.com/10834239/diff/4002/chrome/test/functio... File chrome/test/functional/perf.py (right): https://chromiumcodereview.appspot.com/10834239/diff/4002/chrome/test/functio... chrome/test/functional/perf.py:600: def _GetConfig(self): I just moved the changes to perf.py so that the credential configuration will work for perf.py. How do you think?
On Fri, Aug 10, 2012 at 5:11 PM, <fdeng@chromium.org> wrote: > > https://chromiumcodereview.**appspot.com/10834239/diff/** > 2001/chrome/test/functional/**endure_config.txt<https://chromiumcodereview.appspot.com/10834239/diff/2001/chrome/test/functional/endure_config.txt> > File chrome/test/functional/endure_**config.txt (right): > > https://chromiumcodereview.**appspot.com/10834239/diff/** > 2001/chrome/test/functional/**endure_config.txt#newcode1<https://chromiumcodereview.appspot.com/10834239/diff/2001/chrome/test/functional/endure_config.txt#newcode1> > chrome/test/functional/endure_**config.txt:1: # Configuration file for > Chrome Endure tests. > I tried to make the config file also effective for perf.py > I renamed it to perf.cfg. I did it by modifying _LoginToGoogleAccount() > in perf.py. Would you please let me know how you think about it? > > Looks good - now we can allow users to specify custom login credentials for the tests in perf.py that need to log into a google account. > > On 2012/08/09 17:29:04, dennis_jeffrey wrote: > >> eventually it would be nice to extend the use of this credentials file >> > to the > >> other perf tests in perf.py, since some of those also use the private >> > tests info > >> file to get login credentials. However, I think adding it to Chrome >> > Endure is a > >> good first step. >> > > https://chromiumcodereview.**appspot.com/10834239/diff/** > 2001/chrome/test/functional/**endure_config.txt#newcode13<https://chromiumcodereview.appspot.com/10834239/diff/2001/chrome/test/functional/endure_config.txt#newcode13> > chrome/test/functional/endure_**config.txt:13: # "google_account_url": > "https://my-google-account-**server <https://my-google-account-server>", > I remember Loreena said they have a different gmail server and > authentication server. I am not sure about how they do authentications. > We can ask her whether this is what they want on Monday. > > > On 2012/08/09 17:29:04, dennis_jeffrey wrote: > >> when might the user need to customize this URL? Is it worthwhile >> > making this > >> URL customizable? >> > > https://chromiumcodereview.**appspot.com/10834239/diff/** > 2001/chrome/test/functional/**perf_endure.py<https://chromiumcodereview.appspot.com/10834239/diff/2001/chrome/test/functional/perf_endure.py> > File chrome/test/functional/perf_**endure.py (right): > > https://chromiumcodereview.**appspot.com/10834239/diff/** > 2001/chrome/test/functional/**perf_endure.py#newcode547<https://chromiumcodereview.appspot.com/10834239/diff/2001/chrome/test/functional/perf_endure.py#newcode547> > chrome/test/functional/perf_**endure.py:547: return {} > On 2012/08/09 17:29:04, dennis_jeffrey wrote: > >> if the file doesn't exist, let's return a dictionary here that is full >> > with all > >> default information. That way, the Gmail/Docs/Plus tests below don't >> > need to > >> check if the config information exists. They'll be guaranteed to >> > always get > >> config information. >> > > Done. > > https://chromiumcodereview.**appspot.com/10834239/diff/** > 2001/chrome/test/functional/**perf_endure.py#newcode566<https://chromiumcodereview.appspot.com/10834239/diff/2001/chrome/test/functional/perf_endure.py#newcode566> > chrome/test/functional/perf_**endure.py:566: """ > Added a log message for each case. > > On 2012/08/09 17:29:04, dennis_jeffrey wrote: > >> if this function successfully finds credentials to use, we should log >> > a message > >> indicating whether we're using credentials from the config file or >> > from the > >> private credentials file. >> > > https://chromiumcodereview.**appspot.com/10834239/diff/** > 2001/chrome/test/functional/**perf_endure.py#newcode574<https://chromiumcodereview.appspot.com/10834239/diff/2001/chrome/test/functional/perf_endure.py#newcode574> > chrome/test/functional/perf_**endure.py:574: private_file = > os.path.join(pyauto.PyUITest.**DataDir(), 'pyauto_private', > I tried to make config file also effective for perf.py. > So I put the code in the superclass's _LoginToGoogleAccount. > > On 2012/08/09 17:29:04, dennis_jeffrey wrote: > >> can we just use the superclass's _LoginToGoogleAccount function here? >> > We could > >> do something like this: >> > > if the private_tests_info file exists: >> call the superclass _LoginToGoogleAccount >> else: >> raise a runtime error >> > > https://chromiumcodereview.**appspot.com/10834239/diff/** > 2001/chrome/test/functional/**perf_endure.py#newcode735<https://chromiumcodereview.appspot.com/10834239/diff/2001/chrome/test/functional/perf_endure.py#newcode735> > chrome/test/functional/perf_**endure.py:735: """ > How would a test know the latency is available or not? > Other than waiting for it to appear until it times out, is there another > way? We can check for its existence without waiting. I think by default in chromedriver, if you try to access an element that doesn't exist, an exception will be thrown saying that the element can't be found. We can just try to get a reference to the latency DOM element and -- if an exception is thrown -- then we know it doesn't exist for the current test account. > > > On 2012/08/09 17:29:04, dennis_jeffrey wrote: > >> On 2012/08/09 04:01:58, fdeng1 wrote: >> > Since we allow user to use their own google account. And their own >> > account > >> > doesn't have the "latency" dom element. As such, gmail tests will >> > hang to wait > >> > for that dom element to appear and then fail. We need some way to >> > know whether > >> > latency dom element is available for the current account. We can let >> > the user > >> > specify it in configuration file or we can choose to only record >> > latency when > >> it >> > runs with a particular account(but concerns are that gmail team may >> > also have > >> > some other accounts which have latency indicator). Other ideas? >> > > Good point. It would be good to have the code here dynamically check >> > whether > >> the latency is available on the webpage and only record it if so. The >> > user > >> should not have to worry about it. They'll just see the latency >> > results if > >> they're available, or else they won't see those results. >> > > https://chromiumcodereview.**appspot.com/10834239/diff/** > 4002/chrome/test/functional/**perf.cfg<https://chromiumcodereview.appspot.com/10834239/diff/4002/chrome/test/functional/perf.cfg> > File chrome/test/functional/perf.**cfg (right): > > https://chromiumcodereview.**appspot.com/10834239/diff/** > 4002/chrome/test/functional/**perf.cfg#newcode14<https://chromiumcodereview.appspot.com/10834239/diff/4002/chrome/test/functional/perf.cfg#newcode14> > chrome/test/functional/perf.**cfg:14: # # Settings bellow are only > effective for perf_endure.py > Do we want to allow perf.py to use alternative gmail/plus/docs urls? > > https://chromiumcodereview.**appspot.com/10834239/diff/** > 4002/chrome/test/functional/**perf.py<https://chromiumcodereview.appspot.com/10834239/diff/4002/chrome/test/functional/perf.py> > File chrome/test/functional/perf.py (right): > > https://chromiumcodereview.**appspot.com/10834239/diff/** > 4002/chrome/test/functional/**perf.py#newcode600<https://chromiumcodereview.appspot.com/10834239/diff/4002/chrome/test/functional/perf.py#newcode600> > chrome/test/functional/perf.**py:600: def _GetConfig(self): > I just moved the changes to perf.py so that the credential configuration > will work for perf.py. How do you think? > > https://chromiumcodereview.**appspot.com/10834239/<https://chromiumcodereview... >
Cool! Some more comments. https://chromiumcodereview.appspot.com/10834239/diff/4002/chrome/test/functio... File chrome/test/functional/perf.cfg (right): https://chromiumcodereview.appspot.com/10834239/diff/4002/chrome/test/functio... chrome/test/functional/perf.cfg:2: # This file is used to specify google account credential and credential --> credentials https://chromiumcodereview.appspot.com/10834239/diff/4002/chrome/test/functio... chrome/test/functional/perf.cfg:4: # the public urls of these services. It is formated as a python dictionary. formated --> formatted https://chromiumcodereview.appspot.com/10834239/diff/4002/chrome/test/functio... chrome/test/functional/perf.cfg:14: # # Settings bellow are only effective for perf_endure.py bellow --> below https://chromiumcodereview.appspot.com/10834239/diff/4002/chrome/test/functio... chrome/test/functional/perf.cfg:14: # # Settings bellow are only effective for perf_endure.py On 2012/08/11 00:11:27, fdeng1 wrote: > Do we want to allow perf.py to use alternative gmail/plus/docs urls? probably not. they should just be able to use customized login credentials. The custom webapp URLS are more important for Chrome Endure since other webapp teams will likely need to run those tests. https://chromiumcodereview.appspot.com/10834239/diff/4002/chrome/test/functio... chrome/test/functional/perf.cfg:22: # Empty configuration. Rather than having a comment above to show all the possible config options (and hoping the comment is kept up-to-date as the code changes), I think it's better to explicitly put all the config options here in the actual dictionary. We can just give them a default value of None and tell the user to override them if needed. For example: { 'username': None, # Set to custom username if desired. 'password': None, # Set to custom password if desired. ... } https://chromiumcodereview.appspot.com/10834239/diff/4002/chrome/test/functio... File chrome/test/functional/perf.py (right): https://chromiumcodereview.appspot.com/10834239/diff/4002/chrome/test/functio... chrome/test/functional/perf.py:600: def _GetConfig(self): On 2012/08/11 00:11:27, fdeng1 wrote: > I just moved the changes to perf.py so that the credential configuration will > work for perf.py. How do you think? Good, now we can let the user change the login credentials for the tests that need to log into a google account. This will allow external users to more easily run the tests. https://chromiumcodereview.appspot.com/10834239/diff/4002/chrome/test/functio... chrome/test/functional/perf.py:601: """Load endure configuration file.""" 'endure' --> 'perf test' https://chromiumcodereview.appspot.com/10834239/diff/4002/chrome/test/functio... chrome/test/functional/perf.py:601: """Load endure configuration file.""" Add a "Returns:" section to this docstring to mention that a dictionary is returned that represents the config information. https://chromiumcodereview.appspot.com/10834239/diff/4002/chrome/test/functio... chrome/test/functional/perf.py:609: config.update(pyauto.PyUITest.EvalDataFrom(config_file)) If you address my comment from the .cfg file that asks to explicitly put all config options in the dictionary there, we'll probably have to adjust the implementation here to handle it. We need to make sure we use the default values whenever a config option in the config file has value None. https://chromiumcodereview.appspot.com/10834239/diff/4002/chrome/test/functio... chrome/test/functional/perf.py:611: logging.info('Could not read %s', config_file) maybe you can also log the text that comes along with the SyntaxError too https://chromiumcodereview.appspot.com/10834239/diff/4002/chrome/test/functio... chrome/test/functional/perf.py:620: src/data/pyauto_private/private_tests_info.txt. We can shorten this description to something like this: "Login with user-defined credentials if they exist. Else login with private test credentials if they exist. Else fail." https://chromiumcodereview.appspot.com/10834239/diff/4002/chrome/test/functio... chrome/test/functional/perf.py:658: else: I think it would be simpler to re-arrange the logic here: if username and password: log a message saying we're using the user-specified credentials else if the private test info file exists: log a message saying we're using the private test credentials else: raise a runtime error https://chromiumcodereview.appspot.com/10834239/diff/4002/chrome/test/functio... File chrome/test/functional/perf_endure.py (right): https://chromiumcodereview.appspot.com/10834239/diff/4002/chrome/test/functio... chrome/test/functional/perf_endure.py:36: add 1 more blank line here to separate the imports above from the class below
Hi Dennis, Thanks for the comments. I address them. Please take a look. And I also have two extra questions bellow. https://chromiumcodereview.appspot.com/10834239/diff/4002/chrome/test/functio... File chrome/test/functional/perf.cfg (right): https://chromiumcodereview.appspot.com/10834239/diff/4002/chrome/test/functio... chrome/test/functional/perf.cfg:2: # This file is used to specify google account credential and On 2012/08/15 01:21:17, dennis_jeffrey wrote: > credential --> credentials Done. https://chromiumcodereview.appspot.com/10834239/diff/4002/chrome/test/functio... chrome/test/functional/perf.cfg:4: # the public urls of these services. It is formated as a python dictionary. On 2012/08/15 01:21:17, dennis_jeffrey wrote: > formated --> formatted Done. https://chromiumcodereview.appspot.com/10834239/diff/4002/chrome/test/functio... chrome/test/functional/perf.cfg:14: # # Settings bellow are only effective for perf_endure.py On 2012/08/15 01:21:17, dennis_jeffrey wrote: > bellow --> below Done. https://chromiumcodereview.appspot.com/10834239/diff/4002/chrome/test/functio... chrome/test/functional/perf.cfg:22: # Empty configuration. On 2012/08/15 01:21:17, dennis_jeffrey wrote: > Rather than having a comment above to show all the possible config options (and > hoping the comment is kept up-to-date as the code changes), I think it's better > to explicitly put all the config options here in the actual dictionary. We can > just give them a default value of None and tell the user to override them if > needed. For example: > > { > 'username': None, # Set to custom username if desired. > 'password': None, # Set to custom password if desired. > ... > } Done. https://chromiumcodereview.appspot.com/10834239/diff/4002/chrome/test/functio... File chrome/test/functional/perf.py (right): https://chromiumcodereview.appspot.com/10834239/diff/4002/chrome/test/functio... chrome/test/functional/perf.py:601: """Load endure configuration file.""" On 2012/08/15 01:21:17, dennis_jeffrey wrote: > 'endure' --> 'perf test' Done. https://chromiumcodereview.appspot.com/10834239/diff/4002/chrome/test/functio... chrome/test/functional/perf.py:601: """Load endure configuration file.""" On 2012/08/15 01:21:17, dennis_jeffrey wrote: > Add a "Returns:" section to this docstring to mention that a dictionary is > returned that represents the config information. Done. https://chromiumcodereview.appspot.com/10834239/diff/4002/chrome/test/functio... chrome/test/functional/perf.py:609: config.update(pyauto.PyUITest.EvalDataFrom(config_file)) On 2012/08/15 01:21:17, dennis_jeffrey wrote: > If you address my comment from the .cfg file that asks to explicitly put all > config options in the dictionary there, we'll probably have to adjust the > implementation here to handle it. We need to make sure we use the default > values whenever a config option in the config file has value None. Done. https://chromiumcodereview.appspot.com/10834239/diff/4002/chrome/test/functio... chrome/test/functional/perf.py:611: logging.info('Could not read %s', config_file) On 2012/08/15 01:21:17, dennis_jeffrey wrote: > maybe you can also log the text that comes along with the SyntaxError too Done. https://chromiumcodereview.appspot.com/10834239/diff/4002/chrome/test/functio... chrome/test/functional/perf.py:620: src/data/pyauto_private/private_tests_info.txt. On 2012/08/15 01:21:17, dennis_jeffrey wrote: > We can shorten this description to something like this: > > "Login with user-defined credentials if they exist. Else login with private > test credentials if they exist. Else fail." Done. https://chromiumcodereview.appspot.com/10834239/diff/4002/chrome/test/functio... chrome/test/functional/perf.py:658: else: On 2012/08/15 01:21:17, dennis_jeffrey wrote: > I think it would be simpler to re-arrange the logic here: > > if username and password: > log a message saying we're using the user-specified credentials > else if the private test info file exists: > log a message saying we're using the private test credentials > else: > raise a runtime error Done. https://chromiumcodereview.appspot.com/10834239/diff/4002/chrome/test/functio... File chrome/test/functional/perf_endure.py (right): https://chromiumcodereview.appspot.com/10834239/diff/4002/chrome/test/functio... chrome/test/functional/perf_endure.py:36: On 2012/08/15 01:21:17, dennis_jeffrey wrote: > add 1 more blank line here to separate the imports above from the class below Done. https://chromiumcodereview.appspot.com/10834239/diff/6002/chrome/test/functio... File chrome/test/functional/perf_endure.py (right): https://chromiumcodereview.appspot.com/10834239/diff/6002/chrome/test/functio... chrome/test/functional/perf_endure.py:609: (self._TAB_TITLE_SUBSTRING, loaded_tab_title)) I found that if gmail_url is set to 'https://mail.google.com' The tab loaded_tab_title will not contain "Gmail" and lead to an assertion exception. But if gmail_url is set to 'www.gmail.com' or 'https://mail.google.com/?chat=0&pre=0', it is fine. I can see after Navigating to 'gmail_url', the tab title refreshes several times, the word 'Gmail' is always in the title once the inbox page is loaded. But I guess 'loaded_tab_title' may not point to that tab title? https://chromiumcodereview.appspot.com/10834239/diff/6002/chrome/test/functio... chrome/test/functional/perf_endure.py:648: If we rely on checking the exception that is thrown when trying to access a non-exist element, I think we will need to move the self.WaitForDomNode here (correct me if I am wrong). If so, a problem is that it may think the latency element is not available but in fact it just takes long time to load. Will this situation happen very often?
Thanks! Some more nits, and answers to your two questions. https://chromiumcodereview.appspot.com/10834239/diff/6002/chrome/test/functio... File chrome/test/functional/perf.cfg (right): https://chromiumcodereview.appspot.com/10834239/diff/6002/chrome/test/functio... chrome/test/functional/perf.cfg:1: # Configuration file for pyauto perf tests. We should probably add the license header to the top of this file https://chromiumcodereview.appspot.com/10834239/diff/6002/chrome/test/functio... chrome/test/functional/perf.cfg:12: "google_account_url": "https://accounts.google.com", nit: add a blank line above this to enhance readability (it will separate lines 6-11 as a separate block pertaining to account credentials) https://chromiumcodereview.appspot.com/10834239/diff/6002/chrome/test/functio... chrome/test/functional/perf.cfg:12: "google_account_url": "https://accounts.google.com", Should the google account url be None here? We should use the default URL unless the user explicitly wants to change it. https://chromiumcodereview.appspot.com/10834239/diff/6002/chrome/test/functio... chrome/test/functional/perf.cfg:13: # Settings below are only effective for perf_endure.py nit: add a blank line above this to enhance readability (it will separate lines 13-16 as a separate block pertaining to perf_endure website URLs) https://chromiumcodereview.appspot.com/10834239/diff/6002/chrome/test/functio... chrome/test/functional/perf.cfg:16: "docs_url": "https://drive.google.com", prefer single quotes over double quotes in lines 10-16 above. https://chromiumcodereview.appspot.com/10834239/diff/6002/chrome/test/functio... chrome/test/functional/perf.cfg:16: "docs_url": "https://drive.google.com", should we initialize the gmail, plus, and docs URLs to None in this config file? We should use the default URLs unless the user explicitly wants to change it. https://chromiumcodereview.appspot.com/10834239/diff/6002/chrome/test/functio... File chrome/test/functional/perf.py (right): https://chromiumcodereview.appspot.com/10834239/diff/6002/chrome/test/functio... chrome/test/functional/perf.py:604: A dictionary is returned that represents the config information. remove 'is returned' https://chromiumcodereview.appspot.com/10834239/diff/6002/chrome/test/functio... chrome/test/functional/perf.py:610: 'gmail_url': 'https://www.gmail.com', why is the URL here https whereas the two below are just http? Can we just use http for all sites? https://chromiumcodereview.appspot.com/10834239/diff/6002/chrome/test/functio... chrome/test/functional/perf.py:650: google_account_url = 'https://accounts.google.com/' this should probably use the login URL from the config info https://chromiumcodereview.appspot.com/10834239/diff/6002/chrome/test/functio... chrome/test/functional/perf.py:655: 'Google account credential not defined in %s, using %s', probably don't need to say the config file name or the private file name. Just say something like this: 'User-defined credentials not found, using private test credentials instead.' https://chromiumcodereview.appspot.com/10834239/diff/6002/chrome/test/functio... chrome/test/functional/perf.py:659: 'Please sepecify credential information in %s.\n' \ sepecify --> specify https://chromiumcodereview.appspot.com/10834239/diff/6002/chrome/test/functio... chrome/test/functional/perf.py:659: 'Please sepecify credential information in %s.\n' \ Right before this, say in the log message that no user-defined or private test credentials could be found. https://chromiumcodereview.appspot.com/10834239/diff/6002/chrome/test/functio... chrome/test/functional/perf.py:667: % config_file I think we don't need to print the format in this message. The user can just look inside the credentials file to figure out the format. https://chromiumcodereview.appspot.com/10834239/diff/6002/chrome/test/functio... File chrome/test/functional/perf_endure.py (right): https://chromiumcodereview.appspot.com/10834239/diff/6002/chrome/test/functio... chrome/test/functional/perf_endure.py:609: (self._TAB_TITLE_SUBSTRING, loaded_tab_title)) On 2012/08/16 17:42:49, fdeng1 wrote: > I found that if gmail_url is set to 'https://mail.google.com' > The tab loaded_tab_title will not contain "Gmail" and lead to an assertion > exception. But if gmail_url is set to 'www.gmail.com' or > 'https://mail.google.com/?chat=0&pre=0', it is fine. I can see after Navigating > to 'gmail_url', the tab title refreshes several times, the word 'Gmail' is > always in the title once the inbox page is loaded. But I guess > 'loaded_tab_title' may not point to that tab title? The problem is that line 606 above executes as soon as the call to NavigateToURL in line 605 returns. The call to NavigateToURL returns as soon as chrome gets word that the page load has finished. Sometimes when redirections happen, the first page load may complete, then we get that active tab title (which may say something like "redirecting..."), then we check that title before it actually completes the redirection and changes to something with "Gmail" in it. See line 944-949 below for how we can work around this problem with Google Docs. Maybe we could add the same workaround here for now, to improve reliability. https://chromiumcodereview.appspot.com/10834239/diff/6002/chrome/test/functio... chrome/test/functional/perf_endure.py:648: On 2012/08/16 17:42:49, fdeng1 wrote: > If we rely on checking the exception that is thrown when trying to access a > non-exist element, I think we will need to move the self.WaitForDomNode here > (correct me if I am wrong). If so, a problem is that it may think the latency > element is not available but in fact it just takes long time to load. Will this > situation happen very often? We might be able to avoid this problem in the first place by doing something simpler. The default Gmail test account used by this test has latency dom elements, but user-defined Gmail accounts may not. When latency dom elements do exist, they should exist during the entire test, since they are associated with the Gmail account in use. In other words, they will either always exist, or never exist, for a given test run. How about this idea. At the start of a Gmail test, let's wait up to 3 or 5 seconds for a latency dom element to appear. If it appears within that time, assume that they will always be present and do what the tests currently do. If it does *not* appear in that time, assume they will never exist for this test, and then simply refrain from accessing the latency values during the test run. Do you think that sounds reasonable? This way, we don't need to check on every test iteration whether the latency element exists on the page.
https://chromiumcodereview.appspot.com/10834239/diff/6002/chrome/test/functio... File chrome/test/functional/perf.cfg (right): https://chromiumcodereview.appspot.com/10834239/diff/6002/chrome/test/functio... chrome/test/functional/perf.cfg:1: # Configuration file for pyauto perf tests. On 2012/08/16 18:49:58, dennis_jeffrey wrote: > We should probably add the license header to the top of this file Done. https://chromiumcodereview.appspot.com/10834239/diff/6002/chrome/test/functio... chrome/test/functional/perf.cfg:12: "google_account_url": "https://accounts.google.com", On 2012/08/16 18:49:58, dennis_jeffrey wrote: > nit: add a blank line above this to enhance readability (it will separate lines > 6-11 as a separate block pertaining to account credentials) Done. https://chromiumcodereview.appspot.com/10834239/diff/6002/chrome/test/functio... chrome/test/functional/perf.cfg:13: # Settings below are only effective for perf_endure.py On 2012/08/16 18:49:58, dennis_jeffrey wrote: > nit: add a blank line above this to enhance readability (it will separate lines > 13-16 as a separate block pertaining to perf_endure website URLs) Done. https://chromiumcodereview.appspot.com/10834239/diff/6002/chrome/test/functio... chrome/test/functional/perf.cfg:16: "docs_url": "https://drive.google.com", On 2012/08/16 18:49:58, dennis_jeffrey wrote: > prefer single quotes over double quotes in lines 10-16 above. Done. https://chromiumcodereview.appspot.com/10834239/diff/6002/chrome/test/functio... chrome/test/functional/perf.cfg:16: "docs_url": "https://drive.google.com", On 2012/08/16 18:49:58, dennis_jeffrey wrote: > should we initialize the gmail, plus, and docs URLs to None in this config file? > We should use the default URLs unless the user explicitly wants to change it. Yes, that sounds right. I changed them. https://chromiumcodereview.appspot.com/10834239/diff/6002/chrome/test/functio... File chrome/test/functional/perf.py (right): https://chromiumcodereview.appspot.com/10834239/diff/6002/chrome/test/functio... chrome/test/functional/perf.py:604: A dictionary is returned that represents the config information. On 2012/08/16 18:49:58, dennis_jeffrey wrote: > remove 'is returned' Done. https://chromiumcodereview.appspot.com/10834239/diff/6002/chrome/test/functio... chrome/test/functional/perf.py:610: 'gmail_url': 'https://www.gmail.com', Oh, they are meant to be all https. I think in the test we want https? On 2012/08/16 18:49:58, dennis_jeffrey wrote: > why is the URL here https whereas the two below are just http? Can we just use > http for all sites? https://chromiumcodereview.appspot.com/10834239/diff/6002/chrome/test/functio... chrome/test/functional/perf.py:650: google_account_url = 'https://accounts.google.com/' I was thinking those accounts in private_tests_info.txt are registered with the public google server, so I forced it to be the public one if private_tests_info.txt is used. But on a second thought, I think using the one from the config info may make more sense as if a user has explicitly changed it, we probably should use the custom one he has chosen. On 2012/08/16 18:49:58, dennis_jeffrey wrote: > this should probably use the login URL from the config info https://chromiumcodereview.appspot.com/10834239/diff/6002/chrome/test/functio... chrome/test/functional/perf.py:655: 'Google account credential not defined in %s, using %s', On 2012/08/16 18:49:58, dennis_jeffrey wrote: > probably don't need to say the config file name or the private file name. Just > say something like this: > > 'User-defined credentials not found, using private test credentials instead.' Done. https://chromiumcodereview.appspot.com/10834239/diff/6002/chrome/test/functio... chrome/test/functional/perf.py:659: 'Please sepecify credential information in %s.\n' \ On 2012/08/16 18:49:58, dennis_jeffrey wrote: > sepecify --> specify Done. https://chromiumcodereview.appspot.com/10834239/diff/6002/chrome/test/functio... chrome/test/functional/perf.py:659: 'Please sepecify credential information in %s.\n' \ On 2012/08/16 18:49:58, dennis_jeffrey wrote: > Right before this, say in the log message that no user-defined or private test > credentials could be found. Done. https://chromiumcodereview.appspot.com/10834239/diff/6002/chrome/test/functio... chrome/test/functional/perf.py:667: % config_file On 2012/08/16 18:49:58, dennis_jeffrey wrote: > I think we don't need to print the format in this message. The user can just > look inside the credentials file to figure out the format. Done. https://chromiumcodereview.appspot.com/10834239/diff/6002/chrome/test/functio... File chrome/test/functional/perf_endure.py (right): https://chromiumcodereview.appspot.com/10834239/diff/6002/chrome/test/functio... chrome/test/functional/perf_endure.py:609: (self._TAB_TITLE_SUBSTRING, loaded_tab_title)) On 2012/08/16 18:49:58, dennis_jeffrey wrote: > On 2012/08/16 17:42:49, fdeng1 wrote: > > I found that if gmail_url is set to 'https://mail.google.com' > > The tab loaded_tab_title will not contain "Gmail" and lead to an assertion > > exception. But if gmail_url is set to 'www.gmail.com' or > > 'https://mail.google.com/?chat=0&pre=0', it is fine. I can see after > Navigating > > to 'gmail_url', the tab title refreshes several times, the word 'Gmail' is > > always in the title once the inbox page is loaded. But I guess > > 'loaded_tab_title' may not point to that tab title? > > The problem is that line 606 above executes as soon as the call to NavigateToURL > in line 605 returns. The call to NavigateToURL returns as soon as chrome gets > word that the page load has finished. Sometimes when redirections happen, the > first page load may complete, then we get that active tab title (which may say > something like "redirecting..."), then we check that title before it actually > completes the redirection and changes to something with "Gmail" in it. > > See line 944-949 below for how we can work around this problem with Google Docs. > Maybe we could add the same workaround here for now, to improve reliability. Thanks! Use the same workaround. https://chromiumcodereview.appspot.com/10834239/diff/6002/chrome/test/functio... chrome/test/functional/perf_endure.py:648: On 2012/08/16 18:49:58, dennis_jeffrey wrote: > On 2012/08/16 17:42:49, fdeng1 wrote: > > If we rely on checking the exception that is thrown when trying to access a > > non-exist element, I think we will need to move the self.WaitForDomNode here > > (correct me if I am wrong). If so, a problem is that it may think the latency > > element is not available but in fact it just takes long time to load. Will > this > > situation happen very often? > > We might be able to avoid this problem in the first place by doing something > simpler. The default Gmail test account used by this test has latency dom > elements, but user-defined Gmail accounts may not. When latency dom elements do > exist, they should exist during the entire test, since they are associated with > the Gmail account in use. In other words, they will either always exist, or > never exist, for a given test run. > > How about this idea. At the start of a Gmail test, let's wait up to 3 or 5 > seconds for a latency dom element to appear. If it appears within that time, > assume that they will always be present and do what the tests currently do. If > it does *not* appear in that time, assume they will never exist for this test, > and then simply refrain from accessing the latency values during the test run. > > Do you think that sounds reasonable? This way, we don't need to check on every > test iteration whether the latency element exists on the page. I think we can do this. I implemented this and set the timeout to 3secs. I tested with 100 milliseconds and it works with my current bandwidth. I hope 3 is able to tolerant most of the cases where the element is available.
Few more nits. https://chromiumcodereview.appspot.com/10834239/diff/6002/chrome/test/functio... File chrome/test/functional/perf.py (right): https://chromiumcodereview.appspot.com/10834239/diff/6002/chrome/test/functio... chrome/test/functional/perf.py:610: 'gmail_url': 'https://www.gmail.com', On 2012/08/17 06:39:34, fdeng1 wrote: > Oh, they are meant to be all https. > I think in the test we want https? Previously the tests navigated to the http site, but I think it doesn't make a difference either way. Navigating to http://www.gmail.com seems to redirect to an https site anyway. > > On 2012/08/16 18:49:58, dennis_jeffrey wrote: > > why is the URL here https whereas the two below are just http? Can we just > use > > http for all sites? > https://chromiumcodereview.appspot.com/10834239/diff/6002/chrome/test/functio... File chrome/test/functional/perf_endure.py (right): https://chromiumcodereview.appspot.com/10834239/diff/6002/chrome/test/functio... chrome/test/functional/perf_endure.py:648: On 2012/08/17 06:39:34, fdeng1 wrote: > On 2012/08/16 18:49:58, dennis_jeffrey wrote: > > On 2012/08/16 17:42:49, fdeng1 wrote: > > > If we rely on checking the exception that is thrown when trying to access a > > > non-exist element, I think we will need to move the self.WaitForDomNode here > > > (correct me if I am wrong). If so, a problem is that it may think the > latency > > > element is not available but in fact it just takes long time to load. Will > > this > > > situation happen very often? > > > > We might be able to avoid this problem in the first place by doing something > > simpler. The default Gmail test account used by this test has latency dom > > elements, but user-defined Gmail accounts may not. When latency dom elements > do > > exist, they should exist during the entire test, since they are associated > with > > the Gmail account in use. In other words, they will either always exist, or > > never exist, for a given test run. > > > > How about this idea. At the start of a Gmail test, let's wait up to 3 or 5 > > seconds for a latency dom element to appear. If it appears within that time, > > assume that they will always be present and do what the tests currently do. > If > > it does *not* appear in that time, assume they will never exist for this test, > > and then simply refrain from accessing the latency values during the test run. > > > > Do you think that sounds reasonable? This way, we don't need to check on > every > > test iteration whether the latency element exists on the page. > > I think we can do this. I implemented this and set the timeout to 3secs. I > tested with 100 milliseconds and it works with my current bandwidth. I hope 3 is > able to tolerant most of the cases where the element is available. If you think it'll be safer with 5 seconds, we can do that too. It just means a slight delay at test startup before the test starts. But if the test runs for hours anyhow, an extra 5 seconds shouldn't matter that much. Also, the only potential problem that might happen is we may not find a latency DOM element within 5 seconds, even though one will eventually appear. If that case ever happens, the only effect is that the test simply doesn't record latency values during the run. I think that's ok. https://chromiumcodereview.appspot.com/10834239/diff/14002/chrome/test/functi... File chrome/test/functional/perf.cfg (right): https://chromiumcodereview.appspot.com/10834239/diff/14002/chrome/test/functi... chrome/test/functional/perf.cfg:20: 'google_account_url': None, Add a comment here to let people know what the default is (that way, it's easier for them to realize whether they need to change it): 'google_account_url': None, # Defaults to '...' https://chromiumcodereview.appspot.com/10834239/diff/14002/chrome/test/functi... chrome/test/functional/perf.cfg:26: 'docs_url': None, for each of the above 3 lines, do the same thing as mentioned in the above comment, so people know what the defaults will be. https://chromiumcodereview.appspot.com/10834239/diff/14002/chrome/test/functi... File chrome/test/functional/perf_endure.py (right): https://chromiumcodereview.appspot.com/10834239/diff/14002/chrome/test/functi... chrome/test/functional/perf_endure.py:627: # Test whether latency dom element is available. nit: add a blank line above this to separate the latency-checking code into a separate block https://chromiumcodereview.appspot.com/10834239/diff/14002/chrome/test/functi... chrome/test/functional/perf_endure.py:657: the latency dom element to appear. describe what happens with the default value of -1
Hi Dennis, Your comments are addressed. The last patch is just a merge with origin/master to make sure it still work after merging. PTAL. Thanks, Fang https://chromiumcodereview.appspot.com/10834239/diff/6002/chrome/test/functio... File chrome/test/functional/perf.py (right): https://chromiumcodereview.appspot.com/10834239/diff/6002/chrome/test/functio... chrome/test/functional/perf.py:610: 'gmail_url': 'https://www.gmail.com', Ok, I just leave them to be https. On 2012/08/17 17:35:44, dennis_jeffrey wrote: > On 2012/08/17 06:39:34, fdeng1 wrote: > > Oh, they are meant to be all https. > > I think in the test we want https? > > Previously the tests navigated to the http site, but I think it doesn't make a > difference either way. Navigating to http://www.gmail.com seems to redirect to > an https site anyway. > > > > > On 2012/08/16 18:49:58, dennis_jeffrey wrote: > > > why is the URL here https whereas the two below are just http? Can we just > > use > > > http for all sites? > > > https://chromiumcodereview.appspot.com/10834239/diff/14002/chrome/test/functi... File chrome/test/functional/perf.cfg (right): https://chromiumcodereview.appspot.com/10834239/diff/14002/chrome/test/functi... chrome/test/functional/perf.cfg:20: 'google_account_url': None, On 2012/08/17 17:35:44, dennis_jeffrey wrote: > Add a comment here to let people know what the default is (that way, it's easier > for them to realize whether they need to change it): > > 'google_account_url': None, # Defaults to '...' Done. https://chromiumcodereview.appspot.com/10834239/diff/14002/chrome/test/functi... chrome/test/functional/perf.cfg:26: 'docs_url': None, On 2012/08/17 17:35:44, dennis_jeffrey wrote: > for each of the above 3 lines, do the same thing as mentioned in the above > comment, so people know what the defaults will be. Done. https://chromiumcodereview.appspot.com/10834239/diff/14002/chrome/test/functi... File chrome/test/functional/perf_endure.py (right): https://chromiumcodereview.appspot.com/10834239/diff/14002/chrome/test/functi... chrome/test/functional/perf_endure.py:627: # Test whether latency dom element is available. On 2012/08/17 17:35:44, dennis_jeffrey wrote: > nit: add a blank line above this to separate the latency-checking code into a > separate block Done. https://chromiumcodereview.appspot.com/10834239/diff/14002/chrome/test/functi... chrome/test/functional/perf_endure.py:657: the latency dom element to appear. On 2012/08/17 17:35:44, dennis_jeffrey wrote: > describe what happens with the default value of -1 Done.
LGTM Before submitting, make sure the added file in this CL passes the check_perms script.
Thanks for the reminder, I'll run the check_perms first. Fang On Fri, Aug 17, 2012 at 4:25 PM, <dennisjeffrey@chromium.org> wrote: > > LGTM > > Before submitting, make sure the added file in this CL passes the > check_perms > script. > > https://chromiumcodereview.**appspot.com/10834239/<https://chromiumcodereview... >
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/fdeng@chromium.org/10834239/6005
Change committed as 152224
https://chromiumcodereview.appspot.com/10834239/diff/6005/chrome/test/functio... File chrome/test/functional/perf.py (right): https://chromiumcodereview.appspot.com/10834239/diff/6005/chrome/test/functio... chrome/test/functional/perf.py:616: for key in new_config: this whole block is equivalent to: config.update(new_config) https://chromiumcodereview.appspot.com/10834239/diff/6005/chrome/test/functio... chrome/test/functional/perf.py:617: if new_config.get(key) is not None: 'is not None' is redundant https://chromiumcodereview.appspot.com/10834239/diff/6005/chrome/test/functio... chrome/test/functional/perf.py:640: config_file = os.path.join(os.path.dirname(__file__), 'perf.cfg') this var is used in several locations. Make it a private var in this class. https://chromiumcodereview.appspot.com/10834239/diff/6005/chrome/test/functio... chrome/test/functional/perf.py:648: os.path.join(os.path.dirname(__file__), 'perf.cfg')) use the var https://chromiumcodereview.appspot.com/10834239/diff/6005/chrome/test/functio... File chrome/test/functional/perf_endure.py (right): https://chromiumcodereview.appspot.com/10834239/diff/6005/chrome/test/functio... chrome/test/functional/perf_endure.py:605: timeout=60, expect_retval=True, retry_sleep=1), why are you overriding the timeout? |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
