Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2124973003:
These are the database objects used while finding the regression range. 1/3 (Closed)
DescriptionThese are the database objects used while finding the regression range.
BUG=617808
Committed: https://chromium.googlesource.com/infra/infra/+/0e9802728999cc1072423b17f6d6dc4feeaa0e99
Patch Set 1 #
Total comments: 39
Patch Set 2 : Database objects post offline discussion #
Total comments: 16
Patch Set 3 : cleanup - removed files that weren't used. Still have not addressed comments #Patch Set 4 : cleanup again - these four files are the only ones actually used #Patch Set 5 : Addressed comments #
Total comments: 12
Patch Set 6 : addressed chan's comments #
Total comments: 2
Patch Set 7 : removed unused code #
Total comments: 12
Patch Set 8 : addressed jeff's comments #
Total comments: 6
Patch Set 9 : Added tests for models and gpylinted everything #
Total comments: 2
Patch Set 10 : Addressed comments #Patch Set 11 : added init #Messages
Total messages: 25 (6 generated)
caiw@google.com changed reviewers: + chanli@chromium.org, lijeffrey@google.com, stgao@chromium.org
I cut up the provisionally running pipeline into 3 CLs. This one contains the models which are used during the run.
Description was changed from ========== These are the database objects used while finding the regression range. BUG=617808 ========== to ========== These are the database objects used while finding the regression range. BUG=617808 ==========
I just reviewed this CL and because I haven't seen the logic where you are using these data models, I feel a little confused. Please address my current comments first(nits and comments), and I'll take a closer look when I better understand how these models are used. https://codereview.chromium.org/2124973003/diff/1/appengine/findit/model/base... File appengine/findit/model/base_flake_model.py (right): https://codereview.chromium.org/2124973003/diff/1/appengine/findit/model/base... appengine/findit/model/base_flake_model.py:1: # Copyright 2014 The Chromium Authors. All rights reserved. nit: change the year to 2016, same for others https://codereview.chromium.org/2124973003/diff/1/appengine/findit/model/base... appengine/findit/model/base_flake_model.py:11: The computed properties are master name, builder name, and build number. Please change the docstring here: step_name is also used here. https://codereview.chromium.org/2124973003/diff/1/appengine/findit/model/base... appengine/findit/model/base_flake_model.py:15: ndb.Key('KindName', build_id, 'Optional_KindName', optional_id, ...) Please update this part https://codereview.chromium.org/2124973003/diff/1/appengine/findit/model/base... appengine/findit/model/base_flake_model.py:19: def CreateFlakeId(master_name, builder_name, step_name, build_number): I would have the argument list as : master_name, builder_name, build_number, step_name https://codereview.chromium.org/2124973003/diff/1/appengine/findit/model/base... appengine/findit/model/base_flake_model.py:20: return '%s/%s/%s/%s' % (master_name, builder_name, step_name, build_number) Is there a reason why you want to organize the key like this? I would use (master_name, builder_name, build_number, step_name) since first 3 are for build and step_name is for step in the build https://codereview.chromium.org/2124973003/diff/1/appengine/findit/model/base... File appengine/findit/model/base_swarming_task.py (right): https://codereview.chromium.org/2124973003/diff/1/appengine/findit/model/base... appengine/findit/model/base_swarming_task.py:1: # Copyright 2016 The Chromium Authors. All rights reserved. Could you explain to me what this model is and what's the difference/ relationship with wf_swarming_task? https://codereview.chromium.org/2124973003/diff/1/appengine/findit/model/flak... File appengine/findit/model/flake/base_analysis.py (right): https://codereview.chromium.org/2124973003/diff/1/appengine/findit/model/flak... appengine/findit/model/flake/base_analysis.py:1: # Copyright 2014 The Chromium Authors. All rights reserved. 2014 -> 2016 https://codereview.chromium.org/2124973003/diff/1/appengine/findit/model/flak... File appengine/findit/model/flake/flake_swarming_task.py (right): https://codereview.chromium.org/2124973003/diff/1/appengine/findit/model/flak... appengine/findit/model/flake/flake_swarming_task.py:1: # Copyright 2014 The Chromium Authors. All rights reserved. 2014 -> 2016 https://codereview.chromium.org/2124973003/diff/1/appengine/findit/model/flak... appengine/findit/model/flake/flake_swarming_task.py:42: Please add comments to below parameters. https://codereview.chromium.org/2124973003/diff/1/appengine/findit/model/flak... File appengine/findit/model/flake/flake_swarming_task_result.py (right): https://codereview.chromium.org/2124973003/diff/1/appengine/findit/model/flak... appengine/findit/model/flake/flake_swarming_task_result.py:1: # Copyright 2014 The Chromium Authors. All rights reserved. 2014 -> 2016 https://codereview.chromium.org/2124973003/diff/1/appengine/findit/model/flak... appengine/findit/model/flake/flake_swarming_task_result.py:16: step_name, build_number, testname): # pragma: no cover nit: I would use test_name instead of testname https://codereview.chromium.org/2124973003/diff/1/appengine/findit/model/flak... appengine/findit/model/flake/flake_swarming_task_result.py:74: Same here: please add comments. https://codereview.chromium.org/2124973003/diff/1/appengine/findit/model/flak... File appengine/findit/model/flake/master_flake_analysis.py (right): https://codereview.chromium.org/2124973003/diff/1/appengine/findit/model/flak... appengine/findit/model/flake/master_flake_analysis.py:1: # Copyright 2014 The Chromium Authors. All rights reserved. 2014 -> 2016
Per off-line discussion, we will use some base classes to share code as much as possible. https://codereview.chromium.org/2124973003/diff/1/appengine/findit/model/base... File appengine/findit/model/base_flake_model.py (right): https://codereview.chromium.org/2124973003/diff/1/appengine/findit/model/base... appengine/findit/model/base_flake_model.py:23: def master_name(self): Per discussion, we will use the BaseBuildModel directly. https://codereview.chromium.org/2124973003/diff/1/appengine/findit/model/flak... File appengine/findit/model/flake/base_analysis.py (right): https://codereview.chromium.org/2124973003/diff/1/appengine/findit/model/flak... appengine/findit/model/flake/base_analysis.py:44: def correct(self): This seems not belonging to this base class. https://codereview.chromium.org/2124973003/diff/1/appengine/findit/model/flak... appengine/findit/model/flake/base_analysis.py:78: def failure_type(self): This seems not fit here. https://codereview.chromium.org/2124973003/diff/1/appengine/findit/model/flak... appengine/findit/model/flake/base_analysis.py:99: def failure_type_str(self): Same here. https://codereview.chromium.org/2124973003/diff/1/appengine/findit/model/flak... File appengine/findit/model/flake/flake_swarming_task_result.py (right): https://codereview.chromium.org/2124973003/diff/1/appengine/findit/model/flak... appengine/findit/model/flake/flake_swarming_task_result.py:56: return self.key.pairs()[0][1].split('/')[0] As we discussed, some code here could be cleaned up a bit after using some BaseBuildModel class. https://codereview.chromium.org/2124973003/diff/1/appengine/findit/model/flak... File appengine/findit/model/flake/master_flake_analysis.py (right): https://codereview.chromium.org/2124973003/diff/1/appengine/findit/model/flak... appengine/findit/model/flake/master_flake_analysis.py:21: def master_name(self): Same here for using BaseBuildModel class. https://codereview.chromium.org/2124973003/diff/1/appengine/findit/model/flak... appengine/findit/model/flake/master_flake_analysis.py:70: success_rates = ndb.FloatProperty(indexed=False, repeated=True) Is the success rate per-test? If so, should we make test_name as part of the key?
https://codereview.chromium.org/2124973003/diff/1/appengine/findit/model/base... File appengine/findit/model/base_flake_model.py (right): https://codereview.chromium.org/2124973003/diff/1/appengine/findit/model/base... appengine/findit/model/base_flake_model.py:1: # Copyright 2014 The Chromium Authors. All rights reserved. On 2016/07/07 23:38:27, chanli wrote: > nit: change the year to 2016, same for others Done. https://codereview.chromium.org/2124973003/diff/1/appengine/findit/model/base... appengine/findit/model/base_flake_model.py:15: ndb.Key('KindName', build_id, 'Optional_KindName', optional_id, ...) On 2016/07/07 23:38:27, chanli wrote: > Please update this part Done. https://codereview.chromium.org/2124973003/diff/1/appengine/findit/model/base... appengine/findit/model/base_flake_model.py:19: def CreateFlakeId(master_name, builder_name, step_name, build_number): On 2016/07/07 23:38:28, chanli wrote: > I would have the argument list as : > master_name, builder_name, build_number, step_name Done. https://codereview.chromium.org/2124973003/diff/1/appengine/findit/model/base... appengine/findit/model/base_flake_model.py:20: return '%s/%s/%s/%s' % (master_name, builder_name, step_name, build_number) On 2016/07/07 23:38:28, chanli wrote: > Is there a reason why you want to organize the key like this? I would use > (master_name, builder_name, build_number, step_name) since first 3 are for build > and step_name is for step in the build Done. https://codereview.chromium.org/2124973003/diff/1/appengine/findit/model/base... appengine/findit/model/base_flake_model.py:23: def master_name(self): On 2016/07/08 22:51:26, stgao wrote: > Per discussion, we will use the BaseBuildModel directly. Done. https://codereview.chromium.org/2124973003/diff/1/appengine/findit/model/base... File appengine/findit/model/base_swarming_task.py (right): https://codereview.chromium.org/2124973003/diff/1/appengine/findit/model/base... appengine/findit/model/base_swarming_task.py:1: # Copyright 2016 The Chromium Authors. All rights reserved. On 2016/07/07 23:38:28, chanli wrote: > Could you explain to me what this model is and what's the difference/ > relationship with wf_swarming_task? Done. https://codereview.chromium.org/2124973003/diff/1/appengine/findit/model/flak... File appengine/findit/model/flake/base_analysis.py (right): https://codereview.chromium.org/2124973003/diff/1/appengine/findit/model/flak... appengine/findit/model/flake/base_analysis.py:1: # Copyright 2014 The Chromium Authors. All rights reserved. On 2016/07/07 23:38:28, chanli wrote: > 2014 -> 2016 Done. https://codereview.chromium.org/2124973003/diff/1/appengine/findit/model/flak... appengine/findit/model/flake/base_analysis.py:44: def correct(self): On 2016/07/08 22:51:27, stgao wrote: > This seems not belonging to this base class. Done. https://codereview.chromium.org/2124973003/diff/1/appengine/findit/model/flak... appengine/findit/model/flake/base_analysis.py:78: def failure_type(self): On 2016/07/08 22:51:27, stgao wrote: > This seems not fit here. Done. https://codereview.chromium.org/2124973003/diff/1/appengine/findit/model/flak... appengine/findit/model/flake/base_analysis.py:99: def failure_type_str(self): On 2016/07/08 22:51:27, stgao wrote: > Same here. Done. https://codereview.chromium.org/2124973003/diff/1/appengine/findit/model/flak... File appengine/findit/model/flake/flake_swarming_task.py (right): https://codereview.chromium.org/2124973003/diff/1/appengine/findit/model/flak... appengine/findit/model/flake/flake_swarming_task.py:1: # Copyright 2014 The Chromium Authors. All rights reserved. On 2016/07/07 23:38:28, chanli wrote: > 2014 -> 2016 Done. https://codereview.chromium.org/2124973003/diff/1/appengine/findit/model/flak... appengine/findit/model/flake/flake_swarming_task.py:42: On 2016/07/07 23:38:28, chanli wrote: > Please add comments to below parameters. Done. https://codereview.chromium.org/2124973003/diff/1/appengine/findit/model/flak... File appengine/findit/model/flake/flake_swarming_task_result.py (right): https://codereview.chromium.org/2124973003/diff/1/appengine/findit/model/flak... appengine/findit/model/flake/flake_swarming_task_result.py:1: # Copyright 2014 The Chromium Authors. All rights reserved. On 2016/07/07 23:38:28, chanli wrote: > 2014 -> 2016 Done. https://codereview.chromium.org/2124973003/diff/1/appengine/findit/model/flak... appengine/findit/model/flake/flake_swarming_task_result.py:16: step_name, build_number, testname): # pragma: no cover On 2016/07/07 23:38:28, chanli wrote: > nit: I would use test_name instead of testname Done. https://codereview.chromium.org/2124973003/diff/1/appengine/findit/model/flak... appengine/findit/model/flake/flake_swarming_task_result.py:56: return self.key.pairs()[0][1].split('/')[0] On 2016/07/08 22:51:27, stgao wrote: > As we discussed, some code here could be cleaned up a bit after using some > BaseBuildModel class. Done. https://codereview.chromium.org/2124973003/diff/1/appengine/findit/model/flak... appengine/findit/model/flake/flake_swarming_task_result.py:74: On 2016/07/07 23:38:28, chanli wrote: > Same here: please add comments. Done. https://codereview.chromium.org/2124973003/diff/1/appengine/findit/model/flak... File appengine/findit/model/flake/master_flake_analysis.py (right): https://codereview.chromium.org/2124973003/diff/1/appengine/findit/model/flak... appengine/findit/model/flake/master_flake_analysis.py:1: # Copyright 2014 The Chromium Authors. All rights reserved. On 2016/07/07 23:38:28, chanli wrote: > 2014 -> 2016 Done. https://codereview.chromium.org/2124973003/diff/1/appengine/findit/model/flak... appengine/findit/model/flake/master_flake_analysis.py:21: def master_name(self): On 2016/07/08 22:51:27, stgao wrote: > Same here for using BaseBuildModel class. Done. https://codereview.chromium.org/2124973003/diff/1/appengine/findit/model/flak... appengine/findit/model/flake/master_flake_analysis.py:70: success_rates = ndb.FloatProperty(indexed=False, repeated=True) On 2016/07/08 22:51:27, stgao wrote: > Is the success rate per-test? If so, should we make test_name as part of the > key? Done.
These are some initial comments, but I will discuss more with you offline. https://codereview.chromium.org/2124973003/diff/20001/appengine/findit/model/... File appengine/findit/model/base_analysis.py (right): https://codereview.chromium.org/2124973003/diff/20001/appengine/findit/model/... appengine/findit/model/base_analysis.py:19: def completed(self): Should this and some other properties below belong to the Progress model? https://codereview.chromium.org/2124973003/diff/20001/appengine/findit/model/... appengine/findit/model/base_analysis.py:55: if self.result_status in ( ``result_status`` is not defined in this model. https://codereview.chromium.org/2124973003/diff/20001/appengine/findit/model/... appengine/findit/model/base_analysis.py:78: start_time = ndb.DateTimeProperty(indexed=False) These seems almost the same as those in progress.py, is there a way to merge them? https://codereview.chromium.org/2124973003/diff/20001/appengine/findit/model/... File appengine/findit/model/base_flake_model.py (right): https://codereview.chromium.org/2124973003/diff/20001/appengine/findit/model/... appengine/findit/model/base_flake_model.py:20: return '%s/%s/%s/%s' % (master_name, builder_name, step_name, build_number) Is there a special consideration of putting step name before build number? If they could be swapped, we could reuse code in base_build_model.py https://codereview.chromium.org/2124973003/diff/20001/appengine/findit/model/... File appengine/findit/model/base_swarming_task.py (right): https://codereview.chromium.org/2124973003/diff/20001/appengine/findit/model/... appengine/findit/model/base_swarming_task.py:31: created_time = ndb.DateTimeProperty(indexed=True) seems duplicate from progress.py https://codereview.chromium.org/2124973003/diff/20001/appengine/findit/model/... File appengine/findit/model/flake/base_analysis.py (right): https://codereview.chromium.org/2124973003/diff/20001/appengine/findit/model/... appengine/findit/model/flake/base_analysis.py:14: class BaseAnalysis(BaseBuildModel): Why this one is needed? Is it possible to reuse the one in findit/model/base_analysis.py? https://codereview.chromium.org/2124973003/diff/20001/appengine/findit/model/... File appengine/findit/model/flake/flake_swarming_task.py (right): https://codereview.chromium.org/2124973003/diff/20001/appengine/findit/model/... appengine/findit/model/flake/flake_swarming_task.py:60: git_hash = ndb.StringProperty(indexed=False) In other code, we use revision instead. https://codereview.chromium.org/2124973003/diff/20001/appengine/findit/model/... File appengine/findit/model/flake/flake_swarming_task_result.py (right): https://codereview.chromium.org/2124973003/diff/20001/appengine/findit/model/... appengine/findit/model/flake/flake_swarming_task_result.py:10: class FlakeSwarmingTaskResult(ndb.Model): Do you still need this one? Per discussion earlier, this one is to be deleted. Or there is some other design change afterwards? https://codereview.chromium.org/2124973003/diff/20001/appengine/findit/model/... File appengine/findit/model/progress.py (right): https://codereview.chromium.org/2124973003/diff/20001/appengine/findit/model/... appengine/findit/model/progress.py:15: task_id = ndb.StringProperty(indexed=True) This seems not fit for progress. Same for tests_statuses, build_revision and parameters below.
Hi guys, The cl is now cleaned up and only contains the files I actually want to commit. Cheers.
Addressed comments https://codereview.chromium.org/2124973003/diff/20001/appengine/findit/model/... File appengine/findit/model/base_analysis.py (right): https://codereview.chromium.org/2124973003/diff/20001/appengine/findit/model/... appengine/findit/model/base_analysis.py:19: def completed(self): On 2016/07/14 18:01:32, stgao wrote: > Should this and some other properties below belong to the Progress model? Per offline discussion, I think we are keeping them separate for now. https://codereview.chromium.org/2124973003/diff/20001/appengine/findit/model/... appengine/findit/model/base_analysis.py:55: if self.result_status in ( On 2016/07/14 18:01:33, stgao wrote: > ``result_status`` is not defined in this model. Ok it seems like result_status is more of a wf thing so I removed it. https://codereview.chromium.org/2124973003/diff/20001/appengine/findit/model/... appengine/findit/model/base_analysis.py:78: start_time = ndb.DateTimeProperty(indexed=False) On 2016/07/14 18:01:32, stgao wrote: > These seems almost the same as those in progress.py, is there a way to merge > them? Per offline discussion, we will leave this until later. https://codereview.chromium.org/2124973003/diff/20001/appengine/findit/model/... File appengine/findit/model/base_swarming_task.py (right): https://codereview.chromium.org/2124973003/diff/20001/appengine/findit/model/... appengine/findit/model/base_swarming_task.py:31: created_time = ndb.DateTimeProperty(indexed=True) On 2016/07/14 18:01:33, stgao wrote: > seems duplicate from progress.py Per offline discussion, we are now using this instead of progress.py https://codereview.chromium.org/2124973003/diff/20001/appengine/findit/model/... File appengine/findit/model/flake/flake_swarming_task_result.py (right): https://codereview.chromium.org/2124973003/diff/20001/appengine/findit/model/... appengine/findit/model/flake/flake_swarming_task_result.py:10: class FlakeSwarmingTaskResult(ndb.Model): On 2016/07/14 18:01:33, stgao wrote: > Do you still need this one? > > Per discussion earlier, this one is to be deleted. Or there is some other design > change afterwards? deleted. https://codereview.chromium.org/2124973003/diff/20001/appengine/findit/model/... File appengine/findit/model/progress.py (right): https://codereview.chromium.org/2124973003/diff/20001/appengine/findit/model/... appengine/findit/model/progress.py:15: task_id = ndb.StringProperty(indexed=True) On 2016/07/14 18:01:33, stgao wrote: > This seems not fit for progress. > > Same for tests_statuses, build_revision and parameters below. we are no longer using progress
https://codereview.chromium.org/2124973003/diff/20001/appengine/findit/model/... File appengine/findit/model/progress.py (right): https://codereview.chromium.org/2124973003/diff/20001/appengine/findit/model/... appengine/findit/model/progress.py:19: tests_statuses = ndb.JsonProperty(default={}, indexed=False, compressed=True) Try to avoid using default={} here, it is a known pitfall in python. http://stackoverflow.com/questions/5587582/numpy-array-subclass-unexpedly-sha... https://codereview.chromium.org/2124973003/diff/70001/appengine/findit/model/... File appengine/findit/model/base_analysis.py (right): https://codereview.chromium.org/2124973003/diff/70001/appengine/findit/model/... appengine/findit/model/base_analysis.py:11: from model import result_status constants, failure_type, BaseBuildModel and result_status are not used? https://codereview.chromium.org/2124973003/diff/70001/appengine/findit/model/... File appengine/findit/model/base_swarming_task.py (right): https://codereview.chromium.org/2124973003/diff/70001/appengine/findit/model/... appengine/findit/model/base_swarming_task.py:19: tests_statuses = ndb.JsonProperty(default={}, indexed=False, compressed=True) This kind of default may lead to some unexpected problems. You can either 1. tests_statuses = ndb.JsonProperty(indexed=False, compressed=True) so tests_statuses will default to None Or 2. Initialize it to {} in constructor I used approach 2 (see: https://codereview.chromium.org/2147713002/) but it's up to you https://codereview.chromium.org/2124973003/diff/70001/appengine/findit/model/... File appengine/findit/model/flake/master_flake_analysis.py (right): https://codereview.chromium.org/2124973003/diff/70001/appengine/findit/model/... appengine/findit/model/flake/master_flake_analysis.py:7: from common import constants Is constants used? https://codereview.chromium.org/2124973003/diff/70001/appengine/findit/model/... appengine/findit/model/flake/master_flake_analysis.py:56: # Generates data in the format which the template can read Nit: add '.' at the end of comment https://codereview.chromium.org/2124973003/diff/70001/appengine/findit/model/... appengine/findit/model/flake/master_flake_analysis.py:57: # Go through list of flake_swarming_tasks Nit: Goes https://codereview.chromium.org/2124973003/diff/70001/appengine/findit/model/... appengine/findit/model/flake/master_flake_analysis.py:62: fst.step_name(), fst.build_number(), Not sure if I missed, but where is fst.master_name(), fst.builder_name() and fst.build_number()? Besides, I'm not sure if it's necessary, but intuitively the order should be master_name, builder_name, build_number, step_name and test_name?
addressed comments https://codereview.chromium.org/2124973003/diff/70001/appengine/findit/model/... File appengine/findit/model/base_analysis.py (right): https://codereview.chromium.org/2124973003/diff/70001/appengine/findit/model/... appengine/findit/model/base_analysis.py:11: from model import result_status On 2016/07/19 19:12:20, chanli wrote: > constants, failure_type, BaseBuildModel and result_status are not used? Done. https://codereview.chromium.org/2124973003/diff/70001/appengine/findit/model/... File appengine/findit/model/base_swarming_task.py (right): https://codereview.chromium.org/2124973003/diff/70001/appengine/findit/model/... appengine/findit/model/base_swarming_task.py:19: tests_statuses = ndb.JsonProperty(default={}, indexed=False, compressed=True) On 2016/07/19 19:12:20, chanli wrote: > This kind of default may lead to some unexpected problems. You can either > 1. tests_statuses = ndb.JsonProperty(indexed=False, compressed=True) > so tests_statuses will default to None > Or > > 2. Initialize it to {} in constructor > > I used approach 2 (see: https://codereview.chromium.org/2147713002/) but it's up > to you I just removed it (approach 1). I think it should be ok. https://codereview.chromium.org/2124973003/diff/70001/appengine/findit/model/... File appengine/findit/model/flake/master_flake_analysis.py (right): https://codereview.chromium.org/2124973003/diff/70001/appengine/findit/model/... appengine/findit/model/flake/master_flake_analysis.py:7: from common import constants On 2016/07/19 19:12:20, chanli wrote: > Is constants used? Done. https://codereview.chromium.org/2124973003/diff/70001/appengine/findit/model/... appengine/findit/model/flake/master_flake_analysis.py:56: # Generates data in the format which the template can read On 2016/07/19 19:12:20, chanli wrote: > Nit: add '.' at the end of comment Done. https://codereview.chromium.org/2124973003/diff/70001/appengine/findit/model/... appengine/findit/model/flake/master_flake_analysis.py:57: # Go through list of flake_swarming_tasks On 2016/07/19 19:12:20, chanli wrote: > Nit: Goes Done. https://codereview.chromium.org/2124973003/diff/70001/appengine/findit/model/... appengine/findit/model/flake/master_flake_analysis.py:62: fst.step_name(), fst.build_number(), On 2016/07/19 19:12:20, chanli wrote: > Not sure if I missed, but where is fst.master_name(), fst.builder_name() and > fst.build_number()? > > Besides, I'm not sure if it's necessary, but intuitively the order should be > master_name, builder_name, build_number, step_name and test_name? I think those are from base_build_model? I switched the order, per your suggestion.
removed a piece of unused code.
https://codereview.chromium.org/2124973003/diff/90001/appengine/findit/model/... File appengine/findit/model/flake/master_flake_analysis.py (right): https://codereview.chromium.org/2124973003/diff/90001/appengine/findit/model/... appengine/findit/model/flake/master_flake_analysis.py:53: def generate_data(flake_swarming_tasks): So this function is not needed any more?
lijeffrey@chromium.org changed reviewers: + lijeffrey@chromium.org
Looks good for the most part, just a few style nits https://codereview.chromium.org/2124973003/diff/110001/appengine/findit/model... File appengine/findit/model/base_analysis.py (right): https://codereview.chromium.org/2124973003/diff/110001/appengine/findit/model... appengine/findit/model/base_analysis.py:11: """Represents an analysis of a build of a builder in a Chromium waterfall. nit: "... a build analysis of a ..." see if that gets the closing """ to fit on 1 line https://codereview.chromium.org/2124973003/diff/110001/appengine/findit/model... File appengine/findit/model/base_swarming_task.py (right): https://codereview.chromium.org/2124973003/diff/110001/appengine/findit/model... appengine/findit/model/base_swarming_task.py:13: """ nit: closing """ should be on the same line if possible https://codereview.chromium.org/2124973003/diff/110001/appengine/findit/model... File appengine/findit/model/flake/flake_swarming_task.py (right): https://codereview.chromium.org/2124973003/diff/110001/appengine/findit/model... appengine/findit/model/flake/flake_swarming_task.py:53: master_name, builder_name, build_number, step_name, test_name).get( nit: 4 spaces after opening ( if the following stuff is on a new line https://codereview.chromium.org/2124973003/diff/110001/appengine/findit/model... appengine/findit/model/flake/flake_swarming_task.py:55: # In how many runs did the test succeed? nit: Be explicit what "succeeded" means. Is it the test passed, or failed? i.e. "Number of runs the test passed." https://codereview.chromium.org/2124973003/diff/110001/appengine/findit/model... appengine/findit/model/flake/flake_swarming_task.py:57: # How many times did we rerun the test? Nit: How many times the test was rerun https://codereview.chromium.org/2124973003/diff/110001/appengine/findit/model... File appengine/findit/model/flake/master_flake_analysis.py (right): https://codereview.chromium.org/2124973003/diff/110001/appengine/findit/model... appengine/findit/model/flake/master_flake_analysis.py:14: nit: For docstrings using """ try to get a 1 line description if possible with the closing """ in the same line. Else get rid of the extra empty line here
addressed more comments https://codereview.chromium.org/2124973003/diff/90001/appengine/findit/model/... File appengine/findit/model/flake/master_flake_analysis.py (right): https://codereview.chromium.org/2124973003/diff/90001/appengine/findit/model/... appengine/findit/model/flake/master_flake_analysis.py:53: def generate_data(flake_swarming_tasks): On 2016/07/19 20:02:10, chanli wrote: > So this function is not needed any more? Correct - I think it would be easier to delete this and then add something new later once I create the UI to display the swarming rerun. https://codereview.chromium.org/2124973003/diff/110001/appengine/findit/model... File appengine/findit/model/base_analysis.py (right): https://codereview.chromium.org/2124973003/diff/110001/appengine/findit/model... appengine/findit/model/base_analysis.py:11: """Represents an analysis of a build of a builder in a Chromium waterfall. On 2016/07/19 22:32:21, lijeffrey wrote: > nit: "... a build analysis of a ..." see if that gets the closing """ to fit on > 1 line Done. https://codereview.chromium.org/2124973003/diff/110001/appengine/findit/model... File appengine/findit/model/base_swarming_task.py (right): https://codereview.chromium.org/2124973003/diff/110001/appengine/findit/model... appengine/findit/model/base_swarming_task.py:13: """ On 2016/07/19 22:32:21, lijeffrey wrote: > nit: closing """ should be on the same line if possible Done. https://codereview.chromium.org/2124973003/diff/110001/appengine/findit/model... File appengine/findit/model/flake/flake_swarming_task.py (right): https://codereview.chromium.org/2124973003/diff/110001/appengine/findit/model... appengine/findit/model/flake/flake_swarming_task.py:53: master_name, builder_name, build_number, step_name, test_name).get( On 2016/07/19 22:32:21, lijeffrey wrote: > nit: 4 spaces after opening ( if the following stuff is on a new line Done. https://codereview.chromium.org/2124973003/diff/110001/appengine/findit/model... appengine/findit/model/flake/flake_swarming_task.py:55: # In how many runs did the test succeed? On 2016/07/19 22:32:21, lijeffrey wrote: > nit: Be explicit what "succeeded" means. Is it the test passed, or failed? i.e. > "Number of runs the test passed." Done. https://codereview.chromium.org/2124973003/diff/110001/appengine/findit/model... appengine/findit/model/flake/flake_swarming_task.py:57: # How many times did we rerun the test? On 2016/07/19 22:32:21, lijeffrey wrote: > Nit: How many times the test was rerun Done. https://codereview.chromium.org/2124973003/diff/110001/appengine/findit/model... File appengine/findit/model/flake/master_flake_analysis.py (right): https://codereview.chromium.org/2124973003/diff/110001/appengine/findit/model... appengine/findit/model/flake/master_flake_analysis.py:14: On 2016/07/19 22:32:21, lijeffrey wrote: > nit: For docstrings using """ try to get a 1 line description if possible with > the closing """ in the same line. Else get rid of the extra empty line here Done.
On 2016/07/20 18:11:00, caiw wrote: > addressed more comments > > https://codereview.chromium.org/2124973003/diff/90001/appengine/findit/model/... > File appengine/findit/model/flake/master_flake_analysis.py (right): > > https://codereview.chromium.org/2124973003/diff/90001/appengine/findit/model/... > appengine/findit/model/flake/master_flake_analysis.py:53: def > generate_data(flake_swarming_tasks): > On 2016/07/19 20:02:10, chanli wrote: > > So this function is not needed any more? > > Correct - I think it would be easier to delete this and then add something new > later once I create the UI to display the swarming rerun. > > https://codereview.chromium.org/2124973003/diff/110001/appengine/findit/model... > File appengine/findit/model/base_analysis.py (right): > > https://codereview.chromium.org/2124973003/diff/110001/appengine/findit/model... > appengine/findit/model/base_analysis.py:11: """Represents an analysis of a build > of a builder in a Chromium waterfall. > On 2016/07/19 22:32:21, lijeffrey wrote: > > nit: "... a build analysis of a ..." see if that gets the closing """ to fit > on > > 1 line > > Done. > > https://codereview.chromium.org/2124973003/diff/110001/appengine/findit/model... > File appengine/findit/model/base_swarming_task.py (right): > > https://codereview.chromium.org/2124973003/diff/110001/appengine/findit/model... > appengine/findit/model/base_swarming_task.py:13: """ > On 2016/07/19 22:32:21, lijeffrey wrote: > > nit: closing """ should be on the same line if possible > > Done. > > https://codereview.chromium.org/2124973003/diff/110001/appengine/findit/model... > File appengine/findit/model/flake/flake_swarming_task.py (right): > > https://codereview.chromium.org/2124973003/diff/110001/appengine/findit/model... > appengine/findit/model/flake/flake_swarming_task.py:53: master_name, > builder_name, build_number, step_name, test_name).get( > On 2016/07/19 22:32:21, lijeffrey wrote: > > nit: 4 spaces after opening ( if the following stuff is on a new line > > Done. > > https://codereview.chromium.org/2124973003/diff/110001/appengine/findit/model... > appengine/findit/model/flake/flake_swarming_task.py:55: # In how many runs did > the test succeed? > On 2016/07/19 22:32:21, lijeffrey wrote: > > nit: Be explicit what "succeeded" means. Is it the test passed, or failed? > i.e. > > "Number of runs the test passed." > > Done. > > https://codereview.chromium.org/2124973003/diff/110001/appengine/findit/model... > appengine/findit/model/flake/flake_swarming_task.py:57: # How many times did we > rerun the test? > On 2016/07/19 22:32:21, lijeffrey wrote: > > Nit: How many times the test was rerun > > Done. > > https://codereview.chromium.org/2124973003/diff/110001/appengine/findit/model... > File appengine/findit/model/flake/master_flake_analysis.py (right): > > https://codereview.chromium.org/2124973003/diff/110001/appengine/findit/model... > appengine/findit/model/flake/master_flake_analysis.py:14: > On 2016/07/19 22:32:21, lijeffrey wrote: > > nit: For docstrings using """ try to get a 1 line description if possible with > > the closing """ in the same line. Else get rid of the extra empty line here > > Done. lgtm
lgtm with nits https://codereview.chromium.org/2124973003/diff/130001/appengine/findit/model... File appengine/findit/model/base_analysis.py (right): https://codereview.chromium.org/2124973003/diff/130001/appengine/findit/model... appengine/findit/model/base_analysis.py:11: """Represents a build analysis of a builder in a Chromium waterfall. """ nit: remove the extra space before closing """ https://codereview.chromium.org/2124973003/diff/130001/appengine/findit/model... appengine/findit/model/base_analysis.py:38: default=analysis_status.PENDING, indexed=False) nit: this looks like it fits on 1 line https://codereview.chromium.org/2124973003/diff/130001/appengine/findit/model... File appengine/findit/model/base_swarming_task.py (right): https://codereview.chromium.org/2124973003/diff/130001/appengine/findit/model... appengine/findit/model/base_swarming_task.py:12: """Represents the progress of a general swarming task. """ nit: get rid of extra space before closing """ https://codereview.chromium.org/2124973003/diff/130001/appengine/findit/model... File appengine/findit/model/flake/flake_swarming_task.py (right): https://codereview.chromium.org/2124973003/diff/130001/appengine/findit/model... appengine/findit/model/flake/flake_swarming_task.py:14: """ nit: closing """ fits on previous line https://codereview.chromium.org/2124973003/diff/130001/appengine/findit/model... appengine/findit/model/flake/flake_swarming_task.py:47: nit: remove extra empty line before @staticmethod. run gpylint <this file> to see if it complains about other style nits https://codereview.chromium.org/2124973003/diff/130001/appengine/findit/model... appengine/findit/model/flake/flake_swarming_task.py:53: master_name, builder_name, build_number, step_name, test_name).get( line too long. find a way to put .get() in the same line
Added tests and linted everything
lgtm. feel free to commit once comments are addressed https://codereview.chromium.org/2124973003/diff/150001/appengine/findit/model... File appengine/findit/model/flake/test/master_flake_analysis_test.py (right): https://codereview.chromium.org/2124973003/diff/150001/appengine/findit/model... appengine/findit/model/flake/test/master_flake_analysis_test.py:65: analysis.status = analysis_status.PENDING you're going to hate me for this but this test looks like it should be broken into separate tests usually if a test is a 1-liner you can bundle several of them in 1 test, i.e. testAdd(self): self.assertEqual(0, add(0, 0)) self.assertEqual(1, add(0, 1)) self.assertEqual(1, add (2, -1)) ... but in this case each combination needs a slightly different setup, so break this down into separate tests in case any 1 of them is broken it's easy to tell which combination broke https://codereview.chromium.org/2124973003/diff/150001/appengine/findit/model... File appengine/findit/model/test/base_analysis_test.py (right): https://codereview.chromium.org/2124973003/diff/150001/appengine/findit/model... appengine/findit/model/test/base_analysis_test.py:74: self.assertEqual('Completed', self.dummy_model.status_description) same here, you may want to break this test into separate ones since they have different setups. Alternatively it may be worth exploring creating a dict that maps analysis_status to expected status_description then iterating over that dict i.e. descriptions = { analysis_status.PENDING: 'Pending' analysis_status.RUNNING: 'Running' ... } for status, description in descriptions: self.dummy_model.status = status self.assertEqual(...) not a big deal in either case.
The CQ bit was checked by caiw@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from chanli@chromium.org, lijeffrey@chromium.org Link to the patchset: https://codereview.chromium.org/2124973003/#ps170001 (title: "Addressed comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== These are the database objects used while finding the regression range. BUG=617808 ========== to ========== These are the database objects used while finding the regression range. BUG=617808 Committed: https://chromium.googlesource.com/infra/infra/+/0e9802728999cc1072423b17f6d6d... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:170001) as https://chromium.googlesource.com/infra/infra/+/0e9802728999cc1072423b17f6d6d... |
