Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
778923003:
Added a comment about why testVTuneProfiler is disabled (Closed)
DescriptionAdded a comment about why testVTuneProfiler is disabled
BUG=437085
Committed: https://crrev.com/b047281fca7484dc0381102ae204d48ebb003751
Cr-Commit-Position: refs/heads/master@{#307046}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Disabled test everywhere #Messages
Total messages: 14 (3 generated)
sullivan@chromium.org changed reviewers: + tonyg@chromium.org
https://codereview.chromium.org/778923003/diff/1/tools/telemetry/telemetry/co... File tools/telemetry/telemetry/core/platform/profiler/vtune_profiler_unittest.py (right): https://codereview.chromium.org/778923003/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/platform/profiler/vtune_profiler_unittest.py:92: options.browser_type.startswith('cros')) This is a really strange way of writing this test. Perhaps it should use @benchmark.Disabled and only run on supported platforms? https://codereview.chromium.org/778923003/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/platform/profiler/vtune_profiler_unittest.py:102: @benchmark.Disabled('android') The comment implies that this should be disabled everywhere, but this only disables it on android. Should this just be changed to disable everywhere? Otherwise, it's not clear to me how it's running in other places.
https://codereview.chromium.org/778923003/diff/1/tools/telemetry/telemetry/co... File tools/telemetry/telemetry/core/platform/profiler/vtune_profiler_unittest.py (right): https://codereview.chromium.org/778923003/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/platform/profiler/vtune_profiler_unittest.py:102: @benchmark.Disabled('android') On 2014/12/05 03:07:35, tonyg wrote: > The comment implies that this should be disabled everywhere, but this only > disables it on android. Should this just be changed to disable everywhere? > Otherwise, it's not clear to me how it's running in other places. Dominik, this comment is based on your comments in bug 437035. Can you clarify whether it should be enabled for all platforms, and why?
dominikg@chromium.org changed reviewers: + dominikg@chromium.org
https://codereview.chromium.org/778923003/diff/1/tools/telemetry/telemetry/co... File tools/telemetry/telemetry/core/platform/profiler/vtune_profiler_unittest.py (right): https://codereview.chromium.org/778923003/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/platform/profiler/vtune_profiler_unittest.py:102: @benchmark.Disabled('android') On 2014/12/05 03:10:30, sullivan wrote: > On 2014/12/05 03:07:35, tonyg wrote: > > The comment implies that this should be disabled everywhere, but this only > > disables it on android. Should this just be changed to disable everywhere? > > Otherwise, it's not clear to me how it's running in other places. > > Dominik, this comment is based on your comments in bug 437035. Can you clarify > whether it should be enabled for all platforms, and why? I think Tony's got a point. It makes sense to disable it on all platforms, because none of the bots (regardless of platform) has VTune installed. That also seems to be more in line with the comment you added.
https://codereview.chromium.org/778923003/diff/1/tools/telemetry/telemetry/co... File tools/telemetry/telemetry/core/platform/profiler/vtune_profiler_unittest.py (right): https://codereview.chromium.org/778923003/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/platform/profiler/vtune_profiler_unittest.py:92: options.browser_type.startswith('cros')) On 2014/12/05 03:07:35, tonyg wrote: > This is a really strange way of writing this test. Perhaps it should use > @benchmark.Disabled and only run on supported platforms? I'm not 100% sure what the intent of this code is, and it's not causing any known breakages. Dominik, would you be okay with addressing this in a follow up? https://codereview.chromium.org/778923003/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/platform/profiler/vtune_profiler_unittest.py:102: @benchmark.Disabled('android') On 2014/12/05 08:43:19, Dominik Grewe wrote: > On 2014/12/05 03:10:30, sullivan wrote: > > On 2014/12/05 03:07:35, tonyg wrote: > > > The comment implies that this should be disabled everywhere, but this only > > > disables it on android. Should this just be changed to disable everywhere? > > > Otherwise, it's not clear to me how it's running in other places. > > > > Dominik, this comment is based on your comments in bug 437035. Can you clarify > > whether it should be enabled for all platforms, and why? > > I think Tony's got a point. It makes sense to disable it on all platforms, > because none of the bots (regardless of platform) has VTune installed. That also > seems to be more in line with the comment you added. Done.
On 2014/12/05 13:49:32, sullivan wrote: > https://codereview.chromium.org/778923003/diff/1/tools/telemetry/telemetry/co... > File tools/telemetry/telemetry/core/platform/profiler/vtune_profiler_unittest.py > (right): > > https://codereview.chromium.org/778923003/diff/1/tools/telemetry/telemetry/co... > tools/telemetry/telemetry/core/platform/profiler/vtune_profiler_unittest.py:92: > options.browser_type.startswith('cros')) > On 2014/12/05 03:07:35, tonyg wrote: > > This is a really strange way of writing this test. Perhaps it should use > > @benchmark.Disabled and only run on supported platforms? > > I'm not 100% sure what the intent of this code is, and it's not causing any > known breakages. Dominik, would you be okay with addressing this in a follow up? I'm not working on Chrome any more and don't have a working setup. Could you do me a favour and just add it to this CL? I think if we add a @benchmark.Disabled for all platforms but Android we can remove the two trailing 'or' checks.
Lgtm
On 2014/12/05 14:40:29, Dominik Grewe wrote: > On 2014/12/05 13:49:32, sullivan wrote: > > > https://codereview.chromium.org/778923003/diff/1/tools/telemetry/telemetry/co... > > File > tools/telemetry/telemetry/core/platform/profiler/vtune_profiler_unittest.py > > (right): > > > > > https://codereview.chromium.org/778923003/diff/1/tools/telemetry/telemetry/co... > > > tools/telemetry/telemetry/core/platform/profiler/vtune_profiler_unittest.py:92: > > options.browser_type.startswith('cros')) > > On 2014/12/05 03:07:35, tonyg wrote: > > > This is a really strange way of writing this test. Perhaps it should use > > > @benchmark.Disabled and only run on supported platforms? > > > > I'm not 100% sure what the intent of this code is, and it's not causing any > > known breakages. Dominik, would you be okay with addressing this in a follow > up? > > I'm not working on Chrome any more and don't have a working setup. Could you do > me a favour and just add it to this CL? I think if we add a @benchmark.Disabled > for all platforms but Android we can remove the two trailing 'or' checks. Oh, I see! I'll do this in a follow up.
The CQ bit was checked by sullivan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/778923003/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/b047281fca7484dc0381102ae204d48ebb003751 Cr-Commit-Position: refs/heads/master@{#307046} |
