Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1089383002:
[Password manager tests automation] Refactor test_runner. (Closed)
|
Created:
5 years, 8 months ago by melandory Modified:
5 years, 8 months ago Reviewers:
vabr (Chromium) CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@base Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Password manager tests automation] Refactor test_runner.
Refactoring consists of following steps:
* test-case is runned for all websites, then another one for all websites.
* module |test| is now executing by importing and running its
function instead of running it as python script.
* platform independent timeout mechanism.
* parallel run using threads and threadpool alike solution to
distribute jobs between them.
BUG=369521
R=vabr@chromium.org
Committed: https://crrev.com/a12e1e10c4a910a4f8aa20e3ea8b611846f19ed9
Cr-Commit-Position: refs/heads/master@{#326495}
Patch Set 1 #Patch Set 2 : #
Total comments: 30
Patch Set 3 : #
Total comments: 5
Patch Set 4 : #Patch Set 5 : #
Messages
Total messages: 15 (4 generated)
Hi Vaclav, review this CL, please. I've changed a logging format. Now final message will be: 36 failed tests out of 117, failing tests: ['dailymotion.PromptSuccessTest', 'dailymotion.SaveAndAutofillTest', 'adobe.SaveAndAutofillTest', 'instagram.PromptFailTest', 'instagram.PromptSuccessTest', 'instagram.SaveAndAutofillTest', 'espn.PromptFailTest', 'espn.PromptSuccessTest', 'espn.SaveAndAutofillTest', 'yahoo.PromptFailTest', 'yahoo.PromptSuccessTest', 'yahoo.SaveAndAutofillTest', 'wikia.PromptSuccessTest', 'wikia.SaveAndAutofillTest', 'aliexpress.SaveAndAutofillTest', 'yandex.PromptFailTest', 'yandex.PromptSuccessTest', 'yandex.SaveAndAutofillTest', 'vube.PromptFailTest', 'vube.PromptSuccessTest', 'vube.SaveAndAutofillTest', '163.PromptFailTest', '163.PromptSuccessTest', '163.SaveAndAutofillTest', 'buzzfeed.SaveAndAutofillTest', 'amazon.PromptFailTest', 'ebay.PromptSuccessTest', 'ebay.SaveAndAutofillTest', 'live.PromptFailTest', 'ziddu.PromptSuccessTest', 'ziddu.SaveAndAutofillTest', 'cnn.PromptFailTest', 'cnn.PromptSuccessTest', 'cnn.SaveAndAutofillTest', 'stackexchange.SaveAndAutofillTest', 'flipkart.SaveAndAutofillTest So difference is in counting and outputting individual test cases instead of websites. https://codereview.chromium.org/1089383002/diff/20001/components/test/data/pa... File components/test/data/password_manager/automated_tests/tests.py (right): https://codereview.chromium.org/1089383002/diff/20001/components/test/data/pa... components/test/data/password_manager/automated_tests/tests.py:512: This is intentional: there are two empty lines between functions across this file.
Thanks for this great improvement. I left some comments and questions, please have a look. Cheers, Vaclav https://codereview.chromium.org/1089383002/diff/20001/components/test/data/pa... File components/test/data/password_manager/automated_tests/run_tests.py (right): https://codereview.chromium.org/1089383002/diff/20001/components/test/data/pa... components/test/data/password_manager/automated_tests/run_tests.py:82: def ProcessRun(config, results): nit: Although "Process" is used as a verb in the name, it can be misunderstood as a noun (and Run as a verb instead of a noun), in which case the name sounds like running a process. Maybe rename to LogResultsOfTestRun? https://codereview.chromium.org/1089383002/diff/20001/components/test/data/pa... components/test/data/password_manager/automated_tests/run_tests.py:91: website, {True: "passed", False: "failed"}[success]) I like this use of a dictionary! :) https://codereview.chromium.org/1089383002/diff/20001/components/test/data/pa... components/test/data/password_manager/automated_tests/run_tests.py:99: logger.info("%d failed tests out of %d, failing tests: %s", nit: Please replace "tests" (both occurrences) with "test cases". https://codereview.chromium.org/1089383002/diff/20001/components/test/data/pa... components/test/data/password_manager/automated_tests/run_tests.py:101: [name for name in failed_tests]) When you are at it, could you also sort the lost of failing test cases? That will make it easy to identify changes in the list of failures between two test runs. https://codereview.chromium.org/1089383002/diff/20001/components/test/data/pa... components/test/data/password_manager/automated_tests/run_tests.py:104: def RunTestCaseOnWebsite((website, test_case, config)): This has to take a one argument, which is a triple, as opposed to 3 arguments, because of line 162, yes? https://codereview.chromium.org/1089383002/diff/20001/components/test/data/pa... components/test/data/password_manager/automated_tests/run_tests.py:105: """ Runs a |test_case| on a |website|. If case when |test_case| has typo: If -> In https://codereview.chromium.org/1089383002/diff/20001/components/test/data/pa... components/test/data/password_manager/automated_tests/run_tests.py:106: failed it tries to rerun it. If run is too long, then it get stopped. nits: is too long -> takes too long get stopped -> is stopped https://codereview.chromium.org/1089383002/diff/20001/components/test/data/pa... components/test/data/password_manager/automated_tests/run_tests.py:112: result = ["", "", False, ""] Should this be a quadruple (,,,) instead of a list [,,,]? https://codereview.chromium.org/1089383002/diff/20001/components/test/data/pa... components/test/data/password_manager/automated_tests/run_tests.py:119: with stopit.ThreadingTimeout(100) as timeout_status: Just to confirm that I understand how with with the ThreadingTimeout works: At the end of the with-block, Python calls .cancel() on timeout_status, so that it won't terminate the program after the with-block. But the reference to timeout_status is still valid even after the with-block, so that we can safely query the state. Is that correct? https://codereview.chromium.org/1089383002/diff/20001/components/test/data/pa... components/test/data/password_manager/automated_tests/run_tests.py:119: with stopit.ThreadingTimeout(100) as timeout_status: nit: What about calling timeout_status just timeout? The object is more than just the state, and we need to use .state to access the state anyway. https://codereview.chromium.org/1089383002/diff/20001/components/test/data/pa... components/test/data/password_manager/automated_tests/run_tests.py:129: result = [[website, test_case, False, "Timeout"]] Again, are results quadruples, or lists of length 4? (I do mean it as a question, not a rhetorical one :), if there is a good reason to use lists, please tell me.) https://codereview.chromium.org/1089383002/diff/20001/components/test/data/pa... components/test/data/password_manager/automated_tests/run_tests.py:129: result = [[website, test_case, False, "Timeout"]] Also, why is result now a list of results (cf. line 112)? https://codereview.chromium.org/1089383002/diff/20001/components/test/data/pa... File components/test/data/password_manager/automated_tests/tests.py (right): https://codereview.chromium.org/1089383002/diff/20001/components/test/data/pa... components/test/data/password_manager/automated_tests/tests.py:512: On 2015/04/16 08:48:00, melandory wrote: > This is intentional: there are two empty lines between functions across this > file. Acknowledged. https://codereview.chromium.org/1089383002/diff/20001/components/test/data/pa... components/test/data/password_manager/automated_tests/tests.py:588: test_cases_to_run = args.test_cases_to_run or TEST_CASES nit: Could you please add a TODO about validating the supplied test case names? I know you had plans for using the decorators, that's likely worth pointing out in the TODO.
https://codereview.chromium.org/1089383002/diff/20001/components/test/data/pa... File components/test/data/password_manager/automated_tests/run_tests.py (right): https://codereview.chromium.org/1089383002/diff/20001/components/test/data/pa... components/test/data/password_manager/automated_tests/run_tests.py:82: def ProcessRun(config, results): On 2015/04/16 10:48:15, vabr (Chromium) wrote: > nit: Although "Process" is used as a verb in the name, it can be misunderstood > as a noun (and Run as a verb instead of a noun), in which case the name sounds > like running a process. > Maybe rename to LogResultsOfTestRun? Done. https://codereview.chromium.org/1089383002/diff/20001/components/test/data/pa... components/test/data/password_manager/automated_tests/run_tests.py:82: def ProcessRun(config, results): On 2015/04/16 10:48:15, vabr (Chromium) wrote: > nit: Although "Process" is used as a verb in the name, it can be misunderstood > as a noun (and Run as a verb instead of a noun), in which case the name sounds > like running a process. > Maybe rename to LogResultsOfTestRun? Done. https://codereview.chromium.org/1089383002/diff/20001/components/test/data/pa... components/test/data/password_manager/automated_tests/run_tests.py:91: website, {True: "passed", False: "failed"}[success]) On 2015/04/16 10:48:16, vabr (Chromium) wrote: > I like this use of a dictionary! :) I also like this small nice things you can do in python. https://codereview.chromium.org/1089383002/diff/20001/components/test/data/pa... components/test/data/password_manager/automated_tests/run_tests.py:99: logger.info("%d failed tests out of %d, failing tests: %s", On 2015/04/16 10:48:15, vabr (Chromium) wrote: > nit: Please replace "tests" (both occurrences) with "test cases". Done. https://codereview.chromium.org/1089383002/diff/20001/components/test/data/pa... components/test/data/password_manager/automated_tests/run_tests.py:101: [name for name in failed_tests]) On 2015/04/16 10:48:15, vabr (Chromium) wrote: > When you are at it, could you also sort the lost of failing test cases? That > will make it easy to identify changes in the list of failures between two test > runs. Done. https://codereview.chromium.org/1089383002/diff/20001/components/test/data/pa... components/test/data/password_manager/automated_tests/run_tests.py:104: def RunTestCaseOnWebsite((website, test_case, config)): On 2015/04/16 10:48:16, vabr (Chromium) wrote: > This has to take a one argument, which is a triple, as opposed to 3 arguments, > because of line 162, yes? Exactly. https://codereview.chromium.org/1089383002/diff/20001/components/test/data/pa... components/test/data/password_manager/automated_tests/run_tests.py:105: """ Runs a |test_case| on a |website|. If case when |test_case| has On 2015/04/16 10:48:16, vabr (Chromium) wrote: > typo: If -> In Done. https://codereview.chromium.org/1089383002/diff/20001/components/test/data/pa... components/test/data/password_manager/automated_tests/run_tests.py:106: failed it tries to rerun it. If run is too long, then it get stopped. On 2015/04/16 10:48:16, vabr (Chromium) wrote: > nits: > is too long -> takes too long > get stopped -> is stopped Done. https://codereview.chromium.org/1089383002/diff/20001/components/test/data/pa... components/test/data/password_manager/automated_tests/run_tests.py:112: result = ["", "", False, ""] On 2015/04/16 10:48:15, vabr (Chromium) wrote: > Should this be a quadruple (,,,) instead of a list [,,,]? yep. https://codereview.chromium.org/1089383002/diff/20001/components/test/data/pa... components/test/data/password_manager/automated_tests/run_tests.py:119: with stopit.ThreadingTimeout(100) as timeout_status: On 2015/04/16 10:48:16, vabr (Chromium) wrote: > nit: What about calling timeout_status just timeout? The object is more than > just the state, and we need to use .state to access the state anyway. Done. https://codereview.chromium.org/1089383002/diff/20001/components/test/data/pa... components/test/data/password_manager/automated_tests/run_tests.py:119: with stopit.ThreadingTimeout(100) as timeout_status: On 2015/04/16 10:48:16, vabr (Chromium) wrote: > Just to confirm that I understand how with with the ThreadingTimeout works: > At the end of the with-block, Python calls .cancel() on timeout_status, so that > it won't terminate the program after the with-block. But the reference to > timeout_status is still valid even after the with-block, so that we can safely > query the state. Is that correct? Yep. https://codereview.chromium.org/1089383002/diff/20001/components/test/data/pa... components/test/data/password_manager/automated_tests/run_tests.py:129: result = [[website, test_case, False, "Timeout"]] On 2015/04/16 10:48:15, vabr (Chromium) wrote: > Also, why is result now a list of results (cf. line 112)? Done. https://codereview.chromium.org/1089383002/diff/20001/components/test/data/pa... components/test/data/password_manager/automated_tests/run_tests.py:129: result = [[website, test_case, False, "Timeout"]] On 2015/04/16 10:48:16, vabr (Chromium) wrote: > Again, are results quadruples, or lists of length 4? > (I do mean it as a question, not a rhetorical one :), if there is a good reason > to use lists, please tell me.) Nope, no good reason. You're completely correct that I should change it. https://codereview.chromium.org/1089383002/diff/20001/components/test/data/pa... File components/test/data/password_manager/automated_tests/tests.py (right): https://codereview.chromium.org/1089383002/diff/20001/components/test/data/pa... components/test/data/password_manager/automated_tests/tests.py:588: test_cases_to_run = args.test_cases_to_run or TEST_CASES On 2015/04/16 10:48:16, vabr (Chromium) wrote: > nit: Could you please add a TODO about validating the supplied test case names? > I know you had plans for using the decorators, that's likely worth pointing out > in the TODO. TODO is in this CL: https://codereview.chromium.org/1084553003/diff/40001/components/test/data/pa... Current CL depends on it, so I'll land them one after another. Sorry for not mentioning it.
Thank you, Tanja! This LGTM, with one nit and one more comment. Cheers, Vaclav https://codereview.chromium.org/1089383002/diff/20001/components/test/data/pa... File components/test/data/password_manager/automated_tests/tests.py (right): https://codereview.chromium.org/1089383002/diff/20001/components/test/data/pa... components/test/data/password_manager/automated_tests/tests.py:588: test_cases_to_run = args.test_cases_to_run or TEST_CASES On 2015/04/16 14:38:29, melandory wrote: > On 2015/04/16 10:48:16, vabr (Chromium) wrote: > > nit: Could you please add a TODO about validating the supplied test case > names? > > I know you had plans for using the decorators, that's likely worth pointing > out > > in the TODO. > TODO is in this CL: > https://codereview.chromium.org/1084553003/diff/40001/components/test/data/pa... > > Current CL depends on it, so I'll land them one after another. Sorry for not > mentioning it. Acknowledged. I should have remembered that, thanks for reminding me. https://codereview.chromium.org/1089383002/diff/40001/components/test/data/pa... File components/test/data/password_manager/automated_tests/run_tests.py (right): https://codereview.chromium.org/1089383002/diff/40001/components/test/data/pa... components/test/data/password_manager/automated_tests/run_tests.py:101: sorted([name for name in failed_tests])) nit: indentation seems off https://codereview.chromium.org/1089383002/diff/40001/components/test/data/pa... File components/test/data/password_manager/automated_tests/tests.py (right): https://codereview.chromium.org/1089383002/diff/40001/components/test/data/pa... components/test/data/password_manager/automated_tests/tests.py:545: return environment.tests_results Now that only one test case is run, we should return a single result, not a list. Ultimately, Environment should be changed to expect that it is created for a single test case solely. In particular, environment.tests_results should also not be a list and should not have the name in plural. But that is out of scope of this CL. Adding that as a TODO(vabr) in Environment would be helpful, though.
https://codereview.chromium.org/1089383002/diff/40001/components/test/data/pa... File components/test/data/password_manager/automated_tests/run_tests.py (right): https://codereview.chromium.org/1089383002/diff/40001/components/test/data/pa... components/test/data/password_manager/automated_tests/run_tests.py:101: sorted([name for name in failed_tests])) On 2015/04/16 14:55:43, vabr (Chromium) wrote: > nit: indentation seems off Done. https://codereview.chromium.org/1089383002/diff/40001/components/test/data/pa... File components/test/data/password_manager/automated_tests/tests.py (right): https://codereview.chromium.org/1089383002/diff/40001/components/test/data/pa... components/test/data/password_manager/automated_tests/tests.py:545: return environment.tests_results Some after thoughts: > Now that only one test case is run, we should return a single result, not a list. I don't like idea of making this change now, because one can use Environment to run several test cases. And making this change only because we know that it's not used this way doesn't sounds right. WDYT? >Adding that as a TODO(vabr) in Environment would be helpful, though. There is a similar TODO in Environment.AddWebsiteTest.
LGTM! Cheers, Vaclav https://codereview.chromium.org/1089383002/diff/40001/components/test/data/pa... File components/test/data/password_manager/automated_tests/tests.py (right): https://codereview.chromium.org/1089383002/diff/40001/components/test/data/pa... components/test/data/password_manager/automated_tests/tests.py:545: return environment.tests_results On 2015/04/17 08:11:30, melandory wrote: > > Some after thoughts: > > Now that only one test case is run, we should return a single result, not a > list. > I don't like idea of making this change now, because one can use Environment to > run several test cases. And making this change only because we know that it's > not used this way doesn't sounds right. WDYT? Leaving it for later sounds fine. The plan is to simplify all the classes, and since the way we use them has changed it will very likely lead to such API changes as proposed. But I agree that separating that into different CLs will be easier. > > >Adding that as a TODO(vabr) in Environment would be helpful, though. > There is a similar TODO in Environment.AddWebsiteTest. Fair enough, sounds good.
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1089383002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by melandory@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org Link to the patchset: https://codereview.chromium.org/1089383002/#ps80001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1089383002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/a12e1e10c4a910a4f8aa20e3ea8b611846f19ed9 Cr-Commit-Position: refs/heads/master@{#326495} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
