Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2133093002:
[system-health] Enable browser logging in the smoke test (Closed)
|
Created:
4 years, 5 months ago by petrcermak Modified:
4 years, 5 months ago Reviewers:
nednguyen CC:
chromium-reviews, telemetry-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[system-health] Enable browser logging in the smoke test
BUG=623058, 625172
Committed: https://crrev.com/104d1e7a0e23f9e7a9fb5572026580a98d2944c0
Cr-Commit-Position: refs/heads/master@{#404403}
Patch Set 1 #Patch Set 2 : Use string literal #Messages
Total messages: 28 (11 generated)
Description was changed from ========== [system-health] Enable browser logging in the smoke test BUG=623058,625172 ========== to ========== [system-health] Enable browser logging in the smoke test BUG=623058,625172 CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq ==========
The CQ bit was checked by petrcermak@chromium.org
The CQ bit was unchecked by petrcermak@chromium.org
The CQ bit was checked by petrcermak@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
There were warnings when CQ was processing your CL: * CQ_EXTRA_TRYBOTS flag is deprecated and will soon be removed. Use CQ_INCLUDE_TRYBOTS instead.
There were warnings when CQ was processing your CL: * CQ_EXTRA_TRYBOTS flag is deprecated and will soon be removed. Use CQ_INCLUDE_TRYBOTS instead.
petrcermak@chromium.org changed reviewers: + nednguyen@google.com
PTAL. Thanks, Petr
There were warnings when CQ was processing your CL: * CQ_EXTRA_TRYBOTS flag is deprecated and will soon be removed. Use CQ_INCLUDE_TRYBOTS instead.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
On 2016/07/08 10:38:38, commit-bot: I haz the power wrote: > Dry run: Try jobs failed on following builders: > linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, > https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) > win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...) lgtm but looks like you need to fix things a bit: " AttributeError: BrowserFinderOptions instance has no attribute 'NON_VERBOSE_LOGGING'
On 2016/07/08 11:26:00, nednguyen (ooo til 7-11) wrote: > On 2016/07/08 10:38:38, commit-bot: I haz the power wrote: > > Dry run: Try jobs failed on following builders: > > linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, > > > https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) > > win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, > > > http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...) > > lgtm but looks like you need to fix things a bit: > " AttributeError: BrowserFinderOptions instance has no attribute > 'NON_VERBOSE_LOGGING' So the problem here is that NON_VERBOSE_LOGGING is defined on BrowserOptions rather than BrowserFinderOptions. Unfortunately, I can't access BrowserOptions from here because that class is in telemetry/internal. I can see two options to address this: 1. Replace |options.NON_VERBOSE_LOGGING| with |'non-verbose'| (use a string literal directly). I'm not a big fan of this solution because, if we do this, there's no point in having the NO_LOGGING/NON_VERBOSE_LOGGING/VERBOSE_LOGGING constants at all — we could just use string literals everywhere. 2. Move the enum constants to some public file/class that can be imported from outside telemetry. Any thoughts where to put them? WDYT? Thanks, Petr
On 2016/07/08 13:10:21, petrcermak wrote: > On 2016/07/08 11:26:00, nednguyen (ooo til 7-11) wrote: > > On 2016/07/08 10:38:38, commit-bot: I haz the power wrote: > > > Dry run: Try jobs failed on following builders: > > > linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, > > > > > > https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) > > > win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, > > > > > > http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...) > > > > lgtm but looks like you need to fix things a bit: > > " AttributeError: BrowserFinderOptions instance has no attribute > > 'NON_VERBOSE_LOGGING' > > So the problem here is that NON_VERBOSE_LOGGING is defined on BrowserOptions > rather than BrowserFinderOptions. Unfortunately, I can't access BrowserOptions > from here because that class is in telemetry/internal. I can see two options to > address this: > > 1. Replace |options.NON_VERBOSE_LOGGING| with |'non-verbose'| (use a string > literal directly). I'm not a big fan of this solution because, if we do this, > there's no point in having the NO_LOGGING/NON_VERBOSE_LOGGING/VERBOSE_LOGGING > constants at all — we could just use string literals everywhere. > 2. Move the enum constants to some public file/class that can be imported from > outside telemetry. Any thoughts where to put them? > > WDYT? > > Thanks, > Petr Just use string literal is ok. These flags string are supposed to be stable as they are also passed by users from command line.
The CQ bit was checked by petrcermak@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nednguyen@google.com Link to the patchset: https://codereview.chromium.org/2133093002/#ps20001 (title: "Use string literal")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
There were warnings when CQ was processing your CL: * CQ_EXTRA_TRYBOTS flag is deprecated and will soon be removed. Use CQ_INCLUDE_TRYBOTS instead.
There were warnings when CQ was processing your CL: * CQ_EXTRA_TRYBOTS flag is deprecated and will soon be removed. Use CQ_INCLUDE_TRYBOTS instead.
There were warnings when CQ was processing your CL: * CQ_EXTRA_TRYBOTS flag is deprecated and will soon be removed. Use CQ_INCLUDE_TRYBOTS instead.
There were warnings when CQ was processing your CL: * CQ_EXTRA_TRYBOTS flag is deprecated and will soon be removed. Use CQ_INCLUDE_TRYBOTS instead.
Description was changed from ========== [system-health] Enable browser logging in the smoke test BUG=623058,625172 CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq ========== to ========== [system-health] Enable browser logging in the smoke test BUG=623058,625172 ==========
On 2016/07/08 14:31:27, commit-bot: I haz the power wrote: > There were warnings when CQ was processing your CL: > * CQ_EXTRA_TRYBOTS flag is deprecated and will soon be removed. Use > CQ_INCLUDE_TRYBOTS instead. Removed CQ_EXTRA_TRYBOTS since this does not affect any existing benchmark
Message was sent while issue was closed.
Description was changed from ========== [system-health] Enable browser logging in the smoke test BUG=623058,625172 ========== to ========== [system-health] Enable browser logging in the smoke test BUG=623058,625172 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== [system-health] Enable browser logging in the smoke test BUG=623058,625172 ========== to ========== [system-health] Enable browser logging in the smoke test BUG=623058,625172 Committed: https://crrev.com/104d1e7a0e23f9e7a9fb5572026580a98d2944c0 Cr-Commit-Position: refs/heads/master@{#404403} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/104d1e7a0e23f9e7a9fb5572026580a98d2944c0 Cr-Commit-Position: refs/heads/master@{#404403} |
