Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2232613002:
Chop off platform name from step name (Closed)
DescriptionChop off platform name from step name
BUG=635011
Committed: https://chromium.googlesource.com/infra/infra/+/445359f7853494fee3305e6f3ad99c2c7c34aed6
Patch Set 1 #
Total comments: 10
Patch Set 2 : Remove ' on ', simplification, and code review changes #
Total comments: 2
Messages
Total messages: 14 (5 generated)
Description was changed from ========== Chop off platform name from step name BUG=635011 ========== to ========== Chop off platform name from step name BUG=635011 ==========
josiahk@google.com changed reviewers: + chanli@chromium.org, lijeffrey@chromium.org, stgao@chromium.org
Hello, PTAL! :-) Josiah
https://codereview.chromium.org/2232613002/diff/1/appengine/findit/waterfall/... File appengine/findit/waterfall/test/try_job_util_test.py (right): https://codereview.chromium.org/2232613002/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/test/try_job_util_test.py:1198: ('a_tests on Platform', 'a_tests on '), so why should the ' on ' be left in? https://codereview.chromium.org/2232613002/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/test/try_job_util_test.py:1198: ('a_tests on Platform', 'a_tests on '), it's easier just to make 1 for each of these self.assertEqual(try_job_util.ChopOffPlatformFroStepName(...), ...) self.assertEqual(...) ... https://codereview.chromium.org/2232613002/diff/1/appengine/findit/waterfall/... File appengine/findit/waterfall/try_job_util.py (right): https://codereview.chromium.org/2232613002/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/try_job_util.py:147: def _ChopOffPlatformFromStepName(step_name): nit: rename this function '_RemovePlatformFromStepName' https://codereview.chromium.org/2232613002/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/try_job_util.py:151: step_name: Raw step name. Example: "net_unittests on Windows-10" single quotes for strings https://codereview.chromium.org/2232613002/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/try_job_util.py:157: return step_name[:step_name.find(' on ') + 4] 4? what's this magic number and why is it being used here? The code here is cryptic though all it's doing is blindly returning everything up to and including ' on '. Better to store ' on ' as a variable and use + len(variable)
Patchset #2 (id:20001) has been deleted
Hello--PTAL! https://codereview.chromium.org/2232613002/diff/1/appengine/findit/waterfall/... File appengine/findit/waterfall/test/try_job_util_test.py (right): https://codereview.chromium.org/2232613002/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/test/try_job_util_test.py:1198: ('a_tests on Platform', 'a_tests on '), On 2016/08/10 18:48:08, lijeffrey wrote: > it's easier just to make 1 for each of these > > self.assertEqual(try_job_util.ChopOffPlatformFroStepName(...), ...) > self.assertEqual(...) > ... Done. https://codereview.chromium.org/2232613002/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/test/try_job_util_test.py:1198: ('a_tests on Platform', 'a_tests on '), On 2016/08/10 18:48:08, lijeffrey wrote: > so why should the ' on ' be left in? So that 'super_tests on Windows' doesn't match 'super_tests'. After discussing offline with Chan, the scenario of having 'step_a on platform_x' and just 'step_a' shouldn't happen, so it shouldn't matter to include ' on ' or not. I think it's cleaner without the ' on ', and I took it out. Thanks! https://codereview.chromium.org/2232613002/diff/1/appengine/findit/waterfall/... File appengine/findit/waterfall/try_job_util.py (right): https://codereview.chromium.org/2232613002/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/try_job_util.py:147: def _ChopOffPlatformFromStepName(step_name): On 2016/08/10 18:48:08, lijeffrey wrote: > nit: rename this function '_RemovePlatformFromStepName' Done. https://codereview.chromium.org/2232613002/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/try_job_util.py:151: step_name: Raw step name. Example: "net_unittests on Windows-10" On 2016/08/10 18:48:08, lijeffrey wrote: > single quotes for strings Done. https://codereview.chromium.org/2232613002/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/try_job_util.py:157: return step_name[:step_name.find(' on ') + 4] On 2016/08/10 18:48:08, lijeffrey wrote: > 4? what's this magic number and why is it being used here? > > The code here is cryptic though all it's doing is blindly returning everything > up to and including ' on '. Better to store ' on ' as a variable and use + > len(variable) Fixed!
https://codereview.chromium.org/2232613002/diff/40001/appengine/findit/waterf... File appengine/findit/waterfall/try_job_util.py (right): https://codereview.chromium.org/2232613002/diff/40001/appengine/findit/waterf... appengine/findit/waterfall/try_job_util.py:161: return step_name.split(separator)[0] I believe step_name should always be 1 word, so you should be able just to use step_name.split()[0] as we do in handlers/handler_util.py. Chan can you please confirm this is the behavior that is to be expected? If so this method is unneeded and you can use the 1-liner. Else we should refactor that into a shared module that does the .split()[0] that this and handlers_util.py can both use but that's probably outside the scope of this CL. If we choose to go that route, put a TODO(lijeffrey): refactor this method to share with handlers_util.py.
https://codereview.chromium.org/2232613002/diff/40001/appengine/findit/waterf... File appengine/findit/waterfall/try_job_util.py (right): https://codereview.chromium.org/2232613002/diff/40001/appengine/findit/waterf... appengine/findit/waterfall/try_job_util.py:161: return step_name.split(separator)[0] On 2016/08/10 23:24:25, lijeffrey wrote: > I believe step_name should always be 1 word, so you should be able just to use > step_name.split()[0] as we do in handlers/handler_util.py. > > Chan can you please confirm this is the behavior that is to be expected? > > If so this method is unneeded and you can use the 1-liner. Else we should > refactor that into a shared module that does the .split()[0] that this and > handlers_util.py can both use but that's probably outside the scope of this CL. > If we choose to go that route, put a TODO(lijeffrey): refactor this method to > share with handlers_util.py. The changes here and in handler_util are all temporary change: they should work for all cases we've seen so far but not 100% guaranteed. I'm thinking of keeping track of the real step name without platform in the analysis results. So I think this change is fine for me right now and feel free to add TODO for later change.
Chan: Is this CL still valid?
The CQ bit was checked by chanli@chromium.org
lgtm
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 ========== Chop off platform name from step name BUG=635011 ========== to ========== Chop off platform name from step name BUG=635011 Committed: https://chromium.googlesource.com/infra/infra/+/445359f7853494fee3305e6f3ad99... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:40001) as https://chromium.googlesource.com/infra/infra/+/445359f7853494fee3305e6f3ad99... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
