Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
903763003:
Refactoring and paralelizing Python password manager tests. (Closed)
DescriptionRefactoring and parralelizing Python password manager tests.
1.Parallelizing run of tests
2.All options were moved from command line args to config
3.Introducing possibility to avoid writing to sheet
BUG=435249
Committed: https://crrev.com/e30fc9be0bc9fbcccdfda1e4beb16919cd5bd7cc
Cr-Commit-Position: refs/heads/master@{#315053}
Patch Set 1 #
Total comments: 48
Patch Set 2 : Adress reviewers comments #Patch Set 3 : Small fixes #
Total comments: 2
Patch Set 4 : Fix comment #
Messages
Total messages: 15 (4 generated)
dvadym@chromium.org changed reviewers: + melandory@chromium.org, vabr@chromium.org
Hi Vaclav and Tanja, Could you please review this CL? Best regards, Vadym
Hi Vadym, Some initial comments. If you did not do so yet, please run the pylint (gpylint) on your changes. Cheers, Vaclav https://codereview.chromium.org/903763003/diff/1/components/test/data/passwor... File components/test/data/password_manager/automated_tests/run_tests.py (right): https://codereview.chromium.org/903763003/diff/1/components/test/data/passwor... components/test/data/password_manager/automated_tests/run_tests.py:17: tests_in_parrallel=<number of parrallel tests> parrallel -> parallel https://codereview.chromium.org/903763003/diff/1/components/test/data/passwor... components/test/data/password_manager/automated_tests/run_tests.py:58: if sheet_libraries_import_error != None: nit: You could drop the != None https://codereview.chromium.org/903763003/diff/1/components/test/data/passwor... components/test/data/password_manager/automated_tests/run_tests.py:96: pass # TODO(melandory): Sometimes writing to spreadsheet fails. We need grammar: need -> need to https://codereview.chromium.org/903763003/diff/1/components/test/data/passwor... components/test/data/password_manager/automated_tests/run_tests.py:97: # deal with it better that just ignoring. that -> than ignoring -> by ignoring it https://codereview.chromium.org/903763003/diff/1/components/test/data/passwor... components/test/data/password_manager/automated_tests/run_tests.py:105: print>>sys.stdout, "Test " + test_name + " starting" I'm not sure about the spacing. Did you run gpylint? https://codereview.chromium.org/903763003/diff/1/components/test/data/passwor... components/test/data/password_manager/automated_tests/run_tests.py:105: print>>sys.stdout, "Test " + test_name + " starting" Also, do you need to specify sys.stdout? https://codereview.chromium.org/903763003/diff/1/components/test/data/passwor... components/test/data/password_manager/automated_tests/run_tests.py:108: dir=os.path.join(tempfile.gettempdir()), delete=False) What is the effect of calling path.join on a single argument? https://codereview.chromium.org/903763003/diff/1/components/test/data/passwor... components/test/data/password_manager/automated_tests/run_tests.py:113: self.test_cmd[2] = self.test_name = test_name Are you relying on test_cmd to have a special form? You should at least document it. https://codereview.chromium.org/903763003/diff/1/components/test/data/passwor... components/test/data/password_manager/automated_tests/run_tests.py:140: self._run_test() no return? https://codereview.chromium.org/903763003/diff/1/components/test/data/passwor... components/test/data/password_manager/automated_tests/run_tests.py:166: os.removedirs(self.profile_path) This is going to try to rmdir all parts of profile_path. It will also fail to delete anything if profile_path contains a file. Did you mean to use shutil.rmtree? https://codereview.chromium.org/903763003/diff/1/components/test/data/passwor... components/test/data/password_manager/automated_tests/run_tests.py:198: while len(runners) + len(tests_to_run) > 0: Isn't runners still empty here?
Hi Vadym, here is my observations. Some of them belong not to your code, so I think it's fine to put them as TODOs. Thanks for sending me CL. Thins way it'll be much easer to me to return to this code later. https://codereview.chromium.org/903763003/diff/1/components/test/data/passwor... File components/test/data/password_manager/automated_tests/environment.py (right): https://codereview.chromium.org/903763003/diff/1/components/test/data/passwor... components/test/data/password_manager/automated_tests/environment.py:90: pass I disagree with removing of this log message: it's needed to catch wrong setup of profile folder. And i think I know why you are removing it, it's because you get this exception every time you run tests. It's happening because of line 175 run_tests.py environment = Environment('', '', '', None, False) this dummy environment serves only one purpose -- get tests names (line 190). I think it's better to tackle the problem that we need to know test name before creating real environment, instead of removing consequence. https://codereview.chromium.org/903763003/diff/1/components/test/data/passwor... File components/test/data/password_manager/automated_tests/run_tests.py (right): https://codereview.chromium.org/903763003/diff/1/components/test/data/passwor... components/test/data/password_manager/automated_tests/run_tests.py:13: chrome-path=<chrome binary path> Along with changes in run_tests you/I/someone should also change runner script from internal repo. https://codereview.chromium.org/903763003/diff/1/components/test/data/passwor... components/test/data/password_manager/automated_tests/run_tests.py:53: class SheetWriter: Maybe put it to separate file? https://codereview.chromium.org/903763003/diff/1/components/test/data/passwor... components/test/data/password_manager/automated_tests/run_tests.py:135: print>>sys.stdout, "Test " + self.test_name + " passed" I would use logging instead of print, especially taking into account fact that we're running this script from other python module (and potentially we'll import it there and not just use subprocess), so having something writing to stdout doesn't looks good to me, on the other hand with logging we can propagate this messages. https://codereview.chromium.org/903763003/diff/1/components/test/data/passwor... components/test/data/password_manager/automated_tests/run_tests.py:163: except Exception: Why don't you want to put line 162 and 166 in same try/except block? https://codereview.chromium.org/903763003/diff/1/components/test/data/passwor... components/test/data/password_manager/automated_tests/run_tests.py:192: if config.has_option("run_options", "tests_to_run"): I would do: if config.has_option("run_options", "tests_to_run"): #... else: #line 190 https://codereview.chromium.org/903763003/diff/1/components/test/data/passwor... components/test/data/password_manager/automated_tests/run_tests.py:202: if result != None: # This test run is finished. Can't you do just "if result" here? https://codereview.chromium.org/903763003/diff/1/components/test/data/passwor... components/test/data/password_manager/automated_tests/run_tests.py:213: savefile.close() Consider using "with".
Thanks Vaclav and Tanja, PTAL https://codereview.chromium.org/903763003/diff/1/components/test/data/passwor... File components/test/data/password_manager/automated_tests/environment.py (right): https://codereview.chromium.org/903763003/diff/1/components/test/data/passwor... components/test/data/password_manager/automated_tests/environment.py:90: pass On 2015/02/06 08:37:46, melandory wrote: > I disagree with removing of this log message: it's needed to catch wrong setup > of profile folder. > > And i think I know why you are removing it, it's because you get this exception > every time you run tests. It's happening because of line 175 run_tests.py > environment = Environment('', '', '', None, False) > this dummy environment serves only one purpose -- get tests names (line 190). I > think it's better to tackle the problem that we need to know test name before > creating real environment, instead of removing consequence. Now each test run has own temporary profile directory. So this is not a problem anymore. And yeah, it failed each time, since it can't remove non-existing temporary folder :). https://codereview.chromium.org/903763003/diff/1/components/test/data/passwor... File components/test/data/password_manager/automated_tests/run_tests.py (right): https://codereview.chromium.org/903763003/diff/1/components/test/data/passwor... components/test/data/password_manager/automated_tests/run_tests.py:13: chrome-path=<chrome binary path> On 2015/02/06 08:37:47, melandory wrote: > Along with changes in run_tests you/I/someone should also change runner script > from internal repo. Thanks, I'll do. https://codereview.chromium.org/903763003/diff/1/components/test/data/passwor... components/test/data/password_manager/automated_tests/run_tests.py:17: tests_in_parrallel=<number of parrallel tests> On 2015/02/05 17:49:58, vabr (Chromium) wrote: > parrallel -> parallel Done. https://codereview.chromium.org/903763003/diff/1/components/test/data/passwor... components/test/data/password_manager/automated_tests/run_tests.py:53: class SheetWriter: On 2015/02/06 08:37:47, melandory wrote: > Maybe put it to separate file? It seems that run_tests.py is not so big, having many files especially when you work in vi is painful. Vaclav, what do you think? https://codereview.chromium.org/903763003/diff/1/components/test/data/passwor... components/test/data/password_manager/automated_tests/run_tests.py:58: if sheet_libraries_import_error != None: On 2015/02/05 17:49:57, vabr (Chromium) wrote: > nit: You could drop the != None Done https://codereview.chromium.org/903763003/diff/1/components/test/data/passwor... components/test/data/password_manager/automated_tests/run_tests.py:96: pass # TODO(melandory): Sometimes writing to spreadsheet fails. We need On 2015/02/05 17:49:57, vabr (Chromium) wrote: > grammar: need -> need to Done. https://codereview.chromium.org/903763003/diff/1/components/test/data/passwor... components/test/data/password_manager/automated_tests/run_tests.py:97: # deal with it better that just ignoring. On 2015/02/05 17:49:57, vabr (Chromium) wrote: > that -> than > ignoring -> by ignoring it Done. https://codereview.chromium.org/903763003/diff/1/components/test/data/passwor... components/test/data/password_manager/automated_tests/run_tests.py:105: print>>sys.stdout, "Test " + test_name + " starting" On 2015/02/05 17:49:57, vabr (Chromium) wrote: > Also, do you need to specify sys.stdout? Ah, yeah, thanks, at first I used sys.stderr, but then decide to use stdout.Removed. https://codereview.chromium.org/903763003/diff/1/components/test/data/passwor... components/test/data/password_manager/automated_tests/run_tests.py:105: print>>sys.stdout, "Test " + test_name + " starting" On 2015/02/05 17:49:57, vabr (Chromium) wrote: > I'm not sure about the spacing. Did you run gpylint? I didn't know about it, I run and fix. https://codereview.chromium.org/903763003/diff/1/components/test/data/passwor... components/test/data/password_manager/automated_tests/run_tests.py:108: dir=os.path.join(tempfile.gettempdir()), delete=False) On 2015/02/05 17:49:58, vabr (Chromium) wrote: > What is the effect of calling path.join on a single argument? It makes, sense, I removed tempfile.gettempdir()) entirely. https://codereview.chromium.org/903763003/diff/1/components/test/data/passwor... components/test/data/password_manager/automated_tests/run_tests.py:113: self.test_cmd[2] = self.test_name = test_name On 2015/02/05 17:49:57, vabr (Chromium) wrote: > Are you relying on test_cmd to have a special form? You should at least document > it. Done. I've added comment in description of general_test_cmd in header: "[2] is placeholder for test name." https://codereview.chromium.org/903763003/diff/1/components/test/data/passwor... components/test/data/password_manager/automated_tests/run_tests.py:135: print>>sys.stdout, "Test " + self.test_name + " passed" On 2015/02/06 08:37:46, melandory wrote: > I would use logging instead of print, especially taking into account fact that > we're running this script from other python module (and potentially we'll import > it there and not just use subprocess), so having something writing to stdout > doesn't looks good to me, on the other hand with logging we can propagate this > messages. This is just matter of convenience for case when you run it locally. You can see progress. All information that we need for final result is written to logs. If we need to write something more to log, I think this is another question. https://codereview.chromium.org/903763003/diff/1/components/test/data/passwor... components/test/data/password_manager/automated_tests/run_tests.py:140: self._run_test() On 2015/02/05 17:49:58, vabr (Chromium) wrote: > no return? In python no returns means return None, exactly as I need. I didn't find anything in style guide about it. https://codereview.chromium.org/903763003/diff/1/components/test/data/passwor... components/test/data/password_manager/automated_tests/run_tests.py:163: except Exception: On 2015/02/06 08:37:47, melandory wrote: > Why don't you want to put line 162 and 166 in same try/except block? If the first throws exception, then the second will not run. https://codereview.chromium.org/903763003/diff/1/components/test/data/passwor... components/test/data/password_manager/automated_tests/run_tests.py:166: os.removedirs(self.profile_path) On 2015/02/05 17:49:57, vabr (Chromium) wrote: > This is going to try to rmdir all parts of profile_path. It will also fail to > delete anything if profile_path contains a file. > Did you mean to use shutil.rmtree? Or great, thanks, it has been here for ages, I didn't notice it. https://codereview.chromium.org/903763003/diff/1/components/test/data/passwor... components/test/data/password_manager/automated_tests/run_tests.py:192: if config.has_option("run_options", "tests_to_run"): On 2015/02/06 08:37:47, melandory wrote: > I would do: > if config.has_option("run_options", "tests_to_run"): > #... > else: > #line 190 I want to try to run only those tests that are currently present in tests.py. In order to do this in #line 194 is made of intersection of all tests that we have and those tests that user selected. Also in lines #193 and #194 we can add validation that tests that user selected are available. Vaclav and Tanja, what do you think about validation? https://codereview.chromium.org/903763003/diff/1/components/test/data/passwor... components/test/data/password_manager/automated_tests/run_tests.py:198: while len(runners) + len(tests_to_run) > 0: On 2015/02/05 17:49:58, vabr (Chromium) wrote: > Isn't runners still empty here? Before first iteration yes, but then they are added on line 213. Basically this condition means run until all tasks are finished (runners is empty) and tests queue is empty (tests_to_run empty). https://codereview.chromium.org/903763003/diff/1/components/test/data/passwor... components/test/data/password_manager/automated_tests/run_tests.py:202: if result != None: # This test run is finished. On 2015/02/06 08:37:46, melandory wrote: > Can't you do just "if result" here? Done.
LGTM with comments. Thanks! Vaclav https://codereview.chromium.org/903763003/diff/1/components/test/data/passwor... File components/test/data/password_manager/automated_tests/run_tests.py (right): https://codereview.chromium.org/903763003/diff/1/components/test/data/passwor... components/test/data/password_manager/automated_tests/run_tests.py:53: class SheetWriter: On 2015/02/06 11:02:27, dvadym wrote: > On 2015/02/06 08:37:47, melandory wrote: > > Maybe put it to separate file? > > It seems that run_tests.py is not so big, having many files especially when you > work in vi is painful. Vaclav, what do you think? I don't support the thesis, that multiple files are painful in vi. :) Separate code by function into small single-focussed files is a good practice, and should happen here. I'm fine with having this as a TODO, because this CL's focus is the parallel execution, not general refactoring. https://codereview.chromium.org/903763003/diff/1/components/test/data/passwor... components/test/data/password_manager/automated_tests/run_tests.py:135: print>>sys.stdout, "Test " + self.test_name + " passed" On 2015/02/06 11:02:26, dvadym wrote: > On 2015/02/06 08:37:46, melandory wrote: > > I would use logging instead of print, especially taking into account fact that > > we're running this script from other python module (and potentially we'll > import > > it there and not just use subprocess), so having something writing to stdout > > doesn't looks good to me, on the other hand with logging we can propagate this > > messages. > > This is just matter of convenience for case when you run it locally. You can see > progress. All information that we need for final result is written to logs. If > we need to write something more to log, I think this is another question. In general, this kind of print statements should be really done through some logging framework. It should not go by default to stdout, and should be activated by some "verbose" command line flag only. Running these scripts on cron, which can send alert e-mails if the scripts output anything, is the main use-case, so we should optimise for that. Again, it is OK to put somewhere a TODO to remove the ad-hoc print statements, because this CL is focussed on the parallel running. https://codereview.chromium.org/903763003/diff/1/components/test/data/passwor... components/test/data/password_manager/automated_tests/run_tests.py:140: self._run_test() On 2015/02/06 11:02:27, dvadym wrote: > On 2015/02/05 17:49:58, vabr (Chromium) wrote: > > no return? > > In python no returns means return None, exactly as I need. I didn't find > anything in style guide about it. I don't like mixing the implicit return and the explicit one (see line 133). Putting there the explicit return None would make the code clearer for inexperienced pythoners like me. :) What are the benefits of keeping the last return implicit? https://codereview.chromium.org/903763003/diff/1/components/test/data/passwor... components/test/data/password_manager/automated_tests/run_tests.py:192: if config.has_option("run_options", "tests_to_run"): On 2015/02/06 11:02:27, dvadym wrote: > On 2015/02/06 08:37:47, melandory wrote: > > I would do: > > if config.has_option("run_options", "tests_to_run"): > > #... > > else: > > #line 190 > > I want to try to run only those tests that are currently present in tests.py. In > order to do this in #line 194 is made of intersection of all tests that we have > and those tests that user selected. Also in lines #193 and #194 we can add > validation that tests that user selected are available. Vaclav and Tanja, what > do you think about validation? Validation seems helpful for diagnosing problems, but out of scope of this CL. Please leave TODO for that. https://codereview.chromium.org/903763003/diff/1/components/test/data/passwor... components/test/data/password_manager/automated_tests/run_tests.py:198: while len(runners) + len(tests_to_run) > 0: On 2015/02/06 11:02:26, dvadym wrote: > On 2015/02/05 17:49:58, vabr (Chromium) wrote: > > Isn't runners still empty here? > > Before first iteration yes, but then they are added on line 213. Basically this > condition means run until all tasks are finished (runners is empty) and tests > queue is empty (tests_to_run empty). Sorry, my bad. At the point when I read the end of the loop I forgot to revise my comment. :)
New patchsets have been uploaded after l-g-t-m from vabr@chromium.org
Thanks Vaclav, fixed. https://codereview.chromium.org/903763003/diff/1/components/test/data/passwor... File components/test/data/password_manager/automated_tests/run_tests.py (right): https://codereview.chromium.org/903763003/diff/1/components/test/data/passwor... components/test/data/password_manager/automated_tests/run_tests.py:53: class SheetWriter: On 2015/02/06 14:32:52, vabr (Chromium) wrote: > On 2015/02/06 11:02:27, dvadym wrote: > > On 2015/02/06 08:37:47, melandory wrote: > > > Maybe put it to separate file? > > > > It seems that run_tests.py is not so big, having many files especially when > you > > work in vi is painful. Vaclav, what do you think? > > I don't support the thesis, that multiple files are painful in vi. :) > > Separate code by function into small single-focussed files is a good practice, > and should happen here. > I'm fine with having this as a TODO, because this CL's focus is the parallel > execution, not general refactoring. Ok, added Todo https://codereview.chromium.org/903763003/diff/1/components/test/data/passwor... components/test/data/password_manager/automated_tests/run_tests.py:135: print>>sys.stdout, "Test " + self.test_name + " passed" On 2015/02/06 14:32:52, vabr (Chromium) wrote: > On 2015/02/06 11:02:26, dvadym wrote: > > On 2015/02/06 08:37:46, melandory wrote: > > > I would use logging instead of print, especially taking into account fact > that > > > we're running this script from other python module (and potentially we'll > > import > > > it there and not just use subprocess), so having something writing to stdout > > > doesn't looks good to me, on the other hand with logging we can propagate > this > > > messages. > > > > This is just matter of convenience for case when you run it locally. You can > see > > progress. All information that we need for final result is written to logs. If > > we need to write something more to log, I think this is another question. > > In general, this kind of print statements should be really done through some > logging framework. It should not go by default to stdout, and should be > activated by some "verbose" command line flag only. Running these scripts on > cron, which can send alert e-mails if the scripts output anything, is the main > use-case, so we should optimise for that. > > Again, it is OK to put somewhere a TODO to remove the ad-hoc print statements, > because this CL is focussed on the parallel running. Ok, todo added https://codereview.chromium.org/903763003/diff/1/components/test/data/passwor... components/test/data/password_manager/automated_tests/run_tests.py:140: self._run_test() On 2015/02/06 14:32:52, vabr (Chromium) wrote: > On 2015/02/06 11:02:27, dvadym wrote: > > On 2015/02/05 17:49:58, vabr (Chromium) wrote: > > > no return? > > > > In python no returns means return None, exactly as I need. I didn't find > > anything in style guide about it. > > I don't like mixing the implicit return and the explicit one (see line 133). > Putting there the explicit return None would make the code clearer for > inexperienced pythoners like me. :) > > What are the benefits of keeping the last return implicit? Done. https://codereview.chromium.org/903763003/diff/1/components/test/data/passwor... components/test/data/password_manager/automated_tests/run_tests.py:192: if config.has_option("run_options", "tests_to_run"): On 2015/02/06 14:32:52, vabr (Chromium) wrote: > On 2015/02/06 11:02:27, dvadym wrote: > > On 2015/02/06 08:37:47, melandory wrote: > > > I would do: > > > if config.has_option("run_options", "tests_to_run"): > > > #... > > > else: > > > #line 190 > > > > I want to try to run only those tests that are currently present in tests.py. > In > > order to do this in #line 194 is made of intersection of all tests that we > have > > and those tests that user selected. Also in lines #193 and #194 we can add > > validation that tests that user selected are available. Vaclav and Tanja, what > > do you think about validation? > > Validation seems helpful for diagnosing problems, but out of scope of this CL. > Please leave TODO for that. Done. https://codereview.chromium.org/903763003/diff/1/components/test/data/passwor... components/test/data/password_manager/automated_tests/run_tests.py:198: while len(runners) + len(tests_to_run) > 0: On 2015/02/06 14:32:52, vabr (Chromium) wrote: > On 2015/02/06 11:02:26, dvadym wrote: > > On 2015/02/05 17:49:58, vabr (Chromium) wrote: > > > Isn't runners still empty here? > > > > Before first iteration yes, but then they are added on line 213. Basically > this > > condition means run until all tasks are finished (runners is empty) and tests > > queue is empty (tests_to_run empty). > > Sorry, my bad. At the point when I read the end of the loop I forgot to revise > my comment. :) No problem :) https://codereview.chromium.org/903763003/diff/1/components/test/data/passwor... components/test/data/password_manager/automated_tests/run_tests.py:213: savefile.close() On 2015/02/06 08:37:47, melandory wrote: > Consider using "with". Done.
Still LGTM. Vaclav https://codereview.chromium.org/903763003/diff/40001/components/test/data/pas... File components/test/data/password_manager/automated_tests/run_tests.py (right): https://codereview.chromium.org/903763003/diff/40001/components/test/data/pas... components/test/data/password_manager/automated_tests/run_tests.py:199: # TODO((dvadym) Add validation that tests that user selected are available. typo: user selected -> user selected tests Alternatively a shorter formulation: TODO(dvadym) Validate that user selected tests are available.
New patchsets have been uploaded after l-g-t-m from vabr@chromium.org
https://codereview.chromium.org/903763003/diff/40001/components/test/data/pas... File components/test/data/password_manager/automated_tests/run_tests.py (right): https://codereview.chromium.org/903763003/diff/40001/components/test/data/pas... components/test/data/password_manager/automated_tests/run_tests.py:199: # TODO((dvadym) Add validation that tests that user selected are available. On 2015/02/06 16:04:03, vabr (Chromium) wrote: > typo: user selected -> user selected tests > Alternatively a shorter formulation: > TODO(dvadym) Validate that user selected tests are available. Done.
The CQ bit was checked by dvadym@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/903763003/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/e30fc9be0bc9fbcccdfda1e4beb16919cd5bd7cc Cr-Commit-Position: refs/heads/master@{#315053} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
