Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
273523004:
Password Manager testing automation (Closed)
|
Created:
6 years, 7 months ago by rchtara Modified:
6 years, 7 months ago Reviewers:
vabr (Chromium) CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionPassword Manager testing automation
Adding automatic tests to the password manager to simulate user interaction with websites.
BUG=369521
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=272545
Patch Set 1 #Patch Set 2 : #
Total comments: 2
Patch Set 3 : cmd #
Total comments: 84
Patch Set 4 : waituntildisplayed new action #Patch Set 5 : #Patch Set 6 : new arch #
Total comments: 64
Patch Set 7 : password internals #Patch Set 8 : password internals #Patch Set 9 : review #Patch Set 10 : #Patch Set 11 : clear profile #Patch Set 12 : log #
Total comments: 125
Patch Set 13 : add prompt tests #Patch Set 14 : renaming #
Total comments: 94
Patch Set 15 : #Patch Set 16 : rev #
Total comments: 36
Patch Set 17 : remove enable-password-manager-internals-ui #Patch Set 18 : #
Total comments: 20
Patch Set 19 : #Patch Set 20 : #
Total comments: 6
Patch Set 21 : const #Patch Set 22 : copyright #
Messages
Total messages: 36 (0 generated)
Hi, This then new cl. Could you please check it. Thanks a lot Cheers Riadh https://codereview.chromium.org/273523004/diff/20001/components/test/data/pas... File components/test/data/password_manager/README (right): https://codereview.chromium.org/273523004/diff/20001/components/test/data/pas... components/test/data/password_manager/README:105: 2) tests that can cause a crash (the cause of the crash is not related to the The reason why I split the failing tests into 2 categories: * the failing tests are related to the password manager, which is the component we are testing here. They are failing because of a known bug that we need to solve. * tests that can cause a crash are different, because the reason behind the crashes is unknown (sometimes the crash never happen) and of course not related to the password manager. These tests are going to be probably working when chrome became more stable.
Hi Vaclav, I updated the patch. Could you please review it. Thanks a lot Cheers Riadh
Hi Riadh, Thanks a lot for all the work so far! It's a whole set of new files, so I have a couple of comments. Please note that most of the issues pointed out can be found on multiple places across all the files, despite me mentioning them only once -- please bear that in mind when addressing them and check for the other places as well. Also, I did not have a chance to review the last patch set (p. s. 4). I guess I will take a look at the changes in the next round of review. Cheers, Vaclav https://codereview.chromium.org/273523004/diff/20001/components/test/data/pas... File components/test/data/password_manager/README (right): https://codereview.chromium.org/273523004/diff/20001/components/test/data/pas... components/test/data/password_manager/README:105: 2) tests that can cause a crash (the cause of the crash is not related to the On 2014/05/08 07:46:14, rchtara wrote: > The reason why I split the failing tests into 2 categories: > * the failing tests are related to the password manager, which is the component > we are testing here. They are failing because of a known bug that we need to > solve. > * tests that can cause a crash are different, because the reason behind the > crashes is unknown (sometimes the crash never happen) and of course not related > to the password manager. If the reason is unknown, then how do we know it's not related to PasswordManager? > These tests are going to be probably working when > chrome became more stable. Well, Chrome does not become more stable by itself. If there is a crash, then it is a bug, and should be reported. If it turns out to be outside of PasswordManagement, then we can find an appropriate owner. I still don't see a reason why to separate those failing tests -- they fail because of some bug in Chromium, be it in PasswordManager, or elsewhere. If you know about a benefit coming from the failing tests being in two separate categories, please let me know. Otherwise I think we should only have one group of failing tests. https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... File components/test/data/password_manager/README (right): https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/README:39: Run all the tests tests by executing: tests tests -- typo? https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/README:80: * optinalfillusername: find an input element using CSS Selector and fill it typo: optinal -> optional https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/README:81: with the username. Don't break if the input element is not available. nit: "Don't break" -> "Do nothing" (It's not clear from the context what should not be broken.) https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/README:81: with the username. Don't break if the input element is not available. available -> visible https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/README:100: class website create an instance of Action. If you need a new kind of files I'm not sure I understand what "new kind of files" mean. Could you please clarify? https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... File components/test/data/password_manager/action.py (right): https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/action.py:1: import time Please add a module docstring (also in other Python files). http://legacy.python.org/dev/peps/pep-0008/#id24 https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/action.py:1: import time Also please add the encoding, like you did in tests.py, in all other Python files, unless it is ASCII. http://legacy.python.org/dev/peps/pep-0008/#id16 https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/action.py:8: Please add a docstring documenting this class. Also add it for all other classes you define. https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/action.py:16: """ 'For one liner docstrings, please keep the closing """ on the same line.' http://legacy.python.org/dev/peps/pep-0008/#id24 (For multi-line docstrings, having the closing """ on a separate line is OK.) https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/action.py:17: print "actiontype: %s actionparam: %s" % (self.action_type, self.param) If you need this temporarily as a debugging message, please add a TODO(rchtara): to remove it as soon as possible. In general, we try to avoid spamming the output, so that debug messages we care about at the moment are easily visible. Ditto for all the uses of "print" below. https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/action.py:35: elif self.action_type == "optinalfillusername": typo: optinal -> optional https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/action.py:48: """Chrome protects the password inputs and doesn't fill them until As already mentioned off-line: except for doc-strings for public modules, functions, classes, and methods, do not use string literals as block comments. Use standard comments, starting the line with a "# ". https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/action.py:56: password_element.send_keys("a") Why do we actually care if Chrome fills the password, when we overwrite it instantly? https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/action.py:60: time.sleep(self.param) You already have one if-branch for "wait". Please remove this one. https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/action.py:63: """Make the action behavior when the username and password fields are nit: Please use single-space consistently, no double-space between words. https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/action.py:63: """Make the action behavior when the username and password fields are I cannot parse the sentence. Are you trying to say the following? "For the actions fillusername and fillpassword, check that the state of the DOM already corresponds to the result of intended action. For other actions, just perform Do()." https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/action.py:67: nit: Please remove the blank line. https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/action.py:73: assert username_element.get_attribute("value") == self.website.username, ( The Chromium style guide for Python (http://dev.chromium.org/developers/coding-style#TOC-Python) says: "Note that asserts are of limited use, and should not be used for validating input – throw an exception instead." Here you are validating input -- the displayed page, so you should use an exception. Ditto below. https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/action.py:79: """Chrome protects the password inputs and doesn't fill them until Actually, some errors I saw in the past were when the password value was not accessible even after the user submitted the form. I don't think that "Chrome does not fill password inputs until the user interacts with them" is correct -- mostly, Chrome should autofill the passwords, and make them accessible on any user interaction with any part of the page. There are exceptions -- PSL-matched passwords (should not be a concern of these tests), and forms in iframes (that could happen). In that case, a user interaction with the _username_ input (not password input) should be enough to trigger autocomplete. In other words, filling the username in the username input completely should always guarantee the password being autofilled and accessible. If not, that's a bug. Could we make the test not to overwrite the autofilled passwords, to also catch such bugs? https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/action.py:101: """Do the action behavior when the password field is not expected to be Please once you improve the docstring for the previous method, phrase this one accordingly. https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/action.py:120: assert len(ps) == 0, ("Error: password is autofilled when it shouldn't " len(ps) == 0 should be just ps (Look for "len(" in http://legacy.python.org/dev/peps/pep-0008/#id41.) Other than that, exception instead of an assert please, as noted above. https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... File components/test/data/password_manager/environment.py (right): https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/environment.py:4: import xml.etree.ElementTree as ET Please don't use ET, use the original name, to increase readability. https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/environment.py:11: def __init__(self, passwords_file = ""): No spaces around "=" when defining default values. http://legacy.python.org/dev/peps/pep-0008/#id20 Please correct also elsewhere in this CL. https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/environment.py:16: options.add_argument("enable-automatic-password-saving") Please comment on why this is needed. (I still know why :), but other people reading it might not.) https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/environment.py:17: #chrome path Please separate # and the first word with a space. (Also below.) https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/environment.py:19: "/usr/local/google/home/rchtara/chrome/src/out/Debug/chrome") Please do not hard-code your local path into the uploaded script. Ideally make it a command-line option, or allow specifying a configuration file from which to read it. This holds also for the profile and webdriver path below. https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/environment.py:30: self.passwords_tree = None Please comment on what is the |passwords_tree|. https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/environment.py:34: def AddWebsite(self, Please add a docstring documenting this method, and all other public methods you define. https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/environment.py:63: return (urls, remove_buttons) Why do you bother with returning buttons, when you ignore them at the only place when this method is called? https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/environment.py:69: """It's not possible to open a new tab to chrome:// urls directly, Sorry, this sentence is hard to parse, and I actually don't understand what is going on here. Please make each point one sentence, describe what is the main goal here, what does not work, and what is a work-around. Also, if you find that Chromedriver is not accepting chrome:// URLs, it would be useful to know if that's a known limitation. When not, then ideally file a bug (if Chromedriver developers accept bugreports), and add a tracking link here, so that the work-around can be removed in the future. https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/environment.py:107: for test in tests: Use rather: if website.name in tests: websites.append(website) https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... File components/test/data/password_manager/tests.py (right): https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/tests.py:11: environment = Environment("websites.xml") This should be part of the if __name__ == "__main__": block below. https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/tests.py:17: amazon = environment.AddWebsite("amazon") Could the whole addition of tests be a function? It could be one function for the working tests, and one for the failing tests. That function should be then called from the if __name__ == "__main__": block below. https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/tests.py:58: "password=dN_xw0GVUbgx935g3OU51&username=jkszbfkizhsefb789y4fui%40s" This URL still contains the username. Please scrub and minimize all the test URLs -- they contain relics of your browsing state, which are not helpful for testing. Ideally, no query parameters (stuff after "?") should be needed. If it is, then only add what is needed. The hostname and path might be also worth reviewing in many cases. https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/tests.py:104: mailru.AddLoginAction("goto", "https://ww.mail.ru") ww.mail.ru ? Why not just mail.ru ? https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/tests.py:178: """ Tests that can cause a crash (the cause of the crash is not related to the For all crashing tests, please add a bugreport for the failure, as you do for the ones below (thumbs up for that! :)). If there is no bug report yet, please add a TODO(rchtara): to supply it later. Tests which fail for reason nobody cares about are not helpful. https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/tests.py:301: """ Tests shutdown. """ I don't understand what is meant here. https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/tests.py:306: args = sys.argv[1:] Please enclose the code here in a if __name__ == "__main__": ... block, to follow the convention, and make sure nothing gets executed on importing this module. https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/tests.py:308: if len(args) == 0: Please also consider printing a brief help on arguments usage. https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... File components/test/data/password_manager/website.py (right): https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/website.py:29: self.internals_folder = ("file:///usr/local/google/home/rchtara/chrome" Please add a TODO for removing this as soon as the internals page is ready. That's going to happen soon, so I'm fine with leaving the path here until then. https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/website.py:64: if current_url in url_to_remove or url_to_remove in current_url: You use the x in y or y in x pattern quite often for URLs here. Consider writing a local function for that, e.g., _IsOneSubstringOfAnother Also, note that if an URL is a substring of another, it is in fact its prefix. Testing for prefix is easier than for substring (you just trim the longer string and ask for equality). https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/website.py:68: time.sleep(1) # wait until command is executed nit: please start the comment with a capital letter, and end it with a full-stop. https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/website.py:109: p = self.password Please use more descriptive variable names. Here, e.g.: correct_password.
Hi, These are the new tests with the new arch. Could you please check them? Thanks a lot Cheers Riadh https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... File components/test/data/password_manager/README (right): https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/README:39: Run all the tests tests by executing: On 2014/05/08 13:53:45, vabr (Chromium) wrote: > tests tests -- typo? Done. https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/README:80: * optinalfillusername: find an input element using CSS Selector and fill it On 2014/05/08 13:53:45, vabr (Chromium) wrote: > typo: optinal -> optional Done. https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/README:81: with the username. Don't break if the input element is not available. On 2014/05/08 13:53:45, vabr (Chromium) wrote: > nit: "Don't break" -> "Do nothing" > (It's not clear from the context what should not be broken.) Done. https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/README:81: with the username. Don't break if the input element is not available. On 2014/05/08 13:53:45, vabr (Chromium) wrote: > nit: "Don't break" -> "Do nothing" > (It's not clear from the context what should not be broken.) Done. https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/README:100: class website create an instance of Action. If you need a new kind of files On 2014/05/08 13:53:45, vabr (Chromium) wrote: > I'm not sure I understand what "new kind of files" mean. Could you please > clarify? Done. https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... File components/test/data/password_manager/action.py (right): https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/action.py:1: import time On 2014/05/08 13:53:45, vabr (Chromium) wrote: > Also please add the encoding, like you did in tests.py, in all other Python > files, unless it is ASCII. > http://legacy.python.org/dev/peps/pep-0008/#id16 Done. https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/action.py:16: """ On 2014/05/08 13:53:45, vabr (Chromium) wrote: > 'For one liner docstrings, please keep the closing """ on the same line.' > http://legacy.python.org/dev/peps/pep-0008/#id24 > > (For multi-line docstrings, having the closing """ on a separate line is OK.) Done. https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/action.py:17: print "actiontype: %s actionparam: %s" % (self.action_type, self.param) I think it's important to put the log, because the tests crash quite often. And the only way to figure out when and why the crash has happen is by reading log msg. https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/action.py:35: elif self.action_type == "optinalfillusername": On 2014/05/08 13:53:45, vabr (Chromium) wrote: > typo: optinal -> optional Done. https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/action.py:48: """Chrome protects the password inputs and doesn't fill them until On 2014/05/08 13:53:45, vabr (Chromium) wrote: > As already mentioned off-line: except for doc-strings for public modules, > functions, classes, and methods, do not use string literals as block comments. > Use standard comments, starting the line with a "# ". Done. https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/action.py:56: password_element.send_keys("a") Chrome doesn't fill password input until the user interacts with it. When we send "a", chrome is going to fill the input with the saved password then adds "a" at the end. We then clear the password. And then we can send whatever we like. If we just send the password (even if we clear the input before that) , and there is a saved password in chrome, the input will be filled by a concatenation of both passwords. https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/action.py:60: time.sleep(self.param) On 2014/05/08 13:53:45, vabr (Chromium) wrote: > You already have one if-branch for "wait". Please remove this one. Done. https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/action.py:63: """Make the action behavior when the username and password fields are On 2014/05/08 13:53:45, vabr (Chromium) wrote: > nit: Please use single-space consistently, no double-space between words. Done. https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/action.py:63: """Make the action behavior when the username and password fields are On 2014/05/08 13:53:45, vabr (Chromium) wrote: > nit: Please use single-space consistently, no double-space between words. Done. https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/action.py:67: On 2014/05/08 13:53:45, vabr (Chromium) wrote: > nit: Please remove the blank line. Done. https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/action.py:73: assert username_element.get_attribute("value") == self.website.username, ( On 2014/05/08 13:53:45, vabr (Chromium) wrote: > The Chromium style guide for Python > (http://dev.chromium.org/developers/coding-style#TOC-Python) says: > "Note that asserts are of limited use, and should not be used for validating > input – throw an exception instead." > Here you are validating input -- the displayed page, so you should use an > exception. > > Ditto below. Done. https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/action.py:79: """Chrome protects the password inputs and doesn't fill them until I made the test and you right: the password is autofilled after any kind of interaction with the page. So I will change the comment, but not the code: *sometimes we dont have to fill the user name, getting the password is going to be our first interaction *I tried to click on the password via chromedriver, but i go problem with some with some website, because the password field is hidden. So let's keep the code https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/action.py:101: """Do the action behavior when the password field is not expected to be On 2014/05/08 13:53:45, vabr (Chromium) wrote: > Please once you improve the docstring for the previous method, phrase this one > accordingly. Done. https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/action.py:101: """Do the action behavior when the password field is not expected to be On 2014/05/08 13:53:45, vabr (Chromium) wrote: > Please once you improve the docstring for the previous method, phrase this one > accordingly. Done. https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/action.py:120: assert len(ps) == 0, ("Error: password is autofilled when it shouldn't " On 2014/05/08 13:53:45, vabr (Chromium) wrote: > len(ps) == 0 > should be just > ps > (Look for "len(" in http://legacy.python.org/dev/peps/pep-0008/#id41.) > > Other than that, exception instead of an assert please, as noted above. Done. https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... File components/test/data/password_manager/environment.py (right): https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/environment.py:4: import xml.etree.ElementTree as ET On 2014/05/08 13:53:45, vabr (Chromium) wrote: > Please don't use ET, use the original name, to increase readability. Done. https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/environment.py:11: def __init__(self, passwords_file = ""): On 2014/05/08 13:53:45, vabr (Chromium) wrote: > No spaces around "=" when defining default values. > http://legacy.python.org/dev/peps/pep-0008/#id20 > > Please correct also elsewhere in this CL. Done. https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/environment.py:16: options.add_argument("enable-automatic-password-saving") On 2014/05/08 13:53:45, vabr (Chromium) wrote: > Please comment on why this is needed. > (I still know why :), but other people reading it might not.) Done. https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/environment.py:17: #chrome path On 2014/05/08 13:53:45, vabr (Chromium) wrote: > Please separate # and the first word with a space. (Also below.) Done. https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/environment.py:19: "/usr/local/google/home/rchtara/chrome/src/out/Debug/chrome") On 2014/05/08 13:53:45, vabr (Chromium) wrote: > Please do not hard-code your local path into the uploaded script. > Ideally make it a command-line option, or allow specifying a configuration file > from which to read it. > > This holds also for the profile and webdriver path below. Done. https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/environment.py:30: self.passwords_tree = None On 2014/05/08 13:53:45, vabr (Chromium) wrote: > Please comment on what is the |passwords_tree|. Done. https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/environment.py:63: return (urls, remove_buttons) On 2014/05/08 13:53:45, vabr (Chromium) wrote: > Why do you bother with returning buttons, when you ignore them at the only place > when this method is called? Done. https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/environment.py:69: """It's not possible to open a new tab to chrome:// urls directly, There is two issues here: *There is no straightforward way to open a new tab with chromedriver. One of work-around is to go to a website, insert a link that is going to be opened in a new tab, click on it. To avoid that the chrome popup blocker blocks the new tab, we need to choose a trusted website which is google.com. *Links for local resources (chrome://*) are blocked. So this why the new tab is going to be for google.com and then the browser is asked via chromedriver to go to chrome://settings/passwords https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/environment.py:107: for test in tests: On 2014/05/08 13:53:45, vabr (Chromium) wrote: > Use rather: > if website.name in tests: > websites.append(website) Done. https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... File components/test/data/password_manager/tests.py (right): https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/tests.py:11: environment = Environment("websites.xml") On 2014/05/08 13:53:45, vabr (Chromium) wrote: > This should be part of the > if __name__ == "__main__": > block below. Done. https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/tests.py:17: amazon = environment.AddWebsite("amazon") On 2014/05/08 13:53:45, vabr (Chromium) wrote: > Could the whole addition of tests be a function? > It could be one function for the working tests, and one for the failing tests. > > That function should be then called from the > if __name__ == "__main__": > block below. Done. https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/tests.py:58: "password=dN_xw0GVUbgx935g3OU51&username=jkszbfkizhsefb789y4fui%40s" On 2014/05/08 13:53:45, vabr (Chromium) wrote: > This URL still contains the username. > Please scrub and minimize all the test URLs -- they contain relics of your > browsing state, which are not helpful for testing. > > Ideally, no query parameters (stuff after "?") should be needed. If it is, then > only add what is needed. The hostname and path might be also worth reviewing in > many cases. Done. https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/tests.py:104: mailru.AddLoginAction("goto", "https://ww.mail.ru") On 2014/05/08 13:53:45, vabr (Chromium) wrote: > ww.mail.ru ? > Why not just mail.ru ? Done. https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/tests.py:178: """ Tests that can cause a crash (the cause of the crash is not related to the I think that the chrome version we use to run the test is very unstable especially with websites that have a lot of videos, images, flash. It's not easy to know the reason behind the crash. https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/tests.py:301: """ Tests shutdown. """ On 2014/05/08 13:53:45, vabr (Chromium) wrote: > I don't understand what is meant here. Done. https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/tests.py:306: args = sys.argv[1:] On 2014/05/08 13:53:45, vabr (Chromium) wrote: > Please enclose the code here in a > if __name__ == "__main__": > ... > block, to follow the convention, and make sure nothing gets executed on > importing this module. Done. https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/tests.py:308: if len(args) == 0: On 2014/05/08 13:53:45, vabr (Chromium) wrote: > Please also consider printing a brief help on arguments usage. Done. https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... File components/test/data/password_manager/website.py (right): https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/website.py:29: self.internals_folder = ("file:///usr/local/google/home/rchtara/chrome" On 2014/05/08 13:53:45, vabr (Chromium) wrote: > Please add a TODO for removing this as soon as the internals page is ready. > That's going to happen soon, so I'm fine with leaving the path here until then. Done. https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/website.py:64: if current_url in url_to_remove or url_to_remove in current_url: On 2014/05/08 13:53:45, vabr (Chromium) wrote: > You use the > x in y or y in x > pattern quite often for URLs here. > Consider writing a local function for that, e.g., _IsOneSubstringOfAnother > > Also, note that if an URL is a substring of another, it is in fact its prefix. > Testing for prefix is easier than for substring (you just trim the longer string > and ask for equality). Done. https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/website.py:68: time.sleep(1) # wait until command is executed On 2014/05/08 13:53:45, vabr (Chromium) wrote: > nit: please start the comment with a capital letter, and end it with a > full-stop. Done. https://codereview.chromium.org/273523004/diff/40001/components/test/data/pas... components/test/data/password_manager/website.py:109: p = self.password On 2014/05/08 13:53:45, vabr (Chromium) wrote: > Please use more descriptive variable names. > Here, e.g.: correct_password. Done.
Hi, I updated the code to support the password internals page. Could you please check that too. Thanks a lot Cheers Riadh
Hi Riadh, Thanks for the new version. I saw a couple of improvements already. Unfortunately, I did not manage to review the whole CL yet, I only went trough the first two files. I have a fair amount of comments already, so I'm publishing those. Feel free to incorporate them, and then I will have a look at the rest. Or if you prefer me reviewing the whole CL before you start working on the comments, that's fine. Just let me know in that case, and I'll review the rest tomorrow morning. Cheers, Vaclav https://codereview.chromium.org/273523004/diff/90001/components/test/data/pas... File components/test/data/password_manager/README (right): https://codereview.chromium.org/273523004/diff/90001/components/test/data/pas... components/test/data/password_manager/README:43: python tests.py --chrome-path CHROMEPATH --chromedriver-path CHROMEDRIVERPATH Is this command the same as the one for "Run all the working tests tests by executing"? https://codereview.chromium.org/273523004/diff/90001/components/test/data/pas... components/test/data/password_manager/README:74: class NEWWEBSITE(Website): Using all-caps for class name is against the Python style, and should not be present in a code example. https://codereview.chromium.org/273523004/diff/90001/components/test/data/pas... components/test/data/password_manager/README:77: self.GoTo("hhtp://url") hhtp->http https://codereview.chromium.org/273523004/diff/90001/components/test/data/pas... components/test/data/password_manager/README:78: self.FillUsername("#username") What do you actually mean by #username, #password? I assume that those are CSS selectors for the elements, but it would be great to state it explicitly (it looks like username and password values instead), and be precise about what exactly can be used. (For example, anything from http://www.w3schools.com/cssref/css_selectors.asp ?) https://codereview.chromium.org/273523004/diff/90001/components/test/data/pas... components/test/data/password_manager/README:86: Yahoo("NEWWEBSITE("http://url", username_not_auto=True)) There is an odd number (3) of quotation marks in this expression, and one more opening parenthesis than there are closing parentheses. Did you mean to remove 'Yahoo("'? https://codereview.chromium.org/273523004/diff/90001/components/test/data/pas... components/test/data/password_manager/README:93: * ClickIfAvailable: find an element using CSS Selector and if it's available, Please elaborate on what's the difference between Click and ClickIfAvailable. I assume Click throws an exception if the element is not available? Also, please specify what you mean by "available" (present in the DOM of the page? visible?). https://codereview.chromium.org/273523004/diff/90001/components/test/data/pas... components/test/data/password_manager/README:97: self.Enter("css_selector") nit: Enter -> SendEnterTo (The literal meaning of "enter something" seems to be too strongly competing with the intended abbreviation. In other words, this sounds like "css_selector" should be entered somewhere in the page.) https://codereview.chromium.org/273523004/diff/90001/components/test/data/pas... components/test/data/password_manager/README:98: * GoTo: go to some url. What does "go to" mean? Navigating the main frame? https://codereview.chromium.org/273523004/diff/90001/components/test/data/pas... components/test/data/password_manager/README:100: * Hover: find an element using CSS Selector and hover it. hover it -> hover over it https://codereview.chromium.org/273523004/diff/90001/components/test/data/pas... components/test/data/password_manager/README:101: self.Hover("css_selector") optional nit: Hover -> HoverOver https://codereview.chromium.org/273523004/diff/90001/components/test/data/pas... components/test/data/password_manager/README:102: Why the blank line? Unless it separates different groups of methods, please remove it (or put a blank line between every pair). Ditto below. https://codereview.chromium.org/273523004/diff/90001/components/test/data/pas... components/test/data/password_manager/README:105: * Wait: wait for some amount of time. Please mention units. (Seconds?) https://codereview.chromium.org/273523004/diff/90001/components/test/data/pas... components/test/data/password_manager/README:112: * FillPassword: find an input element using CSS Selector and fill it with nit: FillPasswordInto, FillUsernameInto Just FillPassword, FillUsername hint that the password and username to be filled are the arguments of those functions. https://codereview.chromium.org/273523004/diff/90001/components/test/data/pas... components/test/data/password_manager/README:118: * OptionalFillUsername: find an input element using CSS Selector and fill it nit: OptionalFillUsername -> FillUsernameIfVisible Just saying Optional does not clarify when the username is filled and when no. (no need to add *Into, because the argument is now more tied to IfVisible) https://codereview.chromium.org/273523004/diff/90001/components/test/data/pas... components/test/data/password_manager/README:119: with the username. Do nothing if the input element is not visible. Is "doing nothing" the important difference? Or would FillUsernameInto actually throw an exception, while FillUsernameIfVisible won't? Please clarify in the comment. https://codereview.chromium.org/273523004/diff/90001/components/test/data/pas... components/test/data/password_manager/README:121: * Submit: find an element using CSS Selector and submit it. optional nit: submit it -> call its submit() handler (Again, submit X literally means taking X and putting it somewhere, whereas the submit() method of a form means submitting the content of the form, not the form itself. But I'm probably being language paranoid :), that's why it's just optional. :)) https://codereview.chromium.org/273523004/diff/90001/components/test/data/pas... File components/test/data/password_manager/environment.py (right): https://codereview.chromium.org/273523004/diff/90001/components/test/data/pas... components/test/data/password_manager/environment.py:11: """Handles the testing Environment. """ I don't understand what is meant by "Handles" here. Maybe "Sets up"? https://codereview.chromium.org/273523004/diff/90001/components/test/data/pas... components/test/data/password_manager/environment.py:29: print "user-data-dir=%s" % profile_path Please do not leave debugging statements in the code to be committed. Also on other places in the scripts. https://codereview.chromium.org/273523004/diff/90001/components/test/data/pas... components/test/data/password_manager/environment.py:31: self.driver = webdriver.Chrome(chromedriver_path, 0, options) optional nit: would be nice to explain the "0" passed to webdriver. It's not necessary, but could be helpful later. https://codereview.chromium.org/273523004/diff/90001/components/test/data/pas... components/test/data/password_manager/environment.py:51: name: The name of the website. There is no 'name' among the arguments. Did you mean 'website'? Also note that it is not clear what a "name of a website" is. Is that URL? Or just some user-defined identifier to use during the tests? https://codereview.chromium.org/273523004/diff/90001/components/test/data/pas... components/test/data/password_manager/environment.py:52: username_not_auto: Don't check that the state of the DOM is equal to the This is also not in the arguments list. https://codereview.chromium.org/273523004/diff/90001/components/test/data/pas... components/test/data/password_manager/environment.py:54: failing: Test is expected to fail. Does this rather mean that the test should not be run, because it fails? In that case, I propose renaming 'failing' to 'disabled'. The concept of disabled tests as test currently failing and not being run is widely known within Chromium. Instead, 'failing', would imply that the test should be run and should fail, and if it runs and does not fail, then something is wrong. I don't think that's the case, but please correct me if I'm wrong. https://codereview.chromium.org/273523004/diff/90001/components/test/data/pas... components/test/data/password_manager/environment.py:56: Returns: The function does not appear to return anything. https://codereview.chromium.org/273523004/diff/90001/components/test/data/pas... components/test/data/password_manager/environment.py:57: A new new Website. typo: new new https://codereview.chromium.org/273523004/diff/90001/components/test/data/pas... components/test/data/password_manager/environment.py:61: if self.passwords_tree != None: I believe you can leave out '!= None', as 'None' translates to 'false' in conditions. Same also below, including 'X == None' replaced with 'not X'. https://codereview.chromium.org/273523004/diff/90001/components/test/data/pas... components/test/data/password_manager/environment.py:76: if failing == False: 'failing == False' -> 'not failing' https://codereview.chromium.org/273523004/diff/90001/components/test/data/pas... components/test/data/password_manager/environment.py:90: urls = self.GetURLs() I think you can pull urls = self.GetURLs() before the for cycle, because it does not depend on 'website'. https://codereview.chromium.org/273523004/diff/90001/components/test/data/pas... components/test/data/password_manager/environment.py:99: arr = self.driver.find_elements_by_css_selector( What does 'arr' mean? Could you please use something more descriptive? For example: deletable_passwords? https://codereview.chromium.org/273523004/diff/90001/components/test/data/pas... components/test/data/password_manager/environment.py:111: # One of work-around is to go to a website, insert a link that is going nit: One of -> One https://codereview.chromium.org/273523004/diff/90001/components/test/data/pas... components/test/data/password_manager/environment.py:115: # Links for local resources (chrome://*) are blocked. So this why the nit: this -> this is https://codereview.chromium.org/273523004/diff/90001/components/test/data/pas... components/test/data/password_manager/environment.py:117: # via chromedriver to go to chrome://settings/passwords . Seems out of date: this function does not open chrome://settings/passwords. You can still keep the explanation for why the new tab starts with google.com, though. https://codereview.chromium.org/273523004/diff/90001/components/test/data/pas... components/test/data/password_manager/environment.py:119: "var d=document;var " Please make the line wrapping be one line per statement, if possible. And please also use spaces around binary operators, to make reading the JavaScript easier.
Hi Riadh, On 2014/05/14 15:05:15, rchtara wrote: > Hi, > I updated the code to support the password internals page. > Could you please check that too. I'll take a look during the next pass. Cheers, Vaclav > Thanks a lot > Cheers > Riadh
Hi Vaclav, Awesome, it's enough work for me for a while. So, I'm going to start today Don't worry because you didn't reviewing the whole cl Thanks a lot Cheers Riadh
Hi Vacalv, Could you please finish reviewing the cl? Thanks a lot Cheers Riadh https://codereview.chromium.org/273523004/diff/90001/components/test/data/pas... File components/test/data/password_manager/README (right): https://codereview.chromium.org/273523004/diff/90001/components/test/data/pas... components/test/data/password_manager/README:43: python tests.py --chrome-path CHROMEPATH --chromedriver-path CHROMEDRIVERPATH On 2014/05/14 15:16:09, vabr (Chromium) wrote: > Is this command the same as the one for "Run all the working tests tests by > executing"? Done. https://codereview.chromium.org/273523004/diff/90001/components/test/data/pas... components/test/data/password_manager/README:74: class NEWWEBSITE(Website): On 2014/05/14 15:16:09, vabr (Chromium) wrote: > Using all-caps for class name is against the Python style, and should not be > present in a code example. Done. https://codereview.chromium.org/273523004/diff/90001/components/test/data/pas... components/test/data/password_manager/README:77: self.GoTo("hhtp://url") On 2014/05/14 15:16:09, vabr (Chromium) wrote: > hhtp->http Done. https://codereview.chromium.org/273523004/diff/90001/components/test/data/pas... components/test/data/password_manager/README:78: self.FillUsername("#username") On 2014/05/14 15:16:09, vabr (Chromium) wrote: > What do you actually mean by #username, #password? > I assume that those are CSS selectors for the elements, but it would be great to > state it explicitly (it looks like username and password values instead), and be > precise about what exactly can be used. (For example, anything from > http://www.w3schools.com/cssref/css_selectors.asp ?) Done. https://codereview.chromium.org/273523004/diff/90001/components/test/data/pas... components/test/data/password_manager/README:86: Yahoo("NEWWEBSITE("http://url", username_not_auto=True)) On 2014/05/14 15:16:09, vabr (Chromium) wrote: > There is an odd number (3) of quotation marks in this expression, and one more > opening parenthesis than there are closing parentheses. Did you mean to remove > 'Yahoo("'? Done. https://codereview.chromium.org/273523004/diff/90001/components/test/data/pas... components/test/data/password_manager/README:93: * ClickIfAvailable: find an element using CSS Selector and if it's available, On 2014/05/14 15:16:09, vabr (Chromium) wrote: > Please elaborate on what's the difference between Click and ClickIfAvailable. > I assume Click throws an exception if the element is not available? > Also, please specify what you mean by "available" (present in the DOM of the > page? visible?). Done. https://codereview.chromium.org/273523004/diff/90001/components/test/data/pas... components/test/data/password_manager/README:97: self.Enter("css_selector") On 2014/05/14 15:16:09, vabr (Chromium) wrote: > nit: Enter -> SendEnterTo > (The literal meaning of "enter something" seems to be too strongly competing > with the intended abbreviation. In other words, this sounds like "css_selector" > should be entered somewhere in the page.) Done. https://codereview.chromium.org/273523004/diff/90001/components/test/data/pas... components/test/data/password_manager/README:98: * GoTo: go to some url. On 2014/05/14 15:16:09, vabr (Chromium) wrote: > What does "go to" mean? Navigating the main frame? Done. https://codereview.chromium.org/273523004/diff/90001/components/test/data/pas... components/test/data/password_manager/README:100: * Hover: find an element using CSS Selector and hover it. On 2014/05/14 15:16:09, vabr (Chromium) wrote: > hover it -> hover over it Done. https://codereview.chromium.org/273523004/diff/90001/components/test/data/pas... components/test/data/password_manager/README:101: self.Hover("css_selector") On 2014/05/14 15:16:09, vabr (Chromium) wrote: > optional nit: Hover -> HoverOver Done. https://codereview.chromium.org/273523004/diff/90001/components/test/data/pas... components/test/data/password_manager/README:102: On 2014/05/14 15:16:09, vabr (Chromium) wrote: > Why the blank line? Unless it separates different groups of methods, please > remove it (or put a blank line between every pair). Ditto below. Done. https://codereview.chromium.org/273523004/diff/90001/components/test/data/pas... components/test/data/password_manager/README:105: * Wait: wait for some amount of time. On 2014/05/14 15:16:09, vabr (Chromium) wrote: > Please mention units. (Seconds?) Done. https://codereview.chromium.org/273523004/diff/90001/components/test/data/pas... components/test/data/password_manager/README:112: * FillPassword: find an input element using CSS Selector and fill it with On 2014/05/14 15:16:09, vabr (Chromium) wrote: > nit: FillPasswordInto, FillUsernameInto > > Just FillPassword, FillUsername hint that the password and username to be filled > are the arguments of those functions. Done. https://codereview.chromium.org/273523004/diff/90001/components/test/data/pas... components/test/data/password_manager/README:118: * OptionalFillUsername: find an input element using CSS Selector and fill it On 2014/05/14 15:16:09, vabr (Chromium) wrote: > nit: OptionalFillUsername -> FillUsernameIfVisible > > Just saying Optional does not clarify when the username is filled and when no. > (no need to add *Into, because the argument is now more tied to IfVisible) Done. https://codereview.chromium.org/273523004/diff/90001/components/test/data/pas... components/test/data/password_manager/README:119: with the username. Do nothing if the input element is not visible. On 2014/05/14 15:16:09, vabr (Chromium) wrote: > Is "doing nothing" the important difference? > Or would FillUsernameInto actually throw an exception, while > FillUsernameIfVisible won't? Please clarify in the comment. Done. https://codereview.chromium.org/273523004/diff/90001/components/test/data/pas... components/test/data/password_manager/README:121: * Submit: find an element using CSS Selector and submit it. On 2014/05/14 15:16:09, vabr (Chromium) wrote: > optional nit: submit it -> call its submit() handler > > (Again, submit X literally means taking X and putting it somewhere, whereas the > submit() method of a form means submitting the content of the form, not the form > itself. But I'm probably being language paranoid :), that's why it's just > optional. :)) Done. https://codereview.chromium.org/273523004/diff/90001/components/test/data/pas... File components/test/data/password_manager/environment.py (right): https://codereview.chromium.org/273523004/diff/90001/components/test/data/pas... components/test/data/password_manager/environment.py:11: """Handles the testing Environment. """ On 2014/05/14 15:16:09, vabr (Chromium) wrote: > I don't understand what is meant by "Handles" here. > Maybe "Sets up"? Done. https://codereview.chromium.org/273523004/diff/90001/components/test/data/pas... components/test/data/password_manager/environment.py:29: print "user-data-dir=%s" % profile_path On 2014/05/14 15:16:09, vabr (Chromium) wrote: > Please do not leave debugging statements in the code to be committed. > Also on other places in the scripts. Done. https://codereview.chromium.org/273523004/diff/90001/components/test/data/pas... components/test/data/password_manager/environment.py:31: self.driver = webdriver.Chrome(chromedriver_path, 0, options) On 2014/05/14 15:16:09, vabr (Chromium) wrote: > optional nit: would be nice to explain the "0" passed to webdriver. It's not > necessary, but could be helpful later. Done. https://codereview.chromium.org/273523004/diff/90001/components/test/data/pas... components/test/data/password_manager/environment.py:51: name: The name of the website. On 2014/05/14 15:16:09, vabr (Chromium) wrote: > There is no 'name' among the arguments. Did you mean 'website'? > Also note that it is not clear what a "name of a website" is. Is that URL? Or > just some user-defined identifier to use during the tests? Done. https://codereview.chromium.org/273523004/diff/90001/components/test/data/pas... components/test/data/password_manager/environment.py:52: username_not_auto: Don't check that the state of the DOM is equal to the On 2014/05/14 15:16:09, vabr (Chromium) wrote: > This is also not in the arguments list. Done. https://codereview.chromium.org/273523004/diff/90001/components/test/data/pas... components/test/data/password_manager/environment.py:54: failing: Test is expected to fail. On 2014/05/14 15:16:09, vabr (Chromium) wrote: > Does this rather mean that the test should not be run, because it fails? > In that case, I propose renaming 'failing' to 'disabled'. The concept of > disabled tests as test currently failing and not being run is widely known > within Chromium. Instead, 'failing', would imply that the test should be run and > should fail, and if it runs and does not fail, then something is wrong. I don't > think that's the case, but please correct me if I'm wrong. Done. https://codereview.chromium.org/273523004/diff/90001/components/test/data/pas... components/test/data/password_manager/environment.py:56: Returns: On 2014/05/14 15:16:09, vabr (Chromium) wrote: > The function does not appear to return anything. Done. https://codereview.chromium.org/273523004/diff/90001/components/test/data/pas... components/test/data/password_manager/environment.py:57: A new new Website. On 2014/05/14 15:16:09, vabr (Chromium) wrote: > typo: new new Done. https://codereview.chromium.org/273523004/diff/90001/components/test/data/pas... components/test/data/password_manager/environment.py:61: if self.passwords_tree != None: I got this warning when I tied to replace this None. /usr/local/google/home/rchtara/chrome/src/components/test/data/password_manager/environment.py:63: FutureWarning: The behavior of this method will change in future versions. Use specific 'len(elem)' or 'elem is not None' test instead. if self.passwords_tree: For all the others, I got nothing. So I replaced all the other and I kept only this one https://codereview.chromium.org/273523004/diff/90001/components/test/data/pas... components/test/data/password_manager/environment.py:76: if failing == False: On 2014/05/14 15:16:09, vabr (Chromium) wrote: > 'failing == False' -> 'not failing' Done. https://codereview.chromium.org/273523004/diff/90001/components/test/data/pas... components/test/data/password_manager/environment.py:90: urls = self.GetURLs() The urls list is changing every time. https://codereview.chromium.org/273523004/diff/90001/components/test/data/pas... components/test/data/password_manager/environment.py:99: arr = self.driver.find_elements_by_css_selector( On 2014/05/14 15:16:09, vabr (Chromium) wrote: > What does 'arr' mean? Could you please use something more descriptive? For > example: deletable_passwords? Done. https://codereview.chromium.org/273523004/diff/90001/components/test/data/pas... components/test/data/password_manager/environment.py:111: # One of work-around is to go to a website, insert a link that is going On 2014/05/14 15:16:09, vabr (Chromium) wrote: > nit: One of -> One Done. https://codereview.chromium.org/273523004/diff/90001/components/test/data/pas... components/test/data/password_manager/environment.py:115: # Links for local resources (chrome://*) are blocked. So this why the On 2014/05/14 15:16:09, vabr (Chromium) wrote: > nit: this -> this is Done. https://codereview.chromium.org/273523004/diff/90001/components/test/data/pas... components/test/data/password_manager/environment.py:117: # via chromedriver to go to chrome://settings/passwords . On 2014/05/14 15:16:09, vabr (Chromium) wrote: > Seems out of date: this function does not open chrome://settings/passwords. > You can still keep the explanation for why the new tab starts with http://google.com, > though. Done. https://codereview.chromium.org/273523004/diff/90001/components/test/data/pas... components/test/data/password_manager/environment.py:119: "var d=document;var " On 2014/05/14 15:16:09, vabr (Chromium) wrote: > Please make the line wrapping be one line per statement, if possible. And please > also use spaces around binary operators, to make reading the JavaScript easier. Done.
Hi Riadh. Good job so far, there are many things I really liked in your CL! I added some more comments for the places which still need improvement. I'm happy to explain in person if you find some of the comments unclear or impossible to address. Cheers, Vaclav https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... File components/test/data/password_manager/environment.py (right): https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/environment.py:31: self.Log(e) This deservers a comment on why is it OK to proceed even if rmtree() fails, and what could be the consequences. You can actually mention the consequences in the log message. https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/environment.py:34: options.add_argument("enable-password-manager-internals-ui") nit: Since yesterday, this is no more needed. :) https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/environment.py:57: # The number of the save operations done through all the tests. nit: "through all" contains 2 spaces instead of 1 https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/environment.py:68: """Adds a Website to the testing Environment. Actually, what do you think of renaming Website to WebsiteTest? To me, WebsiteTest looks like a more precise description of what the class represents. If the class Website gets reamed to WebsiteTest, all the variables and method names related to it should also be renamed accordingly. https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/environment.py:77: if self.passwords_tree is not None: nit: Just use if self.passwords_tree: https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/environment.py:91: if disabled == False: nit: Just use: if not disabled: https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/environment.py:95: """Removes the stored passwords for all Websites. Why don't you just flush all the stored passwords, without involving the websites one after each other, and letting them delete what they hope is their password entry? The current way looks unnecessarily fragile and complex. https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/environment.py:120: def OpenTabAndGoInternal(self): nit: Please rename to OpenTabAndGoToInternals ("go internal" sounds confusing) https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/environment.py:121: """Opens a new tab and navigates to passwords internals page in the first The comment does not seem to match the code exactly: If there is a self.websitewindow, nothing happens? Is that expected? Then the comment should say so. If that's not expected (i.e., this method should not be called when self.websitewindow exists), then please make it an error. https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/environment.py:124: self.driver.get("https://google.com") Would chrome://newtab work instead of google.com? Every time we depend on an external page loading, we introduce a source of flakiness. In particular here the flakiness comes in the form of the time.sleep(1) -- if the network hiccups, 1 second may be too short for google.com, and the test is screwed. https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/environment.py:140: time.sleep(1) It's a bit unfortunate that this sleep() does not contribute to the max_duration decrement of the calling Website here and in CheckPromptIsNotShown. You could make OpenTabAndGoInternal and CheckPromptIsNotShown return the total time spent waiting, and let the Website subtract that from its max_duration, and check the result is still positive. https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/environment.py:155: """Checks if the prompt was shown: If the prompt is shown when it shouldn't nit: CheckPromptIsNotShown ... Checks if the prompt was shown That's a bit confusing: was not shown or was shown? Also, the function name indicates "not shown" but the argument |expected| allows to check for both "not shown" and "shown". So I suggest renaming the method to just CheckPrompt, |expected| to |prompt_should_show_up|, and start the comment with something like "Detects whether the save password prompt is shown, and raises an exception if the result does not match the expectation." https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/environment.py:163: wait and execute itself recursively until the time will end. This is not clear (and also does not mention the time units again). I suggest replacing the "In case...will end" sentence with "The method checks periodically during the first |timeout| seconds if the internals page reports the prompt being shown. If the prompt is not reported shown within the first |timeout| seconds, it is considered not shown at all." In general, I also recommend making the units part of the variable; here it would mean something like |timeout_sec|. https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/environment.py:171: count = log.text.count("Message: Decision: SAVE the password") This is not correct. The message actually says that the prompt was not shown, instead the password was saved automatically. Now, I understand that that's because of the --enable-automatic-password-saving flag you passed. But this way we won't find out if the prompt would actually be shown without the flag. For example, if there was a bug and Chrome started to save everything automatically, the test would no know. Do you think you could actually split the tests, and do on run with --enable-automatic-password-saving, and one without? In the run with the flag, you can ignore whether the prompt is shown. In the run without the flag, you can only test if the prompt is shown as expected (the message to look for is then "ASK the user"), and disregard the rest of the tests. https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/environment.py:180: def Log(self, text): Please use the standard Python logging support instead of this: https://docs.python.org/2/howto/logging.html https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/environment.py:191: """Runs the tests on many Websites. nit: "many Websites" is unnecessarily vague. Just say "|websites|". https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... File components/test/data/password_manager/tests.py (right): https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/tests.py:12: def WorkingTests(environment): Since you specify "disabled" in AddWebsite(), you don't have to separate adding the enabled and disabled tests into two functions. Please merge the two functions. https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/tests.py:15: class Amazon(Website): Please extract the class definitions out of the function, into the global space. Only the AddWebsite calls should stay in the function. https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/tests.py:63: Espn( nit: This line-break seems unnecessary. https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/tests.py:86: nit: Keep in mind the style guide requires 2 blank lines between class definitions. http://legacy.python.org/dev/peps/pep-0008/#blank-lines https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/tests.py:234: "browse?qsrc=321&q=&o=0&l=dir#")) nit: indenting is off https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/tests.py:334: # Tests that can cause a crash (the cause of the crash is not related to the Are you really sure that the crash is not related to the password manager? If yes, then fine, but please be careful about making such strong statements. https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/tests.py:395: if "--help" in args: Please use argparse instead of handling the arguments yourself. https://docs.python.org/2/library/argparse.html#module-argparse (sample code, e.g., here: http://chromegw/viewvc/chrome-internal/trunk/tools/py_issue_tracker/src/crbug...) https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/tests.py:395: if "--help" in args: Also, in general, help should be printed if the arguments are not recognised, instead of just when the argument is exactly --help. https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/tests.py:396: print "Password Manager automated tests help." nit: Consider using multi-line strings """...""" instead of repeating print on each line. https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... File components/test/data/password_manager/website.py (right): https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/website.py:13: def _IsOneSubstringOfAnother(s1, s2): Please add a doc string, explaining what this function does. https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/website.py:23: Autofilled = 1 Please use all caps for consistency with the C++ enum naming: AUTOFILLED NOT_AUTOFILLED https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/website.py:31: self, name, url, username=None, password=None, Please remove |url|. You don't seem to use it, and it's always the same URL which is passed as the GoTo() argument in the login steps. https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/website.py:31: self, name, url, username=None, password=None, Are you actually using the username and password arguments? If not, then please remove them. https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/website.py:57: self.username_not_auto = username_not_auto For pages like Wikipedia, where the username is not autofilled, because the page fills in some trash, we still could do better in our test: Ideally, we would clear the username input, and overwrite it with the correct username. After that Chrome should autofill the password, and we should check that it did so correctly. Currently we give up, and fill the password ourselves, which leaves out important things to check. https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/website.py:60: # Waiting duration before stopping the test. Note: max_duration limits the total time spent in waiting, but does not include the time spent running. You might want to make this clear in the comment. Also, since max_duration gets decremented, a more appropriate name would be: remaining_time_to_wait or something similar. https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/website.py:80: """Clicks on an element, if it's available. So, is this about visibility, or availability? If the element is there, but has 'display: none' in its CSS, will it be clicked? https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/website.py:90: return False The method returns False if the element is not clicked, but does not return anything when it is clicked. Please unify this. It looks like you don't use the return value, so don't return False. Use just "return" or "pass". https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/website.py:95: """Navigates the main frame to a url. nit: "a url" -> "|url|" (It does not navigate to just some url, it navigates to the url specified in |url|.) https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/website.py:142: """Wait for a duration. Please specify time units. https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/website.py:161: time.sleep(1) Please call Wait() instead of manually decrementing and checking max_duration below. https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/website.py:196: # tried too, but because the password is sometimes hidden, this didn't I'm not sure I understand the issue here -- if the password input is hidden, then how were the user expected to see it? This looks like the test should fail in that case. Also, to unblock the password, user interaction with the page is needed, not necessarily with the password form. Interacting with the root of the DOM tree should work as well. It is crucial that we use the autofilled password for login, without retyping the password again, as that would mask bugs we need to watch out for. https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/website.py:197: # worked out. nit: worked -> work https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/website.py:199: ps = password_element.get_attribute("value")[:-1] Pleas do not use cryptic abbreviations as names of variables. This looks like the autofilled password, so |autofilled_password| might be an appropriate name. https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/website.py:206: password_element.get_attribute("value"), nit: indenting is off https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/website.py:215: ps = password_element.get_attribute("value")[1:] Again, please rename |ps| appropriately. https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/website.py:217: password_element.send_keys(ps) Why do you send |ps| to password_element here, and why do you re-send "a" to password_element below? https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/website.py:232: def FillUsernameInto(self, selector): In the concrete Websites, FillUsernameInto and FillPasswordInto is always used as a sequence: FillUsernameInto(...) FillPasswordInto(...) To make the code clearer, these two public methods should be replaced by one FillUsernameAndPasswordInto(), which can then call two private (name starting with "_") methods for filling the username and the password. https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/website.py:254: username_element.clear() Should you check that either self.username_not_auto is true, or username_element is empty, before you clear it? https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/website.py:258: """Fills the input with the website username, if the input exists. Please comment on ignoring self.mode here. https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/website.py:309: if (self.url != ""): nit: Just use if self.url: https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/website.py:313: self.driver.execute_script( Please comment on what the script does. In particular mention that it deletes the password entry from the list, and that therefore we don't increase |i| in this case. https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/website.py:315: ".row-delete-button')[%d].click()" % i) Actually, I don't think you use |i| correctly here. Imagine that another Website just removed its passwords. Then the passwords the other Website deleted are still included in |urls|, so you do increment |i| for them, although they are no longer in the list. That shifts your index in a wrong state, doesn't it? https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/website.py:316: time.sleep(1) # Wait until command is executed. Please use Wait(). https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/website.py:334: self.password = correct_password Why do you not Wait() here, but you do wait at the same place in SuccessfullLoginTest below? https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/website.py:353: time.sleep(2) Please use Wait(). https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/website.py:373: time.sleep(2) Please use Wait(). https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/website.py:382: def SuccessfulLoginAfterDeletionTest(self): This does exactly the same thing as SuccessfulLoginTest (except it does not Wait(), which looks like a mistake). Please remove this method and use SuccessfulLoginTest instead.
Hi vacalv, Thanks a lot for the review. Cheers Riadh On Fri, May 16, 2014 at 11:36 AM, <vabr@chromium.org> wrote: > Hi Riadh. > > Good job so far, there are many things I really liked in your CL! > > I added some more comments for the places which still need improvement. I'm > happy to explain in person if you find some of the comments unclear or > impossible to address. > > Cheers, > Vaclav > > > https://codereview.chromium.org/273523004/diff/210001/ > components/test/data/password_manager/environment.py > File components/test/data/password_manager/environment.py (right): > > https://codereview.chromium.org/273523004/diff/210001/ > components/test/data/password_manager/environment.py#newcode31 > components/test/data/password_manager/environment.py:31: self.Log(e) > This deservers a comment on why is it OK to proceed even if rmtree() > fails, and what could be the consequences. > You can actually mention the consequences in the log message. > > https://codereview.chromium.org/273523004/diff/210001/ > components/test/data/password_manager/environment.py#newcode34 > components/test/data/password_manager/environment.py:34: > options.add_argument("enable-password-manager-internals-ui") > nit: Since yesterday, this is no more needed. :) > > https://codereview.chromium.org/273523004/diff/210001/ > components/test/data/password_manager/environment.py#newcode57 > components/test/data/password_manager/environment.py:57: # The number of > the save operations done through all the tests. > nit: "through all" contains 2 spaces instead of 1 > > https://codereview.chromium.org/273523004/diff/210001/ > components/test/data/password_manager/environment.py#newcode68 > components/test/data/password_manager/environment.py:68: """Adds a > Website to the testing Environment. > Actually, what do you think of renaming Website to WebsiteTest? > To me, WebsiteTest looks like a more precise description of what the > class represents. > If the class Website gets reamed to WebsiteTest, all the variables and > method names related to it should also be renamed accordingly. > > https://codereview.chromium.org/273523004/diff/210001/ > components/test/data/password_manager/environment.py#newcode77 > components/test/data/password_manager/environment.py:77: if > self.passwords_tree is not None: > nit: Just use > if self.passwords_tree: > > https://codereview.chromium.org/273523004/diff/210001/ > components/test/data/password_manager/environment.py#newcode91 > components/test/data/password_manager/environment.py:91: if disabled == > False: > nit: Just use: > if not disabled: > > https://codereview.chromium.org/273523004/diff/210001/ > components/test/data/password_manager/environment.py#newcode95 > components/test/data/password_manager/environment.py:95: """Removes the > stored passwords for all Websites. > Why don't you just flush all the stored passwords, without involving the > websites one after each other, and letting them delete what they hope is > their password entry? The current way looks unnecessarily fragile and > complex. > > https://codereview.chromium.org/273523004/diff/210001/ > components/test/data/password_manager/environment.py#newcode120 > components/test/data/password_manager/environment.py:120: def > OpenTabAndGoInternal(self): > nit: Please rename to OpenTabAndGoToInternals > ("go internal" sounds confusing) > > https://codereview.chromium.org/273523004/diff/210001/ > components/test/data/password_manager/environment.py#newcode121 > components/test/data/password_manager/environment.py:121: """Opens a new > tab and navigates to passwords internals page in the first > The comment does not seem to match the code exactly: > If there is a self.websitewindow, nothing happens? > Is that expected? Then the comment should say so. > If that's not expected (i.e., this method should not be called when > self.websitewindow exists), then please make it an error. > > https://codereview.chromium.org/273523004/diff/210001/ > components/test/data/password_manager/environment.py#newcode124 > components/test/data/password_manager/environment.py:124: > self.driver.get("https://google.com") > Would chrome://newtab work instead of google.com? > Every time we depend on an external page loading, we introduce a source > of flakiness. In particular here the flakiness comes in the form of the > time.sleep(1) -- if the network hiccups, 1 second may be too short for > google.com, and the test is screwed. > > https://codereview.chromium.org/273523004/diff/210001/ > components/test/data/password_manager/environment.py#newcode140 > components/test/data/password_manager/environment.py:140: time.sleep(1) > It's a bit unfortunate that this sleep() does not contribute to the > max_duration decrement of the calling Website here and in > CheckPromptIsNotShown. > > You could make OpenTabAndGoInternal and CheckPromptIsNotShown return the > total time spent waiting, and let the Website subtract that from its > max_duration, and check the result is still positive. > > https://codereview.chromium.org/273523004/diff/210001/ > components/test/data/password_manager/environment.py#newcode155 > components/test/data/password_manager/environment.py:155: """Checks if > the prompt was shown: If the prompt is shown when it shouldn't > nit: CheckPromptIsNotShown ... Checks if the prompt was shown > That's a bit confusing: was not shown or was shown? > Also, the function name indicates "not shown" but the argument > |expected| allows to check for both "not shown" and "shown". > So I suggest renaming the method to just CheckPrompt, |expected| to > |prompt_should_show_up|, and start the comment with something like > "Detects whether the save password prompt is shown, and raises an > exception if the result does not match the expectation." > > https://codereview.chromium.org/273523004/diff/210001/ > components/test/data/password_manager/environment.py#newcode163 > components/test/data/password_manager/environment.py:163: wait and > execute itself recursively until the time will end. > This is not clear (and also does not mention the time units again). I > suggest replacing the "In case...will end" sentence with > "The method checks periodically during the first |timeout| seconds if > the internals page reports the prompt being shown. If the prompt is not > reported shown within the first |timeout| seconds, it is considered not > shown at all." > > In general, I also recommend making the units part of the variable; here > it would mean something like |timeout_sec|. > > https://codereview.chromium.org/273523004/diff/210001/ > components/test/data/password_manager/environment.py#newcode171 > components/test/data/password_manager/environment.py:171: count = > log.text.count("Message: Decision: SAVE the password") > This is not correct. > The message actually says that the prompt was not shown, instead the > password was saved automatically. > > Now, I understand that that's because of the > --enable-automatic-password-saving flag you passed. But this way we > won't find out if the prompt would actually be shown without the flag. > For example, if there was a bug and Chrome started to save everything > automatically, the test would no know. > > Do you think you could actually split the tests, and do on run with > --enable-automatic-password-saving, and one without? In the run with the > flag, you can ignore whether the prompt is shown. In the run without the > flag, you can only test if the prompt is shown as expected (the message > to look for is then "ASK the user"), and disregard the rest of the > tests. > > https://codereview.chromium.org/273523004/diff/210001/ > components/test/data/password_manager/environment.py#newcode180 > components/test/data/password_manager/environment.py:180: def Log(self, > text): > Please use the standard Python logging support instead of this: > https://docs.python.org/2/howto/logging.html > > https://codereview.chromium.org/273523004/diff/210001/ > components/test/data/password_manager/environment.py#newcode191 > components/test/data/password_manager/environment.py:191: """Runs the > tests on many Websites. > nit: "many Websites" is unnecessarily vague. Just say "|websites|". > > https://codereview.chromium.org/273523004/diff/210001/ > components/test/data/password_manager/tests.py > File components/test/data/password_manager/tests.py (right): > > https://codereview.chromium.org/273523004/diff/210001/ > components/test/data/password_manager/tests.py#newcode12 > components/test/data/password_manager/tests.py:12: def > WorkingTests(environment): > Since you specify "disabled" in AddWebsite(), you don't have to separate > adding the enabled and disabled tests into two functions. Please merge > the two functions. > > https://codereview.chromium.org/273523004/diff/210001/ > components/test/data/password_manager/tests.py#newcode15 > components/test/data/password_manager/tests.py:15: class > Amazon(Website): > Please extract the class definitions out of the function, into the > global space. Only the AddWebsite calls should stay in the function. > > https://codereview.chromium.org/273523004/diff/210001/ > components/test/data/password_manager/tests.py#newcode63 > components/test/data/password_manager/tests.py:63: Espn( > nit: This line-break seems unnecessary. > > https://codereview.chromium.org/273523004/diff/210001/ > components/test/data/password_manager/tests.py#newcode86 > components/test/data/password_manager/tests.py:86: > nit: Keep in mind the style guide requires 2 blank lines between class > definitions. > http://legacy.python.org/dev/peps/pep-0008/#blank-lines > > https://codereview.chromium.org/273523004/diff/210001/ > components/test/data/password_manager/tests.py#newcode234 > components/test/data/password_manager/tests.py:234: > "browse?qsrc=321&q=&o=0&l=dir#")) > nit: indenting is off > > https://codereview.chromium.org/273523004/diff/210001/ > components/test/data/password_manager/tests.py#newcode334 > components/test/data/password_manager/tests.py:334: # Tests that can > > cause a crash (the cause of the crash is not related to the > Are you really sure that the crash is not related to the password > manager? If yes, then fine, but please be careful about making such > strong statements. > > https://codereview.chromium.org/273523004/diff/210001/ > components/test/data/password_manager/tests.py#newcode395 > components/test/data/password_manager/tests.py:395: if "--help" in args: > Please use argparse instead of handling the arguments yourself. > https://docs.python.org/2/library/argparse.html#module-argparse > > (sample code, e.g., here: > http://chromegw/viewvc/chrome-internal/trunk/tools/py_issue_ > tracker/src/crbugtotrix.py?view=markup) > > https://codereview.chromium.org/273523004/diff/210001/ > components/test/data/password_manager/tests.py#newcode395 > components/test/data/password_manager/tests.py:395: if "--help" in args: > Also, in general, help should be printed if the arguments are not > recognised, instead of just when the argument is exactly --help. > > https://codereview.chromium.org/273523004/diff/210001/ > components/test/data/password_manager/tests.py#newcode396 > components/test/data/password_manager/tests.py:396: print "Password > Manager automated tests help." > nit: Consider using multi-line strings """...""" instead of repeating > print on each line. > > https://codereview.chromium.org/273523004/diff/210001/ > components/test/data/password_manager/website.py > File components/test/data/password_manager/website.py (right): > > https://codereview.chromium.org/273523004/diff/210001/ > components/test/data/password_manager/website.py#newcode13 > components/test/data/password_manager/website.py:13: def > _IsOneSubstringOfAnother(s1, s2): > Please add a doc string, explaining what this function does. > > https://codereview.chromium.org/273523004/diff/210001/ > components/test/data/password_manager/website.py#newcode23 > components/test/data/password_manager/website.py:23: Autofilled = 1 > Please use all caps for consistency with the C++ enum naming: > AUTOFILLED > NOT_AUTOFILLED > > https://codereview.chromium.org/273523004/diff/210001/ > components/test/data/password_manager/website.py#newcode31 > components/test/data/password_manager/website.py:31: self, name, url, > username=None, password=None, > Please remove |url|. You don't seem to use it, and it's always the same > URL which is passed as the GoTo() argument in the login steps. > > https://codereview.chromium.org/273523004/diff/210001/ > components/test/data/password_manager/website.py#newcode31 > components/test/data/password_manager/website.py:31: self, name, url, > username=None, password=None, > Are you actually using the username and password arguments? If not, then > please remove them. > > https://codereview.chromium.org/273523004/diff/210001/ > components/test/data/password_manager/website.py#newcode57 > components/test/data/password_manager/website.py:57: > self.username_not_auto = username_not_auto > For pages like Wikipedia, where the username is not autofilled, because > the page fills in some trash, we still could do better in our test: > > Ideally, we would clear the username input, and overwrite it with the > correct username. After that Chrome should autofill the password, and we > should check that it did so correctly. > > Currently we give up, and fill the password ourselves, which leaves out > important things to check. > > https://codereview.chromium.org/273523004/diff/210001/ > components/test/data/password_manager/website.py#newcode60 > components/test/data/password_manager/website.py:60: # Waiting duration > before stopping the test. > Note: max_duration limits the total time spent in waiting, but does not > include the time spent running. You might want to make this clear in the > comment. > > Also, since max_duration gets decremented, a more appropriate name would > be: remaining_time_to_wait or something similar. > > https://codereview.chromium.org/273523004/diff/210001/ > components/test/data/password_manager/website.py#newcode80 > components/test/data/password_manager/website.py:80: """Clicks on an > element, if it's available. > So, is this about visibility, or availability? > If the element is there, but has 'display: none' in its CSS, will it be > clicked? > > https://codereview.chromium.org/273523004/diff/210001/ > components/test/data/password_manager/website.py#newcode90 > components/test/data/password_manager/website.py:90: return False > The method returns False if the element is not clicked, but does not > return anything when it is clicked. > > Please unify this. It looks like you don't use the return value, so > don't return False. Use just "return" or "pass". > > https://codereview.chromium.org/273523004/diff/210001/ > components/test/data/password_manager/website.py#newcode95 > components/test/data/password_manager/website.py:95: """Navigates the > main frame to a url. > nit: "a url" -> "|url|" > (It does not navigate to just some url, it navigates to the url > specified in |url|.) > > https://codereview.chromium.org/273523004/diff/210001/ > components/test/data/password_manager/website.py#newcode142 > components/test/data/password_manager/website.py:142: """Wait for a > duration. > Please specify time units. > > https://codereview.chromium.org/273523004/diff/210001/ > components/test/data/password_manager/website.py#newcode161 > components/test/data/password_manager/website.py:161: time.sleep(1) > Please call Wait() instead of manually decrementing and checking > max_duration below. > > https://codereview.chromium.org/273523004/diff/210001/ > components/test/data/password_manager/website.py#newcode196 > components/test/data/password_manager/website.py:196: # tried too, but > because the password is sometimes hidden, this didn't > I'm not sure I understand the issue here -- if the password input is > hidden, then how were the user expected to see it? This looks like the > test should fail in that case. > > Also, to unblock the password, user interaction with the page is needed, > not necessarily with the password form. Interacting with the root of the > DOM tree should work as well. > > It is crucial that we use the autofilled password for login, without > retyping the password again, as that would mask bugs we need to watch > out for. > > https://codereview.chromium.org/273523004/diff/210001/ > components/test/data/password_manager/website.py#newcode197 > components/test/data/password_manager/website.py:197: # worked out. > nit: worked -> work > > https://codereview.chromium.org/273523004/diff/210001/ > components/test/data/password_manager/website.py#newcode199 > components/test/data/password_manager/website.py:199: ps = > password_element.get_attribute("value")[:-1] > Pleas do not use cryptic abbreviations as names of variables. > This looks like the autofilled password, so |autofilled_password| might > be an appropriate name. > > https://codereview.chromium.org/273523004/diff/210001/ > components/test/data/password_manager/website.py#newcode206 > components/test/data/password_manager/website.py:206: > password_element.get_attribute("value"), > nit: indenting is off > > https://codereview.chromium.org/273523004/diff/210001/ > components/test/data/password_manager/website.py#newcode215 > components/test/data/password_manager/website.py:215: ps = > password_element.get_attribute("value")[1:] > Again, please rename |ps| appropriately. > > https://codereview.chromium.org/273523004/diff/210001/ > components/test/data/password_manager/website.py#newcode217 > components/test/data/password_manager/website.py:217: > password_element.send_keys(ps) > Why do you send |ps| to password_element here, and why do you re-send > "a" to password_element below? > > https://codereview.chromium.org/273523004/diff/210001/ > components/test/data/password_manager/website.py#newcode232 > components/test/data/password_manager/website.py:232: def > FillUsernameInto(self, selector): > In the concrete Websites, FillUsernameInto and FillPasswordInto is > always used as a sequence: > FillUsernameInto(...) > FillPasswordInto(...) > > To make the code clearer, these two public methods should be replaced by > one FillUsernameAndPasswordInto(), which can then call two private (name > starting with "_") methods for filling the username and the password. > > https://codereview.chromium.org/273523004/diff/210001/ > components/test/data/password_manager/website.py#newcode254 > components/test/data/password_manager/website.py:254: > username_element.clear() > Should you check that either self.username_not_auto is true, or > username_element is empty, before you clear it? > > https://codereview.chromium.org/273523004/diff/210001/ > components/test/data/password_manager/website.py#newcode258 > components/test/data/password_manager/website.py:258: """Fills the input > with the website username, if the input exists. > Please comment on ignoring self.mode here. > > https://codereview.chromium.org/273523004/diff/210001/ > components/test/data/password_manager/website.py#newcode309 > components/test/data/password_manager/website.py:309: if (self.url != > ""): > nit: Just use > if self.url: > > https://codereview.chromium.org/273523004/diff/210001/ > components/test/data/password_manager/website.py#newcode313 > components/test/data/password_manager/website.py:313: > self.driver.execute_script( > Please comment on what the script does. > In particular mention that it deletes the password entry from the list, > and that therefore we don't increase |i| in this case. > > https://codereview.chromium.org/273523004/diff/210001/ > components/test/data/password_manager/website.py#newcode315 > components/test/data/password_manager/website.py:315: > ".row-delete-button')[%d].click()" % i) > Actually, I don't think you use |i| correctly here. > Imagine that another Website just removed its passwords. Then the > passwords the other Website deleted are still included in |urls|, so you > do increment |i| for them, although they are no longer in the list. That > shifts your index in a wrong state, doesn't it? > > https://codereview.chromium.org/273523004/diff/210001/ > components/test/data/password_manager/website.py#newcode316 > components/test/data/password_manager/website.py:316: time.sleep(1) # > Wait until command is executed. > Please use Wait(). > > https://codereview.chromium.org/273523004/diff/210001/ > components/test/data/password_manager/website.py#newcode334 > components/test/data/password_manager/website.py:334: self.password = > correct_password > Why do you not Wait() here, but you do wait at the same place in > SuccessfullLoginTest below? > > https://codereview.chromium.org/273523004/diff/210001/ > components/test/data/password_manager/website.py#newcode353 > components/test/data/password_manager/website.py:353: time.sleep(2) > Please use Wait(). > > https://codereview.chromium.org/273523004/diff/210001/ > components/test/data/password_manager/website.py#newcode373 > components/test/data/password_manager/website.py:373: time.sleep(2) > Please use Wait(). > > https://codereview.chromium.org/273523004/diff/210001/ > components/test/data/password_manager/website.py#newcode382 > components/test/data/password_manager/website.py:382: def > SuccessfulLoginAfterDeletionTest(self): > This does exactly the same thing as SuccessfulLoginTest (except it does > not Wait(), which looks like a mistake). Please remove this method and > use SuccessfulLoginTest instead. > > https://codereview.chromium.org/273523004/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Hi Vaclav, I updated the cl, could please check it? Thanks a lot Cheers Riadh https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... File components/test/data/password_manager/environment.py (right): https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/environment.py:31: self.Log(e) On 2014/05/16 09:36:00, vabr (Chromium) wrote: > This deservers a comment on why is it OK to proceed even if rmtree() fails, and > what could be the consequences. > You can actually mention the consequences in the log message. Done. https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/environment.py:57: # The number of the save operations done through all the tests. On 2014/05/16 09:36:00, vabr (Chromium) wrote: > nit: "through all" contains 2 spaces instead of 1 Done. https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/environment.py:68: """Adds a Website to the testing Environment. On 2014/05/16 09:36:00, vabr (Chromium) wrote: > Actually, what do you think of renaming Website to WebsiteTest? > To me, WebsiteTest looks like a more precise description of what the class > represents. > If the class Website gets reamed to WebsiteTest, all the variables and method > names related to it should also be renamed accordingly. Done. https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/environment.py:77: if self.passwords_tree is not None: I got this warning when I tied to replace this None. /usr/local/google/home/rchtara/chrome/src/components/test/data/password_manager/environment.py:63: FutureWarning: The behavior of this method will change in future versions. Use specific 'len(elem)' or 'elem is not None' test instead. if self.passwords_tree: For all the others, I got nothing. https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/environment.py:91: if disabled == False: On 2014/05/16 09:36:00, vabr (Chromium) wrote: > nit: Just use: > if not disabled: Done. https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/environment.py:95: """Removes the stored passwords for all Websites. On 2014/05/16 09:36:00, vabr (Chromium) wrote: > Why don't you just flush all the stored passwords, without involving the > websites one after each other, and letting them delete what they hope is their > password entry? The current way looks unnecessarily fragile and complex. Done. https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/environment.py:120: def OpenTabAndGoInternal(self): On 2014/05/16 09:36:00, vabr (Chromium) wrote: > nit: Please rename to OpenTabAndGoToInternals > ("go internal" sounds confusing) Done. https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/environment.py:121: """Opens a new tab and navigates to passwords internals page in the first On 2014/05/16 09:36:00, vabr (Chromium) wrote: > The comment does not seem to match the code exactly: > If there is a self.websitewindow, nothing happens? > Is that expected? Then the comment should say so. > If that's not expected (i.e., this method should not be called when > self.websitewindow exists), then please make it an error. Done. https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/environment.py:124: self.driver.get("https://google.com") I updated the test to go to the url of the first website. I think i was wrong about the new tab. If I use this script var a = document.createElement('a'); a.target = '_blank'; a.href = "chrome://newtab"; a.innerHTML = '.'; document.body.appendChild(a); a.click() The pop blocker blocks the tab. If I remove the last line. And the ask the driver to perform the click, every thing work fine https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/environment.py:140: time.sleep(1) I don't think it's make sense to do that.max_duration is related to only one site. And this sleep is made once for all tests. The duration of the tests is not something very important. It's not even shown. Some of the tests have an infinite loop in them, for example the following loop: *click on a menu button *wait for 1 second *if the submenu button is shown-> break *otherwise continue the loop. max_duration is only created to prevent infinite loop in this cases. https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/environment.py:155: """Checks if the prompt was shown: If the prompt is shown when it shouldn't On 2014/05/16 09:36:00, vabr (Chromium) wrote: > nit: CheckPromptIsNotShown ... Checks if the prompt was shown > That's a bit confusing: was not shown or was shown? > Also, the function name indicates "not shown" but the argument |expected| allows > to check for both "not shown" and "shown". > So I suggest renaming the method to just CheckPrompt, |expected| to > |prompt_should_show_up|, and start the comment with something like "Detects > whether the save password prompt is shown, and raises an exception if the result > does not match the expectation." Done. https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/environment.py:163: wait and execute itself recursively until the time will end. On 2014/05/16 09:36:00, vabr (Chromium) wrote: > This is not clear (and also does not mention the time units again). I suggest > replacing the "In case...will end" sentence with > "The method checks periodically during the first |timeout| seconds if the > internals page reports the prompt being shown. If the prompt is not reported > shown within the first |timeout| seconds, it is considered not shown at all." > > In general, I also recommend making the units part of the variable; here it > would mean something like |timeout_sec|. Done. https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/environment.py:171: count = log.text.count("Message: Decision: SAVE the password") On 2014/05/16 09:36:00, vabr (Chromium) wrote: > This is not correct. > The message actually says that the prompt was not shown, instead the password > was saved automatically. > > Now, I understand that that's because of the --enable-automatic-password-saving > flag you passed. But this way we won't find out if the prompt would actually be > shown without the flag. For example, if there was a bug and Chrome started to > save everything automatically, the test would no know. > > Do you think you could actually split the tests, and do on run with > --enable-automatic-password-saving, and one without? In the run with the flag, > you can ignore whether the prompt is shown. In the run without the flag, you can > only test if the prompt is shown as expected (the message to look for is then > "ASK the user"), and disregard the rest of the tests. Done. https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/environment.py:180: def Log(self, text): On 2014/05/16 09:36:00, vabr (Chromium) wrote: > Please use the standard Python logging support instead of this: > https://docs.python.org/2/howto/logging.html Done. https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/environment.py:191: """Runs the tests on many Websites. On 2014/05/16 09:36:00, vabr (Chromium) wrote: > nit: "many Websites" is unnecessarily vague. Just say "|websites|". Done. https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... File components/test/data/password_manager/tests.py (right): https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/tests.py:12: def WorkingTests(environment): On 2014/05/16 09:36:00, vabr (Chromium) wrote: > Since you specify "disabled" in AddWebsite(), you don't have to separate adding > the enabled and disabled tests into two functions. Please merge the two > functions. Done. https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/tests.py:15: class Amazon(Website): On 2014/05/16 09:36:00, vabr (Chromium) wrote: > Please extract the class definitions out of the function, into the global space. > Only the AddWebsite calls should stay in the function. Done. https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/tests.py:63: Espn( On 2014/05/16 09:36:00, vabr (Chromium) wrote: > nit: This line-break seems unnecessary. Done. https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/tests.py:86: On 2014/05/16 09:36:00, vabr (Chromium) wrote: > nit: Keep in mind the style guide requires 2 blank lines between class > definitions. > http://legacy.python.org/dev/peps/pep-0008/#blank-lines Done. https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/tests.py:234: "browse?qsrc=321&q=&o=0&l=dir#")) On 2014/05/16 09:36:00, vabr (Chromium) wrote: > nit: indenting is off Done. https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/tests.py:334: # Tests that can cause a crash (the cause of the crash is not related to the chrome crashes when click on sign out. So it could be any component in chrome that caused the problem. But because password manager is a small part of chrome, I think there is a small chance that the cause of the problem is password manager. This is the log by the way: I think it s realted to the password manager: rchtara@rchtara:~/chrome/src/out/Debug$ ./chrome --enable-password-manager-internals-ui --user-data-dir=/tmp/chrometest --enable-automatic-password-saving [30978:30978:0516/143138:INFO:CONSOLE(0)] "Unchecked runtime.lastError while running metricsPrivate.getVariationParams: Variation parameters are unavailable. at Object.getVariationParams (chrome-extension://pafkbggdmjlpgkdkcbjmhmfcdpncadgh/utility.js:377:11) at chrome-extension://pafkbggdmjlpgkdkcbjmhmfcdpncadgh/background.js:1155:33 at canEnableBackground (chrome-extension://pafkbggdmjlpgkdkcbjmhmfcdpncadgh/background.js:1154:10) at Object.task (chrome-extension://pafkbggdmjlpgkdkcbjmhmfcdpncadgh/background.js:1140:9) at startFirst (chrome-extension://pafkbggdmjlpgkdkcbjmhmfcdpncadgh/utility.js:733:11) at Object.add (chrome-extension://pafkbggdmjlpgkdkcbjmhmfcdpncadgh/utility.js:774:7) at onStateChange (chrome-extension://pafkbggdmjlpgkdkcbjmhmfcdpncadgh/background.js:1137:9)", source: chrome-extension://pafkbggdmjlpgkdkcbjmhmfcdpncadgh/_generated_background_page.html (0) [30978:31005:0516/143152:ERROR:raw_channel_posix.cc(149)] recvmsg: Connection reset by peer [30978:31005:0516/143152:ERROR:channel.cc(293)] RawChannel fatal error (type 1) [30978:31005:0516/143152:ERROR:raw_channel_posix.cc(149)] recvmsg: Connection reset by peer [30978:31005:0516/143152:ERROR:channel.cc(293)] RawChannel fatal error (type 1) [30978:31005:0516/143153:ERROR:raw_channel_posix.cc(149)] recvmsg: Connection reset by peer [30978:31005:0516/143153:ERROR:channel.cc(293)] RawChannel fatal error (type 1) [30978:31005:0516/143154:ERROR:raw_channel_posix.cc(149)] recvmsg: Connection reset by peer [30978:31005:0516/143154:ERROR:channel.cc(293)] RawChannel fatal error (type 1) [30978:30978:0516/143206:INFO:CONSOLE(1)] "RAPID WARNING: Specified module not in DOM: default-p_30345940-bd", source: https://s.yimg.com/zz/combo?nn/lib/metro/g/uicontrib/rapid_v3_3.17.0.js&nn/li... (1) [30978:30978:0516/143206:INFO:CONSOLE(1)] "RAPID WARNING: Specified module not in DOM: default-p_30345967-bd", source: https://s.yimg.com/zz/combo?nn/lib/metro/g/uicontrib/rapid_v3_3.17.0.js&nn/li... (1) [30978:30978:0516/143227:ERROR:render_view_context_menu.cc(271)] Update kUmaEnumToControlId. Unhanded IDC: 41120 [30978:30978:0516/143238:ERROR:render_view_context_menu.cc(271)] Update kUmaEnumToControlId. Unhanded IDC: 41120 [30978:30978:0516/143247:ERROR:render_view_context_menu.cc(271)] Update kUmaEnumToControlId. Unhanded IDC: 41120 [30978:30978:0516/143305:ERROR:render_view_context_menu.cc(271)] Update kUmaEnumToControlId. Unhanded IDC: 41120 [30978:30978:0516/143312:ERROR:render_view_context_menu.cc(271)] Update kUmaEnumToControlId. Unhanded IDC: 41120 [30978:30978:0516/143534:ERROR:render_view_context_menu.cc(271)] Update kUmaEnumToControlId. Unhanded IDC: 41120 [30978:30978:0516/143537:ERROR:render_view_context_menu.cc(271)] Update kUmaEnumToControlId. Unhanded IDC: 41120 [30978:30978:0516/143600:ERROR:render_view_context_menu.cc(271)] Update kUmaEnumToControlId. Unhanded IDC: 41120 [30978:30978:0516/143609:INFO:CONSOLE(0)] "The page at 'https://us-mg6.mail.yahoo.com/neo/launch?.rand=ef9jrsdp77ll1' was loaded over HTTPS, but displayed insecure content from 'http://geo.yahoo.com/t;_ylc=X1MDMjE0Mzc5NzU2OQRhbgNvcGVuTm90aWZpY2F0aW9uBGNoA2VtYWlsBGV0A21lbWJlcnNoaXBfbGduXzJsYwRnZANqb3Noc21pdGh0ZXN0QHlhaG9vLmNvbQRtbwMwBHBsYXRmb3JtA1lIT08tVU5QBHRwA2E0X3NvbHZlZF8ybGNfbG9naW4uaHRtbAR0cwMw?526825026716500203': this content should also be loaded over HTTPS. ", source: https://us-mg6.mail.yahoo.com/neo/launch?.rand=ef9jrsdp77ll1 (0) [30978:30978:0516/143633:INFO:CONSOLE(110)] "RAPID WARNING: undefined", source: https://s.yimg.com/zz/combo?yui:/3.12.0/yui/yui-min.js&/os/mit/td/asset-loade... (110) [30978:30978:0516/143642:INFO:CONSOLE(4348)] "Uncaught TypeError: Cannot read property 'style' of null", source: https://www.yahoo.com/ (4348) [30978:30978:0516/143645:INFO:CONSOLE(1)] "DARLA notice: 405", source: https://s.yimg.com/rq/darla/2-7-4/js/g-r-min.js (1) [30978:30978:0516/143645:INFO:CONSOLE(1)] "DARLA notice: 517", source: https://s.yimg.com/rq/darla/2-7-4/js/g-r-min.js (1) [30978:30978:0516/143652:INFO:CONSOLE(6)] "Uncaught Error: [YWA-BeaconScheduler] No data collection available", source: https://s.yimg.com/rd/combine/en-US/1393325869/common/Ajax.js,vendor/ywa-d.js... (6) [30978:30978:0516/143659:INFO:CONSOLE(4343)] "Uncaught TypeError: Cannot read property 'style' of null", source: https://www.yahoo.com/ (4343) [30978:30978:0516/143701:INFO:CONSOLE(110)] "RAPID WARNING: undefined", source: https://s.yimg.com/zz/combo?yui:/3.12.0/yui/yui-min.js&/os/mit/td/asset-loade... (110) [30978:30978:0516/143703:INFO:CONSOLE(1)] "DARLA notice: 405", source: https://s.yimg.com/rq/darla/2-7-4/js/g-r-min.js (1) [30978:30978:0516/143703:INFO:CONSOLE(1)] "DARLA notice: 517", source: https://s.yimg.com/rq/darla/2-7-4/js/g-r-min.js (1) [30978:30978:0516/143715:INFO:CONSOLE(13)] "yui: NOT loaded: stencil-", source: https://s.yimg.com/zz/combo?yui:/3.12.0/yui/yui-min.js&/os/mit/td/asset-loade... (13) [30978:31005:0516/143715:ERROR:raw_channel_posix.cc(149)] recvmsg: Connection reset by peer [30978:31005:0516/143715:ERROR:channel.cc(293)] RawChannel fatal error (type 1) [30978:31005:0516/143715:ERROR:raw_channel_posix.cc(149)] recvmsg: Connection reset by peer [30978:31005:0516/143715:ERROR:channel.cc(293)] RawChannel fatal error (type 1) https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/tests.py:395: if "--help" in args: On 2014/05/16 09:36:00, vabr (Chromium) wrote: > Please use argparse instead of handling the arguments yourself. > https://docs.python.org/2/library/argparse.html#module-argparse > > (sample code, e.g., here: > http://chromegw/viewvc/chrome-internal/trunk/tools/py_issue_tracker/src/crbug...) Done. https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/tests.py:395: if "--help" in args: On 2014/05/16 09:36:00, vabr (Chromium) wrote: > Also, in general, help should be printed if the arguments are not recognised, > instead of just when the argument is exactly --help. Done. https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/tests.py:396: print "Password Manager automated tests help." On 2014/05/16 09:36:00, vabr (Chromium) wrote: > nit: Consider using multi-line strings """...""" instead of repeating print on > each line. Done. https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... File components/test/data/password_manager/website.py (right): https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/website.py:13: def _IsOneSubstringOfAnother(s1, s2): On 2014/05/16 09:36:00, vabr (Chromium) wrote: > Please add a doc string, explaining what this function does. Done. https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/website.py:23: Autofilled = 1 On 2014/05/16 09:36:00, vabr (Chromium) wrote: > Please use all caps for consistency with the C++ enum naming: > AUTOFILLED > NOT_AUTOFILLED Done. https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/website.py:31: self, name, url, username=None, password=None, Yes, I use them to sign in https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/website.py:31: self, name, url, username=None, password=None, On 2014/05/16 09:36:00, vabr (Chromium) wrote: > Please remove |url|. You don't seem to use it, and it's always the same URL > which is passed as the GoTo() argument in the login steps. Done. https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/website.py:57: self.username_not_auto = username_not_auto username_not_auto affects only the username, but the password is treated exactly the same as for the normal website https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/website.py:60: # Waiting duration before stopping the test. On 2014/05/16 09:36:00, vabr (Chromium) wrote: > Note: max_duration limits the total time spent in waiting, but does not include > the time spent running. You might want to make this clear in the comment. > > Also, since max_duration gets decremented, a more appropriate name would be: > remaining_time_to_wait or something similar. Done. https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/website.py:80: """Clicks on an element, if it's available. On 2014/05/16 09:36:00, vabr (Chromium) wrote: > So, is this about visibility, or availability? > If the element is there, but has 'display: none' in its CSS, will it be clicked? Done. https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/website.py:90: return False On 2014/05/16 09:36:00, vabr (Chromium) wrote: > The method returns False if the element is not clicked, but does not return > anything when it is clicked. > > Please unify this. It looks like you don't use the return value, so don't return > False. Use just "return" or "pass". Done. https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/website.py:95: """Navigates the main frame to a url. On 2014/05/16 09:36:00, vabr (Chromium) wrote: > nit: "a url" -> "|url|" > (It does not navigate to just some url, it navigates to the url specified in > |url|.) Done. https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/website.py:142: """Wait for a duration. On 2014/05/16 09:36:00, vabr (Chromium) wrote: > Please specify time units. Done. https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/website.py:161: time.sleep(1) On 2014/05/16 09:36:00, vabr (Chromium) wrote: > Please call Wait() instead of manually decrementing and checking max_duration > below. Done. https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/website.py:196: # tried too, but because the password is sometimes hidden, this didn't In www.163.com, we the driver fills the username with send_keys, the list of all the similar users is shown. And the password input disappears behind the list even if the full email address is completely filled. So in this case if we click on the password, we are going to get an error because the password is invisible. The username field is almost always visible, so i m going to make sure to click on it in case we didn't fill it and check the result https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/website.py:197: # worked out. On 2014/05/16 09:36:00, vabr (Chromium) wrote: > nit: worked -> work Done. https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/website.py:199: ps = password_element.get_attribute("value")[:-1] On 2014/05/16 09:36:00, vabr (Chromium) wrote: > Pleas do not use cryptic abbreviations as names of variables. > This looks like the autofilled password, so |autofilled_password| might be an > appropriate name. Done. https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/website.py:206: password_element.get_attribute("value"), On 2014/05/16 09:36:00, vabr (Chromium) wrote: > nit: indenting is off Done. https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/website.py:215: ps = password_element.get_attribute("value")[1:] On 2014/05/16 09:36:00, vabr (Chromium) wrote: > Again, please rename |ps| appropriately. Done. https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/website.py:217: password_element.send_keys(ps) I' m going to replace all this by a click on the user input, when the driver didn't fill the username. (if we already filled the username, we don't need an interaction) And see if it work https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/website.py:232: def FillUsernameInto(self, selector): I think it's more flexible to keep it as it's now. It's going to work to work with the current websites, but with other ones that could be added, we could have same issues. https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/website.py:254: username_element.clear() I don't think it's important it important to do so. The autofill manager is going to fill the user, but not the password. https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/website.py:258: """Fills the input with the website username, if the input exists. I removed the method, not needed https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/website.py:303: def RemoveAllPasswords(self, urls): Function removed https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/website.py:309: if (self.url != ""): On 2014/05/16 09:36:00, vabr (Chromium) wrote: > nit: Just use > if self.url: Done. https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/website.py:313: self.driver.execute_script( On 2014/05/16 09:36:00, vabr (Chromium) wrote: > Please comment on what the script does. > In particular mention that it deletes the password entry from the list, and that > therefore we don't increase |i| in this case. Done. https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/website.py:315: ".row-delete-button')[%d].click()" % i) we get a fresh copy of |urls|, after each deletion, this is why we don't have that problem. https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/website.py:316: time.sleep(1) # Wait until command is executed. On 2014/05/16 09:36:00, vabr (Chromium) wrote: > Please use Wait(). Done. https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/website.py:334: self.password = correct_password On 2014/05/16 09:36:00, vabr (Chromium) wrote: > Why do you not Wait() here, but you do wait at the same place in > SuccessfullLoginTest below? Done. https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/website.py:353: time.sleep(2) On 2014/05/16 09:36:00, vabr (Chromium) wrote: > Please use Wait(). Done. https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/website.py:373: time.sleep(2) On 2014/05/16 09:36:00, vabr (Chromium) wrote: > Please use Wait(). Done. https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/website.py:382: def SuccessfulLoginAfterDeletionTest(self): On 2014/05/16 09:36:00, vabr (Chromium) wrote: > This does exactly the same thing as SuccessfulLoginTest (except it does not > Wait(), which looks like a mistake). Please remove this method and use > SuccessfulLoginTest instead. Done.
Hi Riadh, Thanks for your continuing work. I liked the changes you made so far, and although I still have a number of comments, these are rather local than big structure changes. One general note: When you write code comments, please try to check if: 1) the information is helpful for somebody who does not know the code already 2) the information is not too general to be useful (e.g., "raises an exception if there are problems" is too general) 3) the information is not unnecessarily repeated Some of your code comments pass the above test very well, some not so well. It's not a big deal, and for the current CL, I commented at particular places needing improvement. But this is something to keep in mind during coding in the future. Thanks a lot, Vaclav https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... File components/test/data/password_manager/environment.py (right): https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/environment.py:77: if self.passwords_tree is not None: On 2014/05/20 08:24:47, rchtara wrote: > I got this warning when I tied to replace this None. > /usr/local/google/home/rchtara/chrome/src/components/test/data/password_manager/environment.py:63: > FutureWarning: The behavior of this method will change in future versions. Use > specific 'len(elem)' or 'elem is not None' test instead. > if self.passwords_tree: > For all the others, I got nothing. Fair enough. Your test for "is not None" is actually correct here, as being equal to None means exactly that the tree was not constructed based on the external XML file, whereas "if self.passwords_tree" tests if the tree is there and non-empty (and this will break in the future, given the warning). It's fine to leave the current version, sorry for the noise. https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/environment.py:124: self.driver.get("https://google.com") On 2014/05/20 08:24:47, rchtara wrote: > I updated the test to go to the url of the first website. > I think i was wrong about the new tab. > If I use this script > var a = document.createElement('a'); > a.target = '_blank'; > a.href = "chrome://newtab"; > a.innerHTML = '.'; > document.body.appendChild(a); > a.click() > The pop blocker blocks the tab. > If I remove the last line. > And the ask the driver to perform the click, every thing work fine Great, thanks for fixing this! https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/environment.py:140: time.sleep(1) On 2014/05/20 08:24:47, rchtara wrote: > I don't think it's make sense to do that.max_duration is related to only one > site. And this sleep is made once for all tests. > The duration of the tests is not something very important. It's not even shown. > Some of the tests have an infinite loop in them, for example the following loop: > *click on a menu button > *wait for 1 second > *if the submenu button is shown-> break > *otherwise continue the loop. > max_duration is only created to prevent infinite loop in this cases. OK, makes sense, I misunderstood the function of max_duration. I suggested a comment change at the definition remaining_time_to_wait below, to make this easier to understand. https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/environment.py:191: """Runs the tests on many Websites. On 2014/05/20 08:24:47, rchtara wrote: > On 2014/05/16 09:36:00, vabr (Chromium) wrote: > > nit: "many Websites" is unnecessarily vague. Just say "|websites|". > > Done. That's not what I meant: you still kept "many". I commented in the new code. https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... File components/test/data/password_manager/tests.py (right): https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/tests.py:334: # Tests that can cause a crash (the cause of the crash is not related to the On 2014/05/20 08:24:47, rchtara wrote: > chrome crashes when click on sign out. > So it could be any component in chrome that caused the problem. > But because password manager is a small part of chrome, I think there is a small > chance that the cause of the problem is password manager. > This is the log by the way: I think it s realted to the password manager: So do you or do you not think it's related to the password manager? :) Anyway, this is not statistics. Unless there is an actual proof that the crash is unrelated to password manager, don't claim it. You can still separate tests which cause a crash from those just disabled (although I fail to see the benefits), just please don't make claims you cannot prove. > rchtara@rchtara:~/chrome/src/out/Debug$ ./chrome > --enable-password-manager-internals-ui --user-data-dir=/tmp/chrometest > --enable-automatic-password-saving > [30978:30978:0516/143138:INFO:CONSOLE(0)] "Unchecked runtime.lastError while > running metricsPrivate.getVariationParams: Variation parameters are unavailable. > at Object.getVariationParams > (chrome-extension://pafkbggdmjlpgkdkcbjmhmfcdpncadgh/utility.js:377:11) > at chrome-extension://pafkbggdmjlpgkdkcbjmhmfcdpncadgh/background.js:1155:33 > at canEnableBackground > (chrome-extension://pafkbggdmjlpgkdkcbjmhmfcdpncadgh/background.js:1154:10) > at Object.task > (chrome-extension://pafkbggdmjlpgkdkcbjmhmfcdpncadgh/background.js:1140:9) > at startFirst > (chrome-extension://pafkbggdmjlpgkdkcbjmhmfcdpncadgh/utility.js:733:11) > at Object.add > (chrome-extension://pafkbggdmjlpgkdkcbjmhmfcdpncadgh/utility.js:774:7) > at onStateChange > (chrome-extension://pafkbggdmjlpgkdkcbjmhmfcdpncadgh/background.js:1137:9)", > source: > chrome-extension://pafkbggdmjlpgkdkcbjmhmfcdpncadgh/_generated_background_page.html > (0) > [30978:31005:0516/143152:ERROR:raw_channel_posix.cc(149)] recvmsg: Connection > reset by peer > [30978:31005:0516/143152:ERROR:channel.cc(293)] RawChannel fatal error (type 1) > [30978:31005:0516/143152:ERROR:raw_channel_posix.cc(149)] recvmsg: Connection > reset by peer > [30978:31005:0516/143152:ERROR:channel.cc(293)] RawChannel fatal error (type 1) > [30978:31005:0516/143153:ERROR:raw_channel_posix.cc(149)] recvmsg: Connection > reset by peer > [30978:31005:0516/143153:ERROR:channel.cc(293)] RawChannel fatal error (type 1) > [30978:31005:0516/143154:ERROR:raw_channel_posix.cc(149)] recvmsg: Connection > reset by peer > [30978:31005:0516/143154:ERROR:channel.cc(293)] RawChannel fatal error (type 1) > [30978:30978:0516/143206:INFO:CONSOLE(1)] "RAPID WARNING: Specified module not > in DOM: default-p_30345940-bd", source: > https://s.yimg.com/zz/combo?nn/lib/metro/g/uicontrib/rapid_v3_3.17.0.js&nn/li... > (1) > [30978:30978:0516/143206:INFO:CONSOLE(1)] "RAPID WARNING: Specified module not > in DOM: default-p_30345967-bd", source: > https://s.yimg.com/zz/combo?nn/lib/metro/g/uicontrib/rapid_v3_3.17.0.js&nn/li... > (1) > [30978:30978:0516/143227:ERROR:render_view_context_menu.cc(271)] Update > kUmaEnumToControlId. Unhanded IDC: 41120 > [30978:30978:0516/143238:ERROR:render_view_context_menu.cc(271)] Update > kUmaEnumToControlId. Unhanded IDC: 41120 > [30978:30978:0516/143247:ERROR:render_view_context_menu.cc(271)] Update > kUmaEnumToControlId. Unhanded IDC: 41120 > [30978:30978:0516/143305:ERROR:render_view_context_menu.cc(271)] Update > kUmaEnumToControlId. Unhanded IDC: 41120 > [30978:30978:0516/143312:ERROR:render_view_context_menu.cc(271)] Update > kUmaEnumToControlId. Unhanded IDC: 41120 > [30978:30978:0516/143534:ERROR:render_view_context_menu.cc(271)] Update > kUmaEnumToControlId. Unhanded IDC: 41120 > [30978:30978:0516/143537:ERROR:render_view_context_menu.cc(271)] Update > kUmaEnumToControlId. Unhanded IDC: 41120 > [30978:30978:0516/143600:ERROR:render_view_context_menu.cc(271)] Update > kUmaEnumToControlId. Unhanded IDC: 41120 > [30978:30978:0516/143609:INFO:CONSOLE(0)] "The page at > 'https://us-mg6.mail.yahoo.com/neo/launch?.rand=ef9jrsdp77ll1' was loaded over > HTTPS, but displayed insecure content from > 'http://geo.yahoo.com/t;_ylc=X1MDMjE0Mzc5NzU2OQRhbgNvcGVuTm90aWZpY2F0aW9uBGNoA2VtYWlsBGV0A21lbWJlcnNoaXBfbGduXzJsYwRnZANqb3Noc21pdGh0ZXN0QHlhaG9vLmNvbQRtbwMwBHBsYXRmb3JtA1lIT08tVU5QBHRwA2E0X3NvbHZlZF8ybGNfbG9naW4uaHRtbAR0cwMw?526825026716500203': > this content should also be loaded over HTTPS. > ", source: https://us-mg6.mail.yahoo.com/neo/launch?.rand=ef9jrsdp77ll1 (0) > [30978:30978:0516/143633:INFO:CONSOLE(110)] "RAPID WARNING: undefined", source: > https://s.yimg.com/zz/combo?yui:/3.12.0/yui/yui-min.js&/os/mit/td/asset-loade... > (110) > [30978:30978:0516/143642:INFO:CONSOLE(4348)] "Uncaught TypeError: Cannot read > property 'style' of null", source: https://www.yahoo.com/ (4348) > [30978:30978:0516/143645:INFO:CONSOLE(1)] "DARLA notice: 405", source: > https://s.yimg.com/rq/darla/2-7-4/js/g-r-min.js (1) > [30978:30978:0516/143645:INFO:CONSOLE(1)] "DARLA notice: 517", source: > https://s.yimg.com/rq/darla/2-7-4/js/g-r-min.js (1) > [30978:30978:0516/143652:INFO:CONSOLE(6)] "Uncaught Error: [YWA-BeaconScheduler] > No data collection available", source: > https://s.yimg.com/rd/combine/en-US/1393325869/common/Ajax.js,vendor/ywa-d.js... > (6) > [30978:30978:0516/143659:INFO:CONSOLE(4343)] "Uncaught TypeError: Cannot read > property 'style' of null", source: https://www.yahoo.com/ (4343) > [30978:30978:0516/143701:INFO:CONSOLE(110)] "RAPID WARNING: undefined", source: > https://s.yimg.com/zz/combo?yui:/3.12.0/yui/yui-min.js&/os/mit/td/asset-loade... > (110) > [30978:30978:0516/143703:INFO:CONSOLE(1)] "DARLA notice: 405", source: > https://s.yimg.com/rq/darla/2-7-4/js/g-r-min.js (1) > [30978:30978:0516/143703:INFO:CONSOLE(1)] "DARLA notice: 517", source: > https://s.yimg.com/rq/darla/2-7-4/js/g-r-min.js (1) > [30978:30978:0516/143715:INFO:CONSOLE(13)] "yui: NOT loaded: stencil-", source: > https://s.yimg.com/zz/combo?yui:/3.12.0/yui/yui-min.js&/os/mit/td/asset-loade... > (13) > [30978:31005:0516/143715:ERROR:raw_channel_posix.cc(149)] recvmsg: Connection > reset by peer > [30978:31005:0516/143715:ERROR:channel.cc(293)] RawChannel fatal error (type 1) > [30978:31005:0516/143715:ERROR:raw_channel_posix.cc(149)] recvmsg: Connection > reset by peer > [30978:31005:0516/143715:ERROR:channel.cc(293)] RawChannel fatal error (type 1) https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... File components/test/data/password_manager/website.py (right): https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/website.py:31: self, name, url, username=None, password=None, On 2014/05/20 08:24:47, rchtara wrote: > Yes, I use them to sign in But you don't seem to use the __init__ arguments username and password, as opposed to self.username, and self.password, which get assigned to in the environment.py code. I'll add a comment at the most recent patch set. https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/website.py:57: self.username_not_auto = username_not_auto On 2014/05/20 08:24:47, rchtara wrote: > username_not_auto affects only the username, but the password is treated exactly > > the same as for the normal website You are correct, thanks for explanation. https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/website.py:232: def FillUsernameInto(self, selector): On 2014/05/20 08:24:47, rchtara wrote: > I think it's more flexible to keep it as it's now. > It's going to work to work with the current websites, but with other ones that > could be added, we could have same issues. Fair enough, let's keep them separate. https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/website.py:254: username_element.clear() On 2014/05/20 08:24:47, rchtara wrote: > I don't think it's important it important to do so. > The autofill manager is going to fill the user, but not the password. Ah right, that's a good point, I forgot about the non-password autofill. :) Thanks for your explanation. https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/website.py:315: ".row-delete-button')[%d].click()" % i) On 2014/05/20 08:24:47, rchtara wrote: > we get a fresh copy of |urls|, after each deletion, this is why we don't have > that problem. > Ah, that makes sense, thanks for your explanation. Anyway, I like that you now delete all passwords in one pass. :) https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... File components/test/data/password_manager/README (right): https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/README:55: To display the log on the screen, use: nit: "the log" -> "debugging messages" Also below. https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/README:61: https://goto.google.com/pbkoq This URL is internal, please remove it from the sources. Instead, you can use http://goo.gl, or just the full URL. Maybe even better, leave this sentence out completely, it's not hard to google Python documentation. https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/README:68: 2) Add these lines : That's not completely true -- not exactly the lines below should be added. I suggest replacing this line with: 2) Add tests like this: Additionally, I would prefix the body of Login with a comment: # Add login steps for the website, for example: Similarly for Logout(). https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/README:81: environment.AddWebsiteTest(NewWebsiteTest("http://url")) Here you write "http://url", but below in tests.py you just write words like "google". Please update the documentation. https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/README:83: Then, to create the new test, you need just to add: Are these 3 lines obsolete? https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/README:85: NewWebsiteTest("website name", "username", "password")) Please note what you mean by "website name". In particular, what is it used for (is it something the testing person can choose themselves, or does it have to be derived from the website?). https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/README:97: 3) Use the flowing methods to perform the login and logout: This is not "step 3", as in: it should not follow step 2. This knowledge is already needed in step 2. So I suggest dropping "3) ". https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... File components/test/data/password_manager/environment.py (right): https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/environment.py:19: numeric_level, log_screen, log_file): nit: log_screen -> log_to_console https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/environment.py:19: numeric_level, log_screen, log_file): Please provide default values for numeric_level, log_screen and log_file arguments (None, False, and "", respectively). Logging is not the core function of Environment, so whoever uses this API should not be forced to deduce from the code which are the default values. https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/environment.py:23: chrome_path: The chrome binary file.y typo: "y" at the end of the line https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/environment.py:25: profile_path: The testing profile folder. nit: "profile" -> "Chrome profile", just for clarity https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/environment.py:29: numeric_level: The log numeric level. nit: "log" -> "log verbosity" https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/environment.py:30: log_screen: If set, the tests log is going to be shown on the screen. nit: change "screen" to console also in the comment https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/environment.py:30: log_screen: If set, the tests log is going to be shown on the screen. nit: "tests log is going to be" -> "debug logs will be" https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/environment.py:30: log_screen: If set, the tests log is going to be shown on the screen. nit: "set" -> "true" https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/environment.py:31: log_file: The file where to store the log. nit: Comment on what happens if it is empty. https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/environment.py:59: logging.error("""Error: %s.\n The tests execution is continuing. But I don't think you use """ correctly here. If you only need to wrap a string literal over multiple lines, use single quotes and \ at the end of the open lines: "This spans \ two lines" Note that the line break will not be part of the resulting string, i.e., the above string is equal to "This spans two lines". If you use """, you don't have to write \, but the line break will be part of the string: """A B""" is equal to "A\nB". In particular, on this line, I suggest it would be better not to specify line breaks (you don't know how wide the log console will be anyway). Therefore it seems better to use single quotes, and also omit '\n'. https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/environment.py:60: the tests are going to be less stable.""" % e) Please be more specific: write what is the error, and what are the implications. For example: "Error: could not wipe profile directory (%s). This affects the stability of the tests. Continuing to run tests." https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/environment.py:64: options.add_argument("enable-password-manager-internals-ui") You probably missed my comment earlier: enable-password-manager-internals-ui is no more needed. https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/environment.py:88: self.save_count = 0 Note: I see you use this to deal with the internals page containing all the logs so far. That's fine, but soon it will be able to flush all the logs by refreshing the page. If that would make your code easier, please let me know, and I'll prioritise fixing that. https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/environment.py:89: # Show whether or not it's the first time to execute GoTo. "Show"? It's not clear how and to whom this is shown. Did you mean "store"? Also, could you briefly explain in the comment, what this information is used for? https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/environment.py:96: websitetest: The WebsiteTest instance that's going to be added to the nit: You can simply write: "The WebsiteTest instance to be added." Concise comments are quicker and easier to read. https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/environment.py:98: disabled: Test is disabled. nit: Whether test is disabled. https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/environment.py:137: first tab. Raises an exception otherwise. It's not clear what "otherwise" refers to. I believe leaving the last sentence completely is fine, as you describe the exception in the "Raises:" section, and repeating only clutters the comments. https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/environment.py:145: if not self.website_window: nit: Consider starting with if self.website_window: raise (...) Then you can add the rest of the code without being nested in an if-branch. https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/environment.py:184: error_message: Error message for the exception . nit: Remove the two spaces at the end of the sentence. https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/environment.py:192: Exception: An exception is raised in case there is a problem. nit: "there is a problem" -> "the result does not match the expectation" (The latter provides much better information, and is not much longer. Saying just that "exception is raised if there's a problem" does not help much, as it is true almost always. :)) https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/environment.py:201: self.CheckPrompt(log_message, False, error_message, timeout - 1) You hard-code False as the "prompt_should_show_up" argument here. I can see why you do it (so that save_count does not get increased again), but it's rather confusing. Also, the code seems wrong for the case when prompt_should_show_up == False since beginning: In that case count == self.save_count, and so CheckPrompt immediately returns with success, instead of waiting 10 seconds, to see whether the prompt is not shown after all. My suggestion: Define a private helper method, which would wait until either time is out, or the prompt appears, and will return false in the first case, and true in the latter. Then CheckPrompt simply calls that helper method to see whether the prompt was shown, compares that to the expectation, and raises an exception unless they match. https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/environment.py:207: """Runs the tests on all the WebsiteTests. typo: two consequent spaces between "the" and "WebsiteTests". https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/environment.py:210: prompt_test: Prompt tests or normal tests. Please explain what are "prompt tests". The most plausible English meaning would be "fast tests", which is not what you mean here. I'm guessing you mean tests caring about the save-password prompt, and tests which don't; but that's not obvious (and still not clear what exactly means |prompt_test|, and what are the values). Same below. https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/environment.py:232: """Runs the tests on many WebsiteTests. many WebsiteTests -> websites named in |tests| Also below at TestList() and PromptTestList(). https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... File components/test/data/password_manager/tests.py (right): https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/tests.py:198: # crbug.com/368690 nit: If you prefix the URL with "http://", the link will become clickable in CodeSearch. https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/tests.py:393: def RunTests(chrome_path, chromedriver_path, profile_path, Please add a documentation string for this function. https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/tests.py:410: elif len(tests) == 0: "len(tests) == 0" -> "not tests" (search for "len(" in http://legacy.python.org/dev/peps/pep-0008/#programming-recommendations) bonus points: you can swap the "elif" and "else" branches to get rid of "not" :) https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/tests.py:431: help="""Set the profile path (required). You just need to choose a I suggest using single quotes (plus \ at the end of the line) instead of """, because you don't want to dictate line breaks for output. https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/tests.py:479: # the passwords is shown in the the way we expected. nit: Did you mean "stored" instead of "shown"? https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... File components/test/data/password_manager/websitetest.py (right): https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/websitetest.py:40: self, name, username=None, password=None, Please remove username and password, you don't use them. You do use self.username and self.password, but you never assign to them through __init__, so the arguments in __init__ are not used. https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/websitetest.py:66: # The |remaining_time_to_wait| limits the total time spent in waiting, but nit: "spent in waiting, but ... running." -> "spent in potentially infinite loops." This would make clear that the purpose is not to set a hard limit on the test duration, but rather avoid infinite loops. In that case it's not important to comment on spent time which does not contribute to remaining_time_to_wait decrements. https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/websitetest.py:87: """Clicks on an element, if it's visible. I'm not sure I understand what's meant by "visible" -- if one element covers another, if it has 100% transparency, is out of the viewing area, etc., would it not be clicked? The "NoSuchElementException" suggests only elements which are in fact not present in the DOM at all will not be clicked. Please explain. https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/websitetest.py:149: True if the element is visible. Please be consistent with any changes you make about the term "visible" in ClickIfVisible. Also here it's not clear what's the relation between "displayed" and "visible". Please use one term if they mean the same thing. https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/websitetest.py:162: """Wait for a duration in seconds. Please comment on the intended use-case: this needs to be used in potentially infinite loops, to limit their running time. https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/websitetest.py:220: password_element.send_keys(autofilled_password) Unless we raise an exception in the next step, |autofilled_password| is empty. Why do we send the empty string to the |password_element|? https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/websitetest.py:248: # Chrome protects the password inputs and doesn't fill them until This assumes that FillUsernameInto is always called before FillPasswordInto. Because you keep these methods separate, that's not guaranteed. My suggestion here is: do this click() in FillPasswordInto instead. You can either pass the username CSS selector to FillPasswordInto, or you can click just about anywhere on the page. P.S. I really liked the comment you wrote: very clear and helpful to understand the reason for click(). https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/websitetest.py:293: Exception: An exception is raised if the test fail. nit: fails https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/websitetest.py:293: Exception: An exception is raised if the test fail. Actually, "test fails" is confusing in this case: failing means succeeding, since the test tries a wrong password. Please re-formulate here, and below, more concretely when the exception is raised. https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/websitetest.py:354: """Does the wrong login test: Tries to login with a wrong password and Did you forget to change the doc string? https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/websitetest.py:360: logging.info("\nWrong Login Test for %s \n" % self.name) Did you forget to change the log message?
Hi, I updated the code, again, could you please check it? Thanks a lot Riadh https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... File components/test/data/password_manager/environment.py (right): https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/environment.py:34: options.add_argument("enable-password-manager-internals-ui") Yes, i have an old version of code, so i need it. :) I will removed before landing the cl. https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/environment.py:77: if self.passwords_tree is not None: No problem https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/environment.py:124: self.driver.get("https://google.com") Thanks :) https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/environment.py:140: time.sleep(1) On 2014/05/20 14:47:25, vabr (Chromium) wrote: > On 2014/05/20 08:24:47, rchtara wrote: > > I don't think it's make sense to do that.max_duration is related to only one > > site. And this sleep is made once for all tests. > > The duration of the tests is not something very important. It's not even > shown. > > Some of the tests have an infinite loop in them, for example the following > loop: > > *click on a menu button > > *wait for 1 second > > *if the submenu button is shown-> break > > *otherwise continue the loop. > > max_duration is only created to prevent infinite loop in this cases. > > OK, makes sense, I misunderstood the function of max_duration. I suggested a > comment change at the definition remaining_time_to_wait below, to make this > easier to understand. Done. https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/environment.py:191: """Runs the tests on many Websites. sorry :) https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... File components/test/data/password_manager/tests.py (right): https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/tests.py:334: # Tests that can cause a crash (the cause of the crash is not related to the On 2014/05/20 14:47:25, vabr (Chromium) wrote: > On 2014/05/20 08:24:47, rchtara wrote: > > chrome crashes when click on sign out. > > So it could be any component in chrome that caused the problem. > > But because password manager is a small part of chrome, I think there is a > small > > chance that the cause of the problem is password manager. > > This is the log by the way: I think it s realted to the password manager: > > So do you or do you not think it's related to the password manager? :) > > Anyway, this is not statistics. Unless there is an actual proof that the crash > is unrelated to password manager, don't claim it. You can still separate tests > which cause a crash from those just disabled (although I fail to see the > benefits), just please don't make claims you cannot prove. > > > rchtara@rchtara:~/chrome/src/out/Debug$ ./chrome > > --enable-password-manager-internals-ui --user-data-dir=/tmp/chrometest > > --enable-automatic-password-saving > > [30978:30978:0516/143138:INFO:CONSOLE(0)] "Unchecked runtime.lastError while > > running metricsPrivate.getVariationParams: Variation parameters are > unavailable. > > at Object.getVariationParams > > (chrome-extension://pafkbggdmjlpgkdkcbjmhmfcdpncadgh/utility.js:377:11) > > at > chrome-extension://pafkbggdmjlpgkdkcbjmhmfcdpncadgh/background.js:1155:33 > > at canEnableBackground > > (chrome-extension://pafkbggdmjlpgkdkcbjmhmfcdpncadgh/background.js:1154:10) > > at Object.task > > (chrome-extension://pafkbggdmjlpgkdkcbjmhmfcdpncadgh/background.js:1140:9) > > at startFirst > > (chrome-extension://pafkbggdmjlpgkdkcbjmhmfcdpncadgh/utility.js:733:11) > > at Object.add > > (chrome-extension://pafkbggdmjlpgkdkcbjmhmfcdpncadgh/utility.js:774:7) > > at onStateChange > > (chrome-extension://pafkbggdmjlpgkdkcbjmhmfcdpncadgh/background.js:1137:9)", > > source: > > > chrome-extension://pafkbggdmjlpgkdkcbjmhmfcdpncadgh/_generated_background_page.html > > (0) > > [30978:31005:0516/143152:ERROR:raw_channel_posix.cc(149)] recvmsg: Connection > > reset by peer > > [30978:31005:0516/143152:ERROR:channel.cc(293)] RawChannel fatal error (type > 1) > > [30978:31005:0516/143152:ERROR:raw_channel_posix.cc(149)] recvmsg: Connection > > reset by peer > > [30978:31005:0516/143152:ERROR:channel.cc(293)] RawChannel fatal error (type > 1) > > [30978:31005:0516/143153:ERROR:raw_channel_posix.cc(149)] recvmsg: Connection > > reset by peer > > [30978:31005:0516/143153:ERROR:channel.cc(293)] RawChannel fatal error (type > 1) > > [30978:31005:0516/143154:ERROR:raw_channel_posix.cc(149)] recvmsg: Connection > > reset by peer > > [30978:31005:0516/143154:ERROR:channel.cc(293)] RawChannel fatal error (type > 1) > > [30978:30978:0516/143206:INFO:CONSOLE(1)] "RAPID WARNING: Specified module not > > in DOM: default-p_30345940-bd", source: > > > https://s.yimg.com/zz/combo?nn/lib/metro/g/uicontrib/rapid_v3_3.17.0.js&nn/li... > > (1) > > [30978:30978:0516/143206:INFO:CONSOLE(1)] "RAPID WARNING: Specified module not > > in DOM: default-p_30345967-bd", source: > > > https://s.yimg.com/zz/combo?nn/lib/metro/g/uicontrib/rapid_v3_3.17.0.js&nn/li... > > (1) > > [30978:30978:0516/143227:ERROR:render_view_context_menu.cc(271)] Update > > kUmaEnumToControlId. Unhanded IDC: 41120 > > [30978:30978:0516/143238:ERROR:render_view_context_menu.cc(271)] Update > > kUmaEnumToControlId. Unhanded IDC: 41120 > > [30978:30978:0516/143247:ERROR:render_view_context_menu.cc(271)] Update > > kUmaEnumToControlId. Unhanded IDC: 41120 > > [30978:30978:0516/143305:ERROR:render_view_context_menu.cc(271)] Update > > kUmaEnumToControlId. Unhanded IDC: 41120 > > [30978:30978:0516/143312:ERROR:render_view_context_menu.cc(271)] Update > > kUmaEnumToControlId. Unhanded IDC: 41120 > > [30978:30978:0516/143534:ERROR:render_view_context_menu.cc(271)] Update > > kUmaEnumToControlId. Unhanded IDC: 41120 > > [30978:30978:0516/143537:ERROR:render_view_context_menu.cc(271)] Update > > kUmaEnumToControlId. Unhanded IDC: 41120 > > [30978:30978:0516/143600:ERROR:render_view_context_menu.cc(271)] Update > > kUmaEnumToControlId. Unhanded IDC: 41120 > > [30978:30978:0516/143609:INFO:CONSOLE(0)] "The page at > > 'https://us-mg6.mail.yahoo.com/neo/launch?.rand=ef9jrsdp77ll1' was loaded over > > HTTPS, but displayed insecure content from > > > 'http://geo.yahoo.com/t;_ylc=X1MDMjE0Mzc5NzU2OQRhbgNvcGVuTm90aWZpY2F0aW9uBGNoA2VtYWlsBGV0A21lbWJlcnNoaXBfbGduXzJsYwRnZANqb3Noc21pdGh0ZXN0QHlhaG9vLmNvbQRtbwMwBHBsYXRmb3JtA1lIT08tVU5QBHRwA2E0X3NvbHZlZF8ybGNfbG9naW4uaHRtbAR0cwMw?526825026716500203': > > this content should also be loaded over HTTPS. > > ", source: https://us-mg6.mail.yahoo.com/neo/launch?.rand=ef9jrsdp77ll1 (0) > > [30978:30978:0516/143633:INFO:CONSOLE(110)] "RAPID WARNING: undefined", > source: > > > https://s.yimg.com/zz/combo?yui:/3.12.0/yui/yui-min.js&/os/mit/td/asset-loade... > > (110) > > [30978:30978:0516/143642:INFO:CONSOLE(4348)] "Uncaught TypeError: Cannot read > > property 'style' of null", source: https://www.yahoo.com/ (4348) > > [30978:30978:0516/143645:INFO:CONSOLE(1)] "DARLA notice: 405", source: > > https://s.yimg.com/rq/darla/2-7-4/js/g-r-min.js (1) > > [30978:30978:0516/143645:INFO:CONSOLE(1)] "DARLA notice: 517", source: > > https://s.yimg.com/rq/darla/2-7-4/js/g-r-min.js (1) > > [30978:30978:0516/143652:INFO:CONSOLE(6)] "Uncaught Error: > [YWA-BeaconScheduler] > > No data collection available", source: > > > https://s.yimg.com/rd/combine/en-US/1393325869/common/Ajax.js,vendor/ywa-d.js... > > (6) > > [30978:30978:0516/143659:INFO:CONSOLE(4343)] "Uncaught TypeError: Cannot read > > property 'style' of null", source: https://www.yahoo.com/ (4343) > > [30978:30978:0516/143701:INFO:CONSOLE(110)] "RAPID WARNING: undefined", > source: > > > https://s.yimg.com/zz/combo?yui:/3.12.0/yui/yui-min.js&/os/mit/td/asset-loade... > > (110) > > [30978:30978:0516/143703:INFO:CONSOLE(1)] "DARLA notice: 405", source: > > https://s.yimg.com/rq/darla/2-7-4/js/g-r-min.js (1) > > [30978:30978:0516/143703:INFO:CONSOLE(1)] "DARLA notice: 517", source: > > https://s.yimg.com/rq/darla/2-7-4/js/g-r-min.js (1) > > [30978:30978:0516/143715:INFO:CONSOLE(13)] "yui: NOT loaded: stencil-", > source: > > > https://s.yimg.com/zz/combo?yui:/3.12.0/yui/yui-min.js&/os/mit/td/asset-loade... > > (13) > > [30978:31005:0516/143715:ERROR:raw_channel_posix.cc(149)] recvmsg: Connection > > reset by peer > > [30978:31005:0516/143715:ERROR:channel.cc(293)] RawChannel fatal error (type > 1) > > [30978:31005:0516/143715:ERROR:raw_channel_posix.cc(149)] recvmsg: Connection > > reset by peer > > [30978:31005:0516/143715:ERROR:channel.cc(293)] RawChannel fatal error (type > 1) > Done. https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... File components/test/data/password_manager/website.py (right): https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/website.py:57: self.username_not_auto = username_not_auto you re welcome :) https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/website.py:232: def FillUsernameInto(self, selector): On 2014/05/20 14:47:25, vabr (Chromium) wrote: > On 2014/05/20 08:24:47, rchtara wrote: > > I think it's more flexible to keep it as it's now. > > It's going to work to work with the current websites, but with other ones that > > could be added, we could have same issues. > > Fair enough, let's keep them separate. Done. https://codereview.chromium.org/273523004/diff/210001/components/test/data/pa... components/test/data/password_manager/website.py:254: username_element.clear() You re welcome https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... File components/test/data/password_manager/README (right): https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/README:55: To display the log on the screen, use: On 2014/05/20 14:47:25, vabr (Chromium) wrote: > nit: "the log" -> "debugging messages" > Also below. Done. https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/README:61: https://goto.google.com/pbkoq On 2014/05/20 14:47:25, vabr (Chromium) wrote: > This URL is internal, please remove it from the sources. > Instead, you can use http://goo.gl, or just the full URL. > Maybe even better, leave this sentence out completely, it's not hard to google > Python documentation. Done. https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/README:68: 2) Add these lines : On 2014/05/20 14:47:25, vabr (Chromium) wrote: > That's not completely true -- not exactly the lines below should be added. I > suggest replacing this line with: > 2) Add tests like this: > > Additionally, I would prefix the body of Login with a comment: > # Add login steps for the website, for example: > > Similarly for Logout(). Done. https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/README:81: environment.AddWebsiteTest(NewWebsiteTest("http://url")) On 2014/05/20 14:47:25, vabr (Chromium) wrote: > Here you write "http://url", but below in tests.py you just write words like > "google". Please update the documentation. Done. https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/README:83: Then, to create the new test, you need just to add: On 2014/05/20 14:47:25, vabr (Chromium) wrote: > Are these 3 lines obsolete? Done. https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/README:85: NewWebsiteTest("website name", "username", "password")) On 2014/05/20 14:47:25, vabr (Chromium) wrote: > Please note what you mean by "website name". In particular, what is it used for > (is it something the testing person can choose themselves, or does it have to be > derived from the website?). Done. https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/README:97: 3) Use the flowing methods to perform the login and logout: On 2014/05/20 14:47:25, vabr (Chromium) wrote: > This is not "step 3", as in: it should not follow step 2. This knowledge is > already needed in step 2. So I suggest dropping "3) ". Done. https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... File components/test/data/password_manager/environment.py (right): https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/environment.py:19: numeric_level, log_screen, log_file): On 2014/05/20 14:47:25, vabr (Chromium) wrote: > nit: log_screen -> log_to_console Done. https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/environment.py:19: numeric_level, log_screen, log_file): On 2014/05/20 14:47:25, vabr (Chromium) wrote: > Please provide default values for numeric_level, log_screen and log_file > arguments (None, False, and "", respectively). Logging is not the core function > of Environment, so whoever uses this API should not be forced to deduce from the > code which are the default values. Done. https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/environment.py:23: chrome_path: The chrome binary file.y On 2014/05/20 14:47:25, vabr (Chromium) wrote: > typo: "y" at the end of the line Done. https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/environment.py:25: profile_path: The testing profile folder. On 2014/05/20 14:47:25, vabr (Chromium) wrote: > nit: "profile" -> "Chrome profile", just for clarity Done. https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/environment.py:29: numeric_level: The log numeric level. On 2014/05/20 14:47:25, vabr (Chromium) wrote: > nit: "log" -> "log verbosity" Done. https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/environment.py:30: log_screen: If set, the tests log is going to be shown on the screen. On 2014/05/20 14:47:25, vabr (Chromium) wrote: > nit: change "screen" to console also in the comment Done. https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/environment.py:30: log_screen: If set, the tests log is going to be shown on the screen. On 2014/05/20 14:47:25, vabr (Chromium) wrote: > nit: "tests log is going to be" -> "debug logs will be" Done. https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/environment.py:30: log_screen: If set, the tests log is going to be shown on the screen. On 2014/05/20 14:47:25, vabr (Chromium) wrote: > nit: change "screen" to console also in the comment Done. https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/environment.py:31: log_file: The file where to store the log. On 2014/05/20 14:47:25, vabr (Chromium) wrote: > nit: Comment on what happens if it is empty. Done. https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/environment.py:59: logging.error("""Error: %s.\n The tests execution is continuing. But On 2014/05/20 14:47:25, vabr (Chromium) wrote: > I don't think you use """ correctly here. If you only need to wrap a string > literal over multiple lines, use single quotes and \ at the end of the open > lines: > "This spans \ > two lines" > Note that the line break will not be part of the resulting string, i.e., the > above string is equal to > "This spans two lines". > If you use """, you don't have to write \, but the line break will be part of > the string: > """A > B""" > is equal to "A\nB". > > In particular, on this line, I suggest it would be better not to specify line > breaks (you don't know how wide the log console will be anyway). Therefore it > seems better to use single quotes, and also omit '\n'. Done. https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/environment.py:60: the tests are going to be less stable.""" % e) On 2014/05/20 14:47:25, vabr (Chromium) wrote: > Please be more specific: write what is the error, and what are the implications. > For example: > "Error: could not wipe profile directory (%s). This affects the stability of the > tests. Continuing to run tests." Done. https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/environment.py:64: options.add_argument("enable-password-manager-internals-ui") I didn't update my chromium yet, so I will keep using the enable-automatic-password-saving until and remove it at the end. https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/environment.py:88: self.save_count = 0 No, it's going make it harder, because I will need to refresh every time i do that. I will need to know the right time to refresh too. So actually it's even harder for me to do that. https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/environment.py:89: # Show whether or not it's the first time to execute GoTo. On 2014/05/20 14:47:25, vabr (Chromium) wrote: > "Show"? It's not clear how and to whom this is shown. > Did you mean "store"? Also, could you briefly explain in the comment, what this > information is used for? Done. https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/environment.py:96: websitetest: The WebsiteTest instance that's going to be added to the On 2014/05/20 14:47:25, vabr (Chromium) wrote: > nit: You can simply write: > "The WebsiteTest instance to be added." > Concise comments are quicker and easier to read. Done. https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/environment.py:98: disabled: Test is disabled. On 2014/05/20 14:47:25, vabr (Chromium) wrote: > nit: Whether test is disabled. Done. https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/environment.py:137: first tab. Raises an exception otherwise. On 2014/05/20 14:47:25, vabr (Chromium) wrote: > It's not clear what "otherwise" refers to. > I believe leaving the last sentence completely is fine, as you describe the > exception in the "Raises:" section, and repeating only clutters the comments. Done. https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/environment.py:145: if not self.website_window: On 2014/05/20 14:47:25, vabr (Chromium) wrote: > nit: Consider starting with > if self.website_window: > raise (...) > > Then you can add the rest of the code without being nested in an if-branch. Done. https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/environment.py:184: error_message: Error message for the exception . On 2014/05/20 14:47:25, vabr (Chromium) wrote: > nit: Remove the two spaces at the end of the sentence. Done. https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/environment.py:192: Exception: An exception is raised in case there is a problem. On 2014/05/20 14:47:25, vabr (Chromium) wrote: > nit: "there is a problem" -> "the result does not match the expectation" > (The latter provides much better information, and is not much longer. Saying > just that "exception is raised if there's a problem" does not help much, as it > is true almost always. :)) Done. https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/environment.py:201: self.CheckPrompt(log_message, False, error_message, timeout - 1) On 2014/05/20 14:47:25, vabr (Chromium) wrote: > You hard-code False as the "prompt_should_show_up" argument here. I can see why > you do it (so that save_count does not get increased again), but it's rather > confusing. > > Also, the code seems wrong for the case when prompt_should_show_up == False > since beginning: > In that case count == self.save_count, and so CheckPrompt immediately returns > with success, instead of waiting 10 seconds, to see whether the prompt is not > shown after all. > > My suggestion: > Define a private helper method, which would wait until either time is out, or > the prompt appears, and will return false in the first case, and true in the > latter. Then CheckPrompt simply calls that helper method to see whether the > prompt was shown, compares that to the expectation, and raises an exception > unless they match. Done. https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/environment.py:207: """Runs the tests on all the WebsiteTests. On 2014/05/20 14:47:25, vabr (Chromium) wrote: > typo: two consequent spaces between "the" and "WebsiteTests". Done. https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/environment.py:210: prompt_test: Prompt tests or normal tests. On 2014/05/20 14:47:25, vabr (Chromium) wrote: > Please explain what are "prompt tests". The most plausible English meaning would > be "fast tests", which is not what you mean here. I'm guessing you mean tests > caring about the save-password prompt, and tests which don't; but that's not > obvious (and still not clear what exactly means |prompt_test|, and what are the > values). > > Same below. Done. https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/environment.py:232: """Runs the tests on many WebsiteTests. On 2014/05/20 14:47:25, vabr (Chromium) wrote: > many WebsiteTests -> websites named in |tests| > > Also below at TestList() and PromptTestList(). Done. https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... File components/test/data/password_manager/tests.py (right): https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/tests.py:198: # crbug.com/368690 On 2014/05/20 14:47:25, vabr (Chromium) wrote: > nit: If you prefix the URL with "http://", the link will become clickable in > CodeSearch. Done. https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/tests.py:393: def RunTests(chrome_path, chromedriver_path, profile_path, On 2014/05/20 14:47:25, vabr (Chromium) wrote: > Please add a documentation string for this function. Done. https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/tests.py:410: elif len(tests) == 0: On 2014/05/20 14:47:25, vabr (Chromium) wrote: > "len(tests) == 0" -> "not tests" > (search for "len(" in > http://legacy.python.org/dev/peps/pep-0008/#programming-recommendations) > > bonus points: you can swap the "elif" and "else" branches to get rid of "not" :) Done. https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/tests.py:431: help="""Set the profile path (required). You just need to choose a On 2014/05/20 14:47:25, vabr (Chromium) wrote: > I suggest using single quotes (plus \ at the end of the line) instead of """, > because you don't want to dictate line breaks for output. Done. https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/tests.py:479: # the passwords is shown in the the way we expected. On 2014/05/20 14:47:25, vabr (Chromium) wrote: > nit: Did you mean "stored" instead of "shown"? Done. https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... File components/test/data/password_manager/websitetest.py (right): https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/websitetest.py:40: self, name, username=None, password=None, There not used, but they are useful: if someone who wants to create new tests and doesn't want to use an xml file for password https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/websitetest.py:66: # The |remaining_time_to_wait| limits the total time spent in waiting, but On 2014/05/20 14:47:25, vabr (Chromium) wrote: > nit: "spent in waiting, but ... running." -> "spent in potentially infinite > loops." > > This would make clear that the purpose is not to set a hard limit on the test > duration, but rather avoid infinite loops. In that case it's not important to > comment on spent time which does not contribute to remaining_time_to_wait > decrements. Done. https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/websitetest.py:87: """Clicks on an element, if it's visible. On 2014/05/20 14:47:25, vabr (Chromium) wrote: > I'm not sure I understand what's meant by "visible" -- if one element covers > another, if it has 100% transparency, is out of the viewing area, etc., would it > not be clicked? > The "NoSuchElementException" suggests only elements which are in fact not > present in the DOM at all will not be clicked. Please explain. Done. https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/websitetest.py:149: True if the element is visible. On 2014/05/20 14:47:25, vabr (Chromium) wrote: > Please be consistent with any changes you make about the term "visible" in > ClickIfVisible. Also here it's not clear what's the relation between "displayed" > and "visible". Please use one term if they mean the same thing. Done. https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/websitetest.py:162: """Wait for a duration in seconds. On 2014/05/20 14:47:25, vabr (Chromium) wrote: > Please comment on the intended use-case: this needs to be used in potentially > infinite loops, to limit their running time. Done. https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/websitetest.py:220: password_element.send_keys(autofilled_password) On 2014/05/20 14:47:25, vabr (Chromium) wrote: > Unless we raise an exception in the next step, |autofilled_password| is empty. > Why do we send the empty string to the |password_element|? Done. https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/websitetest.py:248: # Chrome protects the password inputs and doesn't fill them until Thanks :) https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/websitetest.py:293: Exception: An exception is raised if the test fail. On 2014/05/20 14:47:25, vabr (Chromium) wrote: > nit: fails Done. https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/websitetest.py:293: Exception: An exception is raised if the test fail. On 2014/05/20 14:47:25, vabr (Chromium) wrote: > Actually, "test fails" is confusing in this case: failing means succeeding, > since the test tries a wrong password. > Please re-formulate here, and below, more concretely when the exception is > raised. Done. https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/websitetest.py:354: """Does the wrong login test: Tries to login with a wrong password and On 2014/05/20 14:47:25, vabr (Chromium) wrote: > Did you forget to change the doc string? Done. https://codereview.chromium.org/273523004/diff/250001/components/test/data/pa... components/test/data/password_manager/websitetest.py:360: logging.info("\nWrong Login Test for %s \n" % self.name) On 2014/05/20 14:47:25, vabr (Chromium) wrote: > Did you forget to change the log message? Done.
Hi Riadh, Thanks! I'm sending the next batch of comments. It's significantly smaller than the previous ones, we are getting near to the finish! :) Cheers, Vaclav https://codereview.chromium.org/273523004/diff/290001/components/test/data/pa... File components/test/data/password_manager/README (right): https://codereview.chromium.org/273523004/diff/290001/components/test/data/pa... components/test/data/password_manager/README:82: NewWebsiteTest("website name", "username", "password")) As we discussed off-line: please remove the username and password construction arguments both from the README examples, and from the code. This is just for the sake of easier understanding on first reading, so that test users are not confused by the many ways to set up the usernam/password pairs. If we turn out to need the construction arguments in the future, we can always easily add them. https://codereview.chromium.org/273523004/diff/290001/components/test/data/pa... File components/test/data/password_manager/environment.py (right): https://codereview.chromium.org/273523004/diff/290001/components/test/data/pa... components/test/data/password_manager/environment.py:151: else: nit: remove "else" (and indent the code below 2 columns left); the script execution would not continue after "raise" anyway. (This is similar to "Don't use else after return:" in http://dev.chromium.org/developers/coding-style#Code_Formatting .) https://codereview.chromium.org/273523004/diff/290001/components/test/data/pa... components/test/data/password_manager/environment.py:180: def _WaitUntilPromptAppear(self, log_message, timeout): Please add a documentation comment for this method, explaining what is the purpose of log_message and timeout. https://codereview.chromium.org/273523004/diff/290001/components/test/data/pa... components/test/data/password_manager/environment.py:180: def _WaitUntilPromptAppear(self, log_message, timeout): Maybe rename to _DidPromptAppearUntilTimeout to emphasise that this returns whether the prompt appeared. https://codereview.chromium.org/273523004/diff/290001/components/test/data/pa... components/test/data/password_manager/environment.py:184: return True What about adding self.save_count = count before "return True" here, and removing "self.save_count += 1" from below? I'm suggesting this for 2 purposes: 1) Doing the save_count update right away is more robust than relying on the caller to do that, and makes more sense in reading the code (right now, one has to have read both methods to understand the role of count and save_count). 2) assigning count to save_count, rather than just adding 1 to save_count is more robust against problems in the internals page (if, for some reason, prompt notification was duplicated, and thus count was more than save_count+1); it is also consistent with your test "count > self.save_count" (because the test is not count == self.save_count+1). https://codereview.chromium.org/273523004/diff/290001/components/test/data/pa... components/test/data/password_manager/environment.py:220: prompt_test: If True, tests caring about the save-password prompt are nit: "caring about the save-password prompt" -> "caring about showing the save-password prompt" https://codereview.chromium.org/273523004/diff/290001/components/test/data/pa... components/test/data/password_manager/environment.py:221: going to be ran, otherwise tests which don't are going to be typu: ran -> run https://codereview.chromium.org/273523004/diff/290001/components/test/data/pa... components/test/data/password_manager/environment.py:221: going to be ran, otherwise tests which don't are going to be nit: "don't" -> "don't care about the prompt" https://codereview.chromium.org/273523004/diff/290001/components/test/data/pa... components/test/data/password_manager/environment.py:236: prompt_test: If True, tests caring about the save-password prompt are Please apply the changes to the explanation of promt_test made for AllTests(). Also apply them below at Test(). https://codereview.chromium.org/273523004/diff/290001/components/test/data/pa... File components/test/data/password_manager/tests.py (right): https://codereview.chromium.org/273523004/diff/290001/components/test/data/pa... components/test/data/password_manager/tests.py:428: environment.AllTests(not enable_automatic_password_saving) nit: Maybe instead of using "not enable_automatic_password_saving" here and below, define a new variable |run_prompt_tests| explaining the relation to prompt tests and use that: # Test which care about the save-password prompt need the prompt # to be shown. Automatic password saving results in no prompt. run_prompt_tests = not enable_automatic_password_saving https://codereview.chromium.org/273523004/diff/290001/components/test/data/pa... components/test/data/password_manager/tests.py:429: elif len(tests): Please use "tests", not "len(tests)". This is explained in http://legacy.python.org/dev/peps/pep-0008/#programming-recommendations, as pointed out in my previous comment. https://codereview.chromium.org/273523004/diff/290001/components/test/data/pa... components/test/data/password_manager/tests.py:450: help="Set the profile path (required). You just need to choose a \ This seems to include also the spaces caused by indentation on the next line. Due to the opened parenthesis 2 lines above, you might actually split the strings one per line, and they will get concatenated. Watch also the indentation: help="Set ... choose a " "temporary ... is " "going to be removed.", nargs=1, required=True) (I'm learning Python syntax on the go, so sorry for not pointing this out before, when I mentioned the backslash. :)) https://codereview.chromium.org/273523004/diff/290001/components/test/data/pa... File components/test/data/password_manager/websitetest.py (right): https://codereview.chromium.org/273523004/diff/290001/components/test/data/pa... components/test/data/password_manager/websitetest.py:64: # The |remaining_time_to_wait| limits the total time spent in potentially nit: add units (seconds?) https://codereview.chromium.org/273523004/diff/290001/components/test/data/pa... components/test/data/password_manager/websitetest.py:143: """Checks whether an element would be visible to the user: If it doesn't The "visible to the user" contradicts both "100% transparent" and "covered or out of viewing area". Assuming the returned value is True for at least one of the two latter situations, this comment is wrong. https://codereview.chromium.org/273523004/diff/290001/components/test/data/pa... components/test/data/password_manager/websitetest.py:144: exist in the DOM or is 100% transparent True is returned. Otherwise, even You mentioned True twice. Did you mean "False" in one of the cases? https://codereview.chromium.org/273523004/diff/290001/components/test/data/pa... components/test/data/password_manager/websitetest.py:218: element.click() Why don't you use ClickIfClickable? That would be easier to read, and protect against code duplication (and nested "except" blocks in this case). https://codereview.chromium.org/273523004/diff/290001/components/test/data/pa... components/test/data/password_manager/websitetest.py:353: test: Checks that the password is autofilled, tries to login with a right nit: "a right" -> "the autofilled"
Hi, Could you please check the cl now. Thanks a lot Riadh https://codereview.chromium.org/273523004/diff/290001/components/test/data/pa... File components/test/data/password_manager/README (right): https://codereview.chromium.org/273523004/diff/290001/components/test/data/pa... components/test/data/password_manager/README:82: NewWebsiteTest("website name", "username", "password")) On 2014/05/22 10:07:02, vabr (Chromium) wrote: > As we discussed off-line: please remove the username and password construction > arguments both from the README examples, and from the code. This is just for the > sake of easier understanding on first reading, so that test users are not > confused by the many ways to set up the usernam/password pairs. If we turn out > to need the construction arguments in the future, we can always easily add them. Done. https://codereview.chromium.org/273523004/diff/290001/components/test/data/pa... File components/test/data/password_manager/environment.py (right): https://codereview.chromium.org/273523004/diff/290001/components/test/data/pa... components/test/data/password_manager/environment.py:151: else: On 2014/05/22 10:07:02, vabr (Chromium) wrote: > nit: remove "else" (and indent the code below 2 columns left); the script > execution would not continue after "raise" anyway. > (This is similar to "Don't use else after return:" in > http://dev.chromium.org/developers/coding-style#Code_Formatting .) Done. https://codereview.chromium.org/273523004/diff/290001/components/test/data/pa... components/test/data/password_manager/environment.py:180: def _WaitUntilPromptAppear(self, log_message, timeout): On 2014/05/22 10:07:02, vabr (Chromium) wrote: > Maybe rename to _DidPromptAppearUntilTimeout > to emphasise that this returns whether the prompt appeared. Done. https://codereview.chromium.org/273523004/diff/290001/components/test/data/pa... components/test/data/password_manager/environment.py:180: def _WaitUntilPromptAppear(self, log_message, timeout): On 2014/05/22 10:07:02, vabr (Chromium) wrote: > Please add a documentation comment for this method, explaining what is the > purpose of log_message and timeout. Done. https://codereview.chromium.org/273523004/diff/290001/components/test/data/pa... components/test/data/password_manager/environment.py:184: return True On 2014/05/22 10:07:02, vabr (Chromium) wrote: > What about adding > self.save_count = count > before "return True" here, and removing "self.save_count += 1" from below? > > I'm suggesting this for 2 purposes: > 1) Doing the save_count update right away is more robust than relying on the > caller to do that, and makes more sense in reading the code (right now, one has > to have read both methods to understand the role of count and save_count). > 2) assigning count to save_count, rather than just adding 1 to save_count is > more robust against problems in the internals page (if, for some reason, prompt > notification was duplicated, and thus count was more than save_count+1); it is > also consistent with your test "count > self.save_count" (because the test is > not count == self.save_count+1). Done. https://codereview.chromium.org/273523004/diff/290001/components/test/data/pa... components/test/data/password_manager/environment.py:220: prompt_test: If True, tests caring about the save-password prompt are On 2014/05/22 10:07:02, vabr (Chromium) wrote: > nit: "caring about the save-password prompt" -> "caring about showing the > save-password prompt" Done. https://codereview.chromium.org/273523004/diff/290001/components/test/data/pa... components/test/data/password_manager/environment.py:221: going to be ran, otherwise tests which don't are going to be On 2014/05/22 10:07:02, vabr (Chromium) wrote: > typu: ran -> run Done. https://codereview.chromium.org/273523004/diff/290001/components/test/data/pa... components/test/data/password_manager/environment.py:221: going to be ran, otherwise tests which don't are going to be On 2014/05/22 10:07:02, vabr (Chromium) wrote: > nit: "don't" -> "don't care about the prompt" Done. https://codereview.chromium.org/273523004/diff/290001/components/test/data/pa... components/test/data/password_manager/environment.py:236: prompt_test: If True, tests caring about the save-password prompt are On 2014/05/22 10:07:02, vabr (Chromium) wrote: > Please apply the changes to the explanation of promt_test made for AllTests(). > Also apply them below at Test(). Done. https://codereview.chromium.org/273523004/diff/290001/components/test/data/pa... File components/test/data/password_manager/tests.py (right): https://codereview.chromium.org/273523004/diff/290001/components/test/data/pa... components/test/data/password_manager/tests.py:428: environment.AllTests(not enable_automatic_password_saving) On 2014/05/22 10:07:02, vabr (Chromium) wrote: > nit: Maybe instead of using "not enable_automatic_password_saving" here and > below, define a new variable |run_prompt_tests| explaining the relation to > prompt tests and use that: > > # Test which care about the save-password prompt need the prompt > # to be shown. Automatic password saving results in no prompt. > run_prompt_tests = not enable_automatic_password_saving Done. https://codereview.chromium.org/273523004/diff/290001/components/test/data/pa... components/test/data/password_manager/tests.py:429: elif len(tests): On 2014/05/22 10:07:02, vabr (Chromium) wrote: > Please use "tests", not "len(tests)". > This is explained in > http://legacy.python.org/dev/peps/pep-0008/#programming-recommendations, as > pointed out in my previous comment. Done. https://codereview.chromium.org/273523004/diff/290001/components/test/data/pa... components/test/data/password_manager/tests.py:450: help="Set the profile path (required). You just need to choose a \ On 2014/05/22 10:07:02, vabr (Chromium) wrote: > This seems to include also the spaces caused by indentation on the next line. > Due to the opened parenthesis 2 lines above, you might actually split the > strings one per line, and they will get concatenated. Watch also the > indentation: > help="Set ... choose a " > "temporary ... is " > "going to be removed.", > nargs=1, required=True) > > (I'm learning Python syntax on the go, so sorry for not pointing this out > before, when I mentioned the backslash. :)) Done. https://codereview.chromium.org/273523004/diff/290001/components/test/data/pa... File components/test/data/password_manager/websitetest.py (right): https://codereview.chromium.org/273523004/diff/290001/components/test/data/pa... components/test/data/password_manager/websitetest.py:64: # The |remaining_time_to_wait| limits the total time spent in potentially On 2014/05/22 10:07:02, vabr (Chromium) wrote: > nit: add units (seconds?) Done. https://codereview.chromium.org/273523004/diff/290001/components/test/data/pa... components/test/data/password_manager/websitetest.py:143: """Checks whether an element would be visible to the user: If it doesn't On 2014/05/22 10:07:02, vabr (Chromium) wrote: > The "visible to the user" contradicts both "100% transparent" and "covered or > out of viewing area". Assuming the returned value is True for at least one of > the two latter situations, this comment is wrong. Done. https://codereview.chromium.org/273523004/diff/290001/components/test/data/pa... components/test/data/password_manager/websitetest.py:144: exist in the DOM or is 100% transparent True is returned. Otherwise, even On 2014/05/22 10:07:02, vabr (Chromium) wrote: > You mentioned True twice. Did you mean "False" in one of the cases? Done. https://codereview.chromium.org/273523004/diff/290001/components/test/data/pa... components/test/data/password_manager/websitetest.py:218: element.click() I don't used because ClickIfClickable because I need the css selector which is fine for the password_element but not that fine for the parents. https://codereview.chromium.org/273523004/diff/290001/components/test/data/pa... components/test/data/password_manager/websitetest.py:353: test: Checks that the password is autofilled, tries to login with a right On 2014/05/22 10:07:02, vabr (Chromium) wrote: > nit: "a right" -> "the autofilled" Done.
Thanks, Riadh. The number of comments decreased again :), but we still need at least one round -- please take a look and address the comments below. Feel free to talk to me if you want to discuss anything. Cheers, Vaclav https://codereview.chromium.org/273523004/diff/290001/components/test/data/pa... File components/test/data/password_manager/websitetest.py (right): https://codereview.chromium.org/273523004/diff/290001/components/test/data/pa... components/test/data/password_manager/websitetest.py:218: element.click() On 2014/05/22 12:42:01, rchtara wrote: > I don't used because ClickIfClickable because I need the css selector which is > fine for the password_element but not that fine for the parents. Fair enough, let's keep it this way then. https://codereview.chromium.org/273523004/diff/290002/components/test/data/pa... File components/test/data/password_manager/README (right): https://codereview.chromium.org/273523004/diff/290002/components/test/data/pa... components/test/data/password_manager/README:84: * For security reasons, you have to use websites.xml which is a private to The name websites.xml is not fixed, so please do not use it. Please consider replacing the comment with: "* For security reasons, passwords and usernames need to be supplied in a separate XML file and never checked in to the repository. The XML file should contain data structured like this:" https://codereview.chromium.org/273523004/diff/290002/components/test/data/pa... File components/test/data/password_manager/environment.py (right): https://codereview.chromium.org/273523004/diff/290002/components/test/data/pa... components/test/data/password_manager/environment.py:84: if passwords_path: Once the path to the passwords is no longer optional (see my comment below), just assign the result of parse() to passwords_tree, without the if. https://codereview.chromium.org/273523004/diff/290002/components/test/data/pa... components/test/data/password_manager/environment.py:89: self.save_count = 0 I see a potential problem: |save_count| is used to count messages in the internals page log. But although there are different messages ("ASK..." vs. "SAVE..."), there is only one save_count. The reason why this actually works is that during one browsing session, either only prompt-interested tests are run, or the other tests, which means either only "ASK..." is searched for, or only "SAVE...". To avoid future bugs, I suggest both 1) renaming this counter to, say, message_count (so that it does not indicate a particular message), and 2) add a method UpdateMessageCount(message) which will count the number of occurrences of |message| on the internals page and store it in |message_count|. After that, you need to make sure UpdateMessageCount is called before the prompt-triggering action is taken. I'm open to alternative suggestions to deal with this, if you have any. https://codereview.chromium.org/273523004/diff/290002/components/test/data/pa... components/test/data/password_manager/environment.py:179: def _DidPromptAppearUntilTimeout(self, log_message, timeout): Looking at the code again, this actually does not check for the prompt, but for the message in the internals page. So maybe we should rename to _DidMessageAppearUntilTimeout. (Sorry for changing the name again, I should have spotted it before.) https://codereview.chromium.org/273523004/diff/290002/components/test/data/pa... components/test/data/password_manager/environment.py:205: def CheckPrompt(self, log_message, prompt_should_show_up, Also here: I suggest renaming to CheckForNewMessage + renaming "prompt" to "message" in the arguments. https://codereview.chromium.org/273523004/diff/290002/components/test/data/pa... components/test/data/password_manager/environment.py:223: if (self._WaitUntilPromptAppear(log_message, timeout) != Outdated method name. https://codereview.chromium.org/273523004/diff/290002/components/test/data/pa... components/test/data/password_manager/environment.py:233: the prompt are going to be executed. nit: Be consistent: either use "run" or "executed"; having two expressions is confusing, as it sounds like you mean two different things. Also below. https://codereview.chromium.org/273523004/diff/290002/components/test/data/pa... File components/test/data/password_manager/tests.py (right): https://codereview.chromium.org/273523004/diff/290002/components/test/data/pa... components/test/data/password_manager/tests.py:460: help="Set the usernames/passwords path (optional).", nargs=1) Let's make this no longer optional, so that the test users know they have to supply the passwords. https://codereview.chromium.org/273523004/diff/290002/components/test/data/pa... File components/test/data/password_manager/websitetest.py (right): https://codereview.chromium.org/273523004/diff/290002/components/test/data/pa... components/test/data/password_manager/websitetest.py:79: """Clicks on an element if it's clickable: If it doesn't exist in the DOM, This is still inconsistent with IsDisplayed: ClickIfClickable IsDisplayed not in DOM False False covered False True out of area False True transparent True False visible True True Since both methods use find_element_by_css_selector, I guess the columns above should be identical, no?
Hi, Could you please check this one. Thanks a lot Riadh https://codereview.chromium.org/273523004/diff/290001/components/test/data/pa... File components/test/data/password_manager/websitetest.py (right): https://codereview.chromium.org/273523004/diff/290001/components/test/data/pa... components/test/data/password_manager/websitetest.py:218: element.click() On 2014/05/22 13:39:24, vabr (Chromium) wrote: > On 2014/05/22 12:42:01, rchtara wrote: > > I don't used because ClickIfClickable because I need the css selector which is > > fine for the password_element but not that fine for the parents. > > Fair enough, let's keep it this way then. Done. https://codereview.chromium.org/273523004/diff/290002/components/test/data/pa... File components/test/data/password_manager/README (right): https://codereview.chromium.org/273523004/diff/290002/components/test/data/pa... components/test/data/password_manager/README:84: * For security reasons, you have to use websites.xml which is a private to On 2014/05/22 13:39:24, vabr (Chromium) wrote: > The name websites.xml is not fixed, so please do not use it. Please consider > replacing the comment with: > "* For security reasons, passwords and usernames need to be supplied in a > separate XML file and never checked in to the repository. The XML file should > contain data structured like this:" Done. https://codereview.chromium.org/273523004/diff/290002/components/test/data/pa... File components/test/data/password_manager/environment.py (right): https://codereview.chromium.org/273523004/diff/290002/components/test/data/pa... components/test/data/password_manager/environment.py:84: if passwords_path: On 2014/05/22 13:39:24, vabr (Chromium) wrote: > Once the path to the passwords is no longer optional (see my comment below), > just assign the result of parse() to passwords_tree, without the if. Done. https://codereview.chromium.org/273523004/diff/290002/components/test/data/pa... components/test/data/password_manager/environment.py:89: self.save_count = 0 On 2014/05/22 13:39:24, vabr (Chromium) wrote: > I see a potential problem: |save_count| is used to count messages in the > internals page log. But although there are different messages ("ASK..." vs. > "SAVE..."), there is only one save_count. > > The reason why this actually works is that during one browsing session, either > only prompt-interested tests are run, or the other tests, which means either > only "ASK..." is searched for, or only "SAVE...". > > To avoid future bugs, I suggest both > 1) renaming this counter to, say, message_count (so that it does not indicate a > particular message), and > 2) add a method UpdateMessageCount(message) which will count the number of > occurrences of |message| on the internals page and store it in |message_count|. > > After that, you need to make sure UpdateMessageCount is called before the > prompt-triggering action is taken. > > I'm open to alternative suggestions to deal with this, if you have any. Done. https://codereview.chromium.org/273523004/diff/290002/components/test/data/pa... components/test/data/password_manager/environment.py:179: def _DidPromptAppearUntilTimeout(self, log_message, timeout): no problem :) Done. https://codereview.chromium.org/273523004/diff/290002/components/test/data/pa... components/test/data/password_manager/environment.py:205: def CheckPrompt(self, log_message, prompt_should_show_up, On 2014/05/22 13:39:24, vabr (Chromium) wrote: > Also here: I suggest renaming to CheckForNewMessage + renaming "prompt" to > "message" in the arguments. Done. https://codereview.chromium.org/273523004/diff/290002/components/test/data/pa... components/test/data/password_manager/environment.py:223: if (self._WaitUntilPromptAppear(log_message, timeout) != On 2014/05/22 13:39:24, vabr (Chromium) wrote: > Outdated method name. Done. https://codereview.chromium.org/273523004/diff/290002/components/test/data/pa... components/test/data/password_manager/environment.py:233: the prompt are going to be executed. On 2014/05/22 13:39:24, vabr (Chromium) wrote: > nit: Be consistent: either use "run" or "executed"; having two expressions is > confusing, as it sounds like you mean two different things. > Also below. Done. https://codereview.chromium.org/273523004/diff/290002/components/test/data/pa... File components/test/data/password_manager/tests.py (right): https://codereview.chromium.org/273523004/diff/290002/components/test/data/pa... components/test/data/password_manager/tests.py:460: help="Set the usernames/passwords path (optional).", nargs=1) On 2014/05/22 13:39:24, vabr (Chromium) wrote: > Let's make this no longer optional, so that the test users know they have to > supply the passwords. Done. https://codereview.chromium.org/273523004/diff/290002/components/test/data/pa... File components/test/data/password_manager/websitetest.py (right): https://codereview.chromium.org/273523004/diff/290002/components/test/data/pa... components/test/data/password_manager/websitetest.py:79: """Clicks on an element if it's clickable: If it doesn't exist in the DOM, This is what thought of before, but when I tried to test them yesterday, I discovered that they are different which weird by the way.
Hi, Could you please check this one. Thanks a lot Riadh
Thanks, Riadh. LGTM, but please address the last 3 of the 4 comments below. Thank you, Vaclav https://codereview.chromium.org/273523004/diff/290002/components/test/data/pa... File components/test/data/password_manager/websitetest.py (right): https://codereview.chromium.org/273523004/diff/290002/components/test/data/pa... components/test/data/password_manager/websitetest.py:79: """Clicks on an element if it's clickable: If it doesn't exist in the DOM, On 2014/05/22 15:20:56, rchtara wrote: > This is what thought of before, but when I tried to test them yesterday, I > discovered that they are different which weird by the way. OK, I can see how the .click() can throw more exceptions in addition to find_element_by_css_selector(), so I'm fine with the situation when ClickIfClickable returns False even though IsDisplayed returns True. The only remaining case is the transparent one, and in that case although find_element_by_css_selector() succeeds, is_displayed() probably simply returns False, which makes sense. So this sounds OK. Thanks for the thorough comments for both methods, they are really helpful. https://codereview.chromium.org/273523004/diff/360001/components/test/data/pa... File components/test/data/password_manager/environment.py (right): https://codereview.chromium.org/273523004/diff/360001/components/test/data/pa... components/test/data/password_manager/environment.py:88: self.message_count["Message: Decision: ASK the user"] = 0 nit: Since you provide the strings on multiple places, please define them as constants, on the global level (outside of class definition, right after the import statements): # Message strings to look for in chrome://password-manager-internals MESSAGE_ASK = "Message: ... ASK ..." MESSAGE_SAVE = "Message: ... SAVE ..." (See also http://legacy.python.org/dev/peps/pep-0008/#constants for the naming conventions.) https://codereview.chromium.org/273523004/diff/360001/components/test/data/pa... components/test/data/password_manager/environment.py:211: log_message: Log message to look for in the password internals. Because for arbitrary messages this will throw an exception (key not found in |message_count|), it would be worth noting, that the only valid values are the constants MESSAGE_* defined at the beginning of this file. https://codereview.chromium.org/273523004/diff/360001/components/test/data/pa... File components/test/data/password_manager/tests.py (right): https://codereview.chromium.org/273523004/diff/360001/components/test/data/pa... components/test/data/password_manager/tests.py:475: passwords_path = None nit: Now that passwords_path is required, you can use args.passwords_path[0] directly. Please remove the helper variable |passwords_path| and the "if" clause.
Hi, Awesome, I'm going to push the changes Cheers Riadh https://codereview.chromium.org/273523004/diff/290002/components/test/data/pa... File components/test/data/password_manager/websitetest.py (right): https://codereview.chromium.org/273523004/diff/290002/components/test/data/pa... components/test/data/password_manager/websitetest.py:79: """Clicks on an element if it's clickable: If it doesn't exist in the DOM, You re welecome https://codereview.chromium.org/273523004/diff/360001/components/test/data/pa... File components/test/data/password_manager/environment.py (right): https://codereview.chromium.org/273523004/diff/360001/components/test/data/pa... components/test/data/password_manager/environment.py:88: self.message_count["Message: Decision: ASK the user"] = 0 On 2014/05/22 15:46:24, vabr (Chromium) wrote: > nit: Since you provide the strings on multiple places, please define them as > constants, on the global level (outside of class definition, right after the > import statements): > > # Message strings to look for in chrome://password-manager-internals > MESSAGE_ASK = "Message: ... ASK ..." > MESSAGE_SAVE = "Message: ... SAVE ..." > > (See also http://legacy.python.org/dev/peps/pep-0008/#constants for the naming > conventions.) Done. https://codereview.chromium.org/273523004/diff/360001/components/test/data/pa... components/test/data/password_manager/environment.py:211: log_message: Log message to look for in the password internals. On 2014/05/22 15:46:24, vabr (Chromium) wrote: > Because for arbitrary messages this will throw an exception (key not found in > |message_count|), it would be worth noting, that the only valid values are the > constants MESSAGE_* defined at the beginning of this file. Done. https://codereview.chromium.org/273523004/diff/360001/components/test/data/pa... File components/test/data/password_manager/tests.py (right): https://codereview.chromium.org/273523004/diff/360001/components/test/data/pa... components/test/data/password_manager/tests.py:475: passwords_path = None On 2014/05/22 15:46:24, vabr (Chromium) wrote: > nit: Now that passwords_path is required, you can use args.passwords_path[0] > directly. Please remove the helper variable |passwords_path| and the "if" > clause. Done.
The CQ bit was checked by rchtara@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rchtara@chromium.org/273523004/360002
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
Hi Riadh, There were a couple of presubmit errors. 1) Bug in the description needed to be BUG. I fixed this, and shortened the description a bit, please take a look if that's OK with you. 2) Missing copyright headers in the files. Could you please follow the information in the presubmit error log and possibly have a look at other python sources in Chromium, and add the copyright headers? Thanks! 3) OWNERS approval -- I thought that I have sufficient OWNERShip for this, but apparently I don't. :) I'll take care of this ASAP and let you know once it's OK to land this. Cheers, Vaclav
Hi Vaclav, OK,I have added the copyright message. Thanks Riadh
On 2014/05/23 07:25:46, rchtara wrote: > Hi Vaclav, > OK,I have added the copyright message. > Thanks > Riadh Great, LGTM. In the meantime, https://codereview.chromium.org/296513020/ will solve the OWNERS issue. It's currently waiting for the tree to be open. I could ask the sheriff for a permission to land it anyway, because it's not a code change, but it won't unblock your CL, which needs to wait for the tree to open. So I won't bother our busy sheriff. :) I'll update this CL once https://codereview.chromium.org/296513020/ lands. Cheers, Vaclav
Hi vaclav, :) No problem, I'm not blocked with that so of course i can wait. Thanks a lot Riadh On Fri, May 23, 2014 at 10:45 AM, <vabr@chromium.org> wrote: > On 2014/05/23 07:25:46, rchtara wrote: > >> Hi Vaclav, >> OK,I have added the copyright message. >> Thanks >> Riadh >> > > Great, LGTM. > In the meantime, https://codereview.chromium.org/296513020/ will solve the > OWNERS issue. > It's currently waiting for the tree to be open. I could ask the sheriff > for a > permission to land it anyway, because it's not a code change, but it won't > unblock your CL, which needs to wait for the tree to open. So I won't > bother our > busy sheriff. :) > > I'll update this CL once https://codereview.chromium.org/296513020/ lands. > > Cheers, > Vaclav > > https://codereview.chromium.org/273523004/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Hi Riadh, Just to clarify: the point was that this CL needs to wait until the tree is open, and that at that point the OWNERS CL will land automatically (so the blocker is the tree, not the other CL). But good to hear you have other work to fill the waiting with. :) Cheers, Vaclav On 2014/05/23 08:51:16, chromium-reviews_chromium.org wrote: > Hi vaclav, > :) No problem, I'm not blocked with that so of course i can wait. > Thanks a lot > Riadh > > > On Fri, May 23, 2014 at 10:45 AM, <mailto:vabr@chromium.org> wrote: > > > On 2014/05/23 07:25:46, rchtara wrote: > > > >> Hi Vaclav, > >> OK,I have added the copyright message. > >> Thanks > >> Riadh > >> > > > > Great, LGTM. > > In the meantime, https://codereview.chromium.org/296513020/ will solve the > > OWNERS issue. > > It's currently waiting for the tree to be open. I could ask the sheriff > > for a > > permission to land it anyway, because it's not a code change, but it won't > > unblock your CL, which needs to wait for the tree to open. So I won't > > bother our > > busy sheriff. :) > > > > I'll update this CL once https://codereview.chromium.org/296513020/ lands. > > > > Cheers, > > Vaclav > > > > https://codereview.chromium.org/273523004/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
Hi, Thanks for the clarification. Riadh
The CQ bit was checked by vabr@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rchtara@chromium.org/273523004/390001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
Message was sent while issue was closed.
Change committed as 272545 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
