Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
386423002:
Allow the test bots to easily run the password manager python tests. (Closed)
DescriptionAllow the test bots to easily run the password manager python
tests.
Until now, setting up password manager python tests is
complicated and doesn't fit the requirement of the test
bots. This cl allows to make running and configuring of the
tests much easier for the bots.
BUG=393531
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=283806
Patch Set 1 #
Total comments: 12
Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #
Total comments: 11
Patch Set 5 : #
Total comments: 6
Patch Set 6 : #Patch Set 7 : #
Total comments: 4
Patch Set 8 : #
Messages
Total messages: 27 (0 generated)
Hi, Could you please review this cl? Thanks a lot Riadh
Hi, Could you please review this cl? Thanks a lot Riadh
+sergiyb, because there is one question for you in my comments, Sergiy (look for "@sergiyb:"). Thanks, Riadh, Please have a look at my comments. Cheers, Vaclav https://codereview.chromium.org/386423002/diff/1/components/test/data/passwor... File components/test/data/password_manager/run_tests.py (right): https://codereview.chromium.org/386423002/diff/1/components/test/data/passwor... components/test/data/password_manager/run_tests.py:42: results = tempfile.NamedTemporaryFile(dir=os.path.join(tempfile.gettempdir()), nit: Could you break the line so that all the arguments are on one line? I.e.: var = function( arg, arg, arg) https://codereview.chromium.org/386423002/diff/1/components/test/data/passwor... components/test/data/password_manager/run_tests.py:43: delete=False) Instead of creating the results here, and with delete=False, I suggest only creating it just before using it (below), with delete=True. I understand that one of the requirements for tests run on trybots is that they clean up after themselves, so we should make sure that no files created are kept on the disk. If you need to keep the code this way, then please make sure to manually delete |results| after the test is finished (and please check that that happens even if some code between throws an uncaught exception). https://codereview.chromium.org/386423002/diff/1/components/test/data/passwor... components/test/data/password_manager/run_tests.py:53: # repeat the tests three times. nit: repeat->Repeat (A capital letter at the beginning.) https://codereview.chromium.org/386423002/diff/1/components/test/data/passwor... components/test/data/password_manager/run_tests.py:53: # repeat the tests three times. Could you please also explain why the tests are repeated 3 times? https://codereview.chromium.org/386423002/diff/1/components/test/data/passwor... components/test/data/password_manager/run_tests.py:61: os.system("timeout 300 python %s %s --chrome-path %s --chromedriver-path " @sergiyb: Is this the way tests on trybots should impose time restrictions on their runtime? Or should specifying timeout be part of the recipe or other piece of configuration? https://codereview.chromium.org/386423002/diff/1/components/test/data/passwor... components/test/data/password_manager/run_tests.py:61: os.system("timeout 300 python %s %s --chrome-path %s --chromedriver-path " Please comment on the choice of 300 (including stating it means 300 seconds).
Hi, Could you please fix this cl again? Thanks a lot Riadh
https://codereview.chromium.org/386423002/diff/1/components/test/data/passwor... File components/test/data/password_manager/run_tests.py (right): https://codereview.chromium.org/386423002/diff/1/components/test/data/passwor... components/test/data/password_manager/run_tests.py:42: results = tempfile.NamedTemporaryFile(dir=os.path.join(tempfile.gettempdir()), On 2014/07/14 12:36:09, vabr (Chromium) wrote: > nit: Could you break the line so that all the arguments are on one line? I.e.: > var = function( > arg, arg, arg) Done. https://codereview.chromium.org/386423002/diff/1/components/test/data/passwor... components/test/data/password_manager/run_tests.py:43: delete=False) On 2014/07/14 12:36:09, vabr (Chromium) wrote: > Instead of creating the results here, and with delete=False, I suggest only > creating it just before using it (below), with delete=True. > > I understand that one of the requirements for tests run on trybots is that they > clean up after themselves, so we should make sure that no files created are kept > on the disk. > > If you need to keep the code this way, then please make sure to manually delete > |results| after the test is finished (and please check that that happens even if > some code between throws an uncaught exception). Done. https://codereview.chromium.org/386423002/diff/1/components/test/data/passwor... components/test/data/password_manager/run_tests.py:53: # repeat the tests three times. On 2014/07/14 12:36:10, vabr (Chromium) wrote: > nit: repeat->Repeat (A capital letter at the beginning.) Done. https://codereview.chromium.org/386423002/diff/1/components/test/data/passwor... components/test/data/password_manager/run_tests.py:61: os.system("timeout 300 python %s %s --chrome-path %s --chromedriver-path " On 2014/07/14 12:36:09, vabr (Chromium) wrote: > Please comment on the choice of 300 (including stating it means 300 seconds). Done.
Thanks, Riadh! A couple of more comments below. Cheers, Vaclav https://codereview.chromium.org/386423002/diff/60001/components/test/data/pas... File components/test/data/password_manager/run_tests.py (right): https://codereview.chromium.org/386423002/diff/60001/components/test/data/pas... components/test/data/password_manager/run_tests.py:46: delete=False) nit: That's not what I meant -- the arguments do fit all on one line, don't they? results = tempfile.NamedTemporaryFile( dir=os.path.join(tempfile.gettempdir()), delete=False) https://codereview.chromium.org/386423002/diff/60001/components/test/data/pas... components/test/data/password_manager/run_tests.py:56: # not related to the password manager(ex: unexpected chrome crash). This nits: (1) space before "(" (2) ex: -> e.g., https://codereview.chromium.org/386423002/diff/60001/components/test/data/pas... components/test/data/password_manager/run_tests.py:56: # not related to the password manager(ex: unexpected chrome crash). This I don't think we should tolerate unexpected chrome crashes. If Chrome crashes, that test needs to be disabled and a bug for that filed. https://codereview.chromium.org/386423002/diff/60001/components/test/data/pas... components/test/data/password_manager/run_tests.py:57: # is why we need to run them many times to reduce the effect of such I can see 2 issues with the current approach: (1) All tests are retried, regardless of their success. (2) There seems to be no filtering of the results for tests which both fail and succeed (during different runs). Would a test failing at some run make the overall result look like failed? I'm afraid, there needs to be a more clever logic to handle this, which keeps re-running only failed tests (at most 3 times), and marks the whole test job as failed if and only if there are still some failed runs in the last round. https://codereview.chromium.org/386423002/diff/60001/components/test/data/pas... components/test/data/password_manager/run_tests.py:68: # it's more than enough to run the test. "it's more than enough" -- that's a bold claim. If we had been confident it's more than enough, we should have made it shorter. Instead, I suggest referring to the |remaining_time_to_wait| timeout limit set in websitetest.py, and increasing the number below back to 300 seconds -- the remaining_time_to_wait timeout is 200, and it does not include everything, only time spent in Wait() calls. https://codereview.chromium.org/386423002/diff/60001/components/test/data/pas... components/test/data/password_manager/run_tests.py:84: except Exception: Are you sure you want to flush all exceptions here? This will make it impossible to understand what happened in the try block. Did you mean to use a finally clause for removing the results instead? try: #all the test execution here finally: os.remove(results_path)
Hi Sergiy, > https://codereview.chromium.org/386423002/diff/1/components/test/data/passwor... > components/test/data/password_manager/run_tests.py:61: os.system("timeout 300 > python %s %s --chrome-path %s --chromedriver-path " > @sergiyb: Is this the way tests on trybots should impose time restrictions on > their runtime? Or should specifying timeout be part of the recipe or other piece > of configuration? Any thoughts on the above? Cheers, Vaclav
https://codereview.chromium.org/386423002/diff/1/components/test/data/passwor... File components/test/data/password_manager/run_tests.py (right): https://codereview.chromium.org/386423002/diff/1/components/test/data/passwor... components/test/data/password_manager/run_tests.py:61: os.system("timeout 300 python %s %s --chrome-path %s --chromedriver-path " On 2014/07/14 12:36:10, vabr (Chromium) wrote: > @sergiyb: Is this the way tests on trybots should impose time restrictions on > their runtime? Or should specifying timeout be part of the recipe or other piece > of configuration? That doesn't look like a good approach and I hope there is a better way. Please ask on chrome-recipes@ as I have not actually written any recipes. I only know about them theoretically.
https://codereview.chromium.org/386423002/diff/1/components/test/data/passwor... File components/test/data/password_manager/run_tests.py (right): https://codereview.chromium.org/386423002/diff/1/components/test/data/passwor... components/test/data/password_manager/run_tests.py:61: os.system("timeout 300 python %s %s --chrome-path %s --chromedriver-path " OK, I have just posted the question in chrome-recipes@. Thanks a lot @sergiyb https://codereview.chromium.org/386423002/diff/60001/components/test/data/pas... File components/test/data/password_manager/run_tests.py (right): https://codereview.chromium.org/386423002/diff/60001/components/test/data/pas... components/test/data/password_manager/run_tests.py:46: delete=False) On 2014/07/16 08:48:44, vabr (Chromium) wrote: > nit: That's not what I meant -- the arguments do fit all on one line, don't > they? > results = tempfile.NamedTemporaryFile( > dir=os.path.join(tempfile.gettempdir()), delete=False) Done. https://codereview.chromium.org/386423002/diff/60001/components/test/data/pas... components/test/data/password_manager/run_tests.py:56: # not related to the password manager(ex: unexpected chrome crash). This On 2014/07/16 08:48:44, vabr (Chromium) wrote: > nits: > (1) space before "(" > (2) ex: -> e.g., Done. https://codereview.chromium.org/386423002/diff/60001/components/test/data/pas... components/test/data/password_manager/run_tests.py:57: # is why we need to run them many times to reduce the effect of such On 2014/07/16 08:48:44, vabr (Chromium) wrote: > I can see 2 issues with the current approach: > (1) All tests are retried, regardless of their success. > (2) There seems to be no filtering of the results for tests which both fail and > succeed (during different runs). Would a test failing at some run make the > overall result look like failed? > > I'm afraid, there needs to be a more clever logic to handle this, which keeps > re-running only failed tests (at most 3 times), and marks the whole test job as > failed if and only if there are still some failed runs in the last round. Done. https://codereview.chromium.org/386423002/diff/60001/components/test/data/pas... components/test/data/password_manager/run_tests.py:68: # it's more than enough to run the test. On 2014/07/16 08:48:44, vabr (Chromium) wrote: > "it's more than enough" -- that's a bold claim. If we had been confident it's > more than enough, we should have made it shorter. > Instead, I suggest referring to the |remaining_time_to_wait| timeout limit set > in websitetest.py, and increasing the number below back to 300 seconds -- the > remaining_time_to_wait timeout is 200, and it does not include everything, only > time spent in Wait() calls. Done. https://codereview.chromium.org/386423002/diff/60001/components/test/data/pas... components/test/data/password_manager/run_tests.py:84: except Exception: On 2014/07/16 08:48:44, vabr (Chromium) wrote: > Are you sure you want to flush all exceptions here? > This will make it impossible to understand what happened in the try block. > Did you mean to use a finally clause for removing the results instead? > try: > #all the test execution here > finally: > os.remove(results_path) Done.
Thanks, Riadh (and Sergiy for response). One question: How does the run_tests.py script indicate the overall success? What I mean -- the normal test suites turn up green or red on the trybot webpage, depending on whether all tests passed, or some did not. How would this be done for your tests? Other than that, LGTM with 3 nits below. Cheers, Vaclav https://codereview.chromium.org/386423002/diff/80001/components/test/data/pas... File components/test/data/password_manager/run_tests.py (right): https://codereview.chromium.org/386423002/diff/80001/components/test/data/pas... components/test/data/password_manager/run_tests.py:55: # not related to the password manager (e.g., unexpected chrome crash). I still oppose to mentioning "unexpected chrome crash" as an accepted reason for failing tests. (Re-running tests because they are flaky is OK and done with other tests too.) Please reformulate the comment. https://codereview.chromium.org/386423002/diff/80001/components/test/data/pas... components/test/data/password_manager/run_tests.py:65: # test. Each of these tests have a |remaining_time_to_wait| timeout nit: I'd suggest a slight rephrasing to indicate that detailed knowledge of the internal tests logic is not required. How about: "The website test runs in two passes, each pass has an internal timeout for 200s for waiting (see |remaining_time_to_wait| and Wait() in websitetest.py). Accounting for some more time spent on the non-waiting execution, 300 seconds should be the upper bound on the runtime of one pass, thus 600 seconds for the whole test." https://codereview.chromium.org/386423002/diff/80001/components/test/data/pas... components/test/data/password_manager/run_tests.py:69: os.system("timeout 600 python %s %s --chrome-path %s " Please add a "TODO(rchtara): " comment explaining that using "timeout" is just temporary until a better, platform-independent solution is found.
Hi, Last check please :) Cheers Riadh
https://codereview.chromium.org/386423002/diff/80001/components/test/data/pas... File components/test/data/password_manager/run_tests.py (right): https://codereview.chromium.org/386423002/diff/80001/components/test/data/pas... components/test/data/password_manager/run_tests.py:55: # not related to the password manager (e.g., unexpected chrome crash). On 2014/07/17 09:15:44, vabr (Chromium) wrote: > I still oppose to mentioning "unexpected chrome crash" as an accepted reason for > failing tests. > > (Re-running tests because they are flaky is OK and done with other tests too.) > > Please reformulate the comment. Done. https://codereview.chromium.org/386423002/diff/80001/components/test/data/pas... components/test/data/password_manager/run_tests.py:65: # test. Each of these tests have a |remaining_time_to_wait| timeout On 2014/07/17 09:15:44, vabr (Chromium) wrote: > nit: I'd suggest a slight rephrasing to indicate that detailed knowledge of the > internal tests logic is not required. How about: > "The website test runs in two passes, each pass has an internal timeout for 200s > for waiting (see |remaining_time_to_wait| and Wait() in websitetest.py). > Accounting for some more time spent on the non-waiting execution, 300 seconds > should be the upper bound on the runtime of one pass, thus 600 seconds for the > whole test." Done. https://codereview.chromium.org/386423002/diff/80001/components/test/data/pas... components/test/data/password_manager/run_tests.py:69: os.system("timeout 600 python %s %s --chrome-path %s " On 2014/07/17 09:15:44, vabr (Chromium) wrote: > Please add a "TODO(rchtara): " comment explaining that using "timeout" is just > temporary until a better, platform-independent solution is found. Done.
Thanks, Riadh, Still LGTM, but you did not answer my previous question: > How does the run_tests.py script indicate the overall > success? What I mean -- the normal test suites turn up > green or red on the trybot webpage, depending on whether > all tests passed, or some did not. How would this be > done for your tests? Also 2 more nits. Cheers, Vaclav https://codereview.chromium.org/386423002/diff/120001/components/test/data/pa... File components/test/data/password_manager/run_tests.py (right): https://codereview.chromium.org/386423002/diff/120001/components/test/data/pa... components/test/data/password_manager/run_tests.py:54: # The tests are flaky. This is why we need to rerun them many times. 2 nits: are flaky -> can be flaky need to rerun many times -> try to rerun up to 3 times https://codereview.chromium.org/386423002/diff/120001/components/test/data/pa... components/test/data/password_manager/run_tests.py:67: # timeout for 200s for waiting (see |remaining_time_to_wait| and nit: timeout for -> timeout of
Ok. Great Thanks for the review Riadh https://codereview.chromium.org/386423002/diff/120001/components/test/data/pa... File components/test/data/password_manager/run_tests.py (right): https://codereview.chromium.org/386423002/diff/120001/components/test/data/pa... components/test/data/password_manager/run_tests.py:54: # The tests are flaky. This is why we need to rerun them many times. On 2014/07/17 14:13:42, vabr (Chromium) wrote: > 2 nits: > are flaky -> can be flaky > need to rerun many times -> try to rerun up to 3 times Done. https://codereview.chromium.org/386423002/diff/120001/components/test/data/pa... components/test/data/password_manager/run_tests.py:67: # timeout for 200s for waiting (see |remaining_time_to_wait| and On 2014/07/17 14:13:42, vabr (Chromium) wrote: > nit: timeout for -> timeout of 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/386423002/110005
Hi Riadh, I'm going to ask my question here for the third time, in a hope for an answer from you. :) > How does the run_tests.py script indicate the overall > success? What I mean -- the normal test suites turn up > green or red on the trybot webpage, depending on whether > all tests passed, or some did not. How would this be > done for your tests? Thanks, Vaclav
The CQ bit was unchecked by rchtara@chromium.org
Hi, Sorry, I totally forgot about it. The script just create an xml file containing the results. But the xml file is going to stay there. However, I have no idea how to send the results. (probably the best idea is to encode the xml file and send it to app engine with urllib as we have an internet connections). Sorry again and thanks for reminding me of this important problem. Riadh
On 2014/07/17 15:52:09, rchtara wrote: > Hi, > Sorry, > I totally forgot about it. > The script just create an xml file containing the results. > But the xml file is going to stay there. > However, I have no idea how to send the results. > (probably the best idea is to encode the xml file and send it to app engine with > urllib as we have an internet connections). > Sorry again and thanks for reminding me of this important problem. > Riadh Thanks for answering, Riadh! I don't think I understand the mechanism fully, but I suggest once you land this and the recipe, that we have a chat about passing the results. And pleas do feel free to send back to the CQ, you have had the approval already. Thanks again, Vaclav
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/386423002/110005
I meant to something like this:
import urllib2, urllib
data=[('xml', xml_data)]
data=urllib.urlencode(mydata)
path='http://dashboard/'
req=urllib2.Request(path, data)
req.add_header("Content-type", "application/x-www-form-urlencoded")
page=urllib2.urlopen(req).read()
In the dashbord webpage we need just to check "xml" value in the posted http
request.
We could chat about this more when we the recipe and the current cl land.
Thanks
Riadh
Message was sent while issue was closed.
Change committed as 283806
Message was sent while issue was closed.
Hi, I made the change you asked me to do in the discussion with aaron. Could you please review them Thanks a lot Riadh
Message was sent while issue was closed.
On 2014/07/18 15:15:02, rchtara wrote: > Hi, > I made the change you asked me to do in the discussion with aaron. > Could you please review them > Thanks a lot > Riadh Hi Riadh, Now that you filed the new changes in https://codereview.chromium.org/403323002/, could you please confirm that patch set 8 is the one which got committed here? And if so, then please delete patch sets 9 and 10 here. Cheers, Vaclav
Message was sent while issue was closed.
Yes, patch 8 is one that was committed here. I'm going to remove patch 9 and patch 10. Thanks Cheers Riadh |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
