Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
884573004:
Check the context type in StopChromeTracing (Closed)
|
Created:
5 years, 10 months ago by horo Modified:
5 years, 10 months ago CC:
chromium-reviews, yurys, paulirish+reviews_chromium.org, telemetry-reviews_chromium.org, devtools-reviews_chromium.org, aandrey+blink_chromium.org, pfeldman, serviceworker-reviews, lafeenstra Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCheck the context type in StopChromeTracing
DevToolsClientBackend.StopChromeTracing() tries to call console.time & console.timeEnd.
But in ServiceWorker telemetry tests, ServiceWorker may have already stopped by itself.
This is causing the test flakiness.
So this cl add checking if the context type is 'iframe', 'page' or 'webview'.
BUG=433943
TEST=./tools/perf/run_benchmark --browser-executable=~/chromium/src/out/Debug/chrome --also-run-disabled-tests service_worker.service_worker
Committed: https://crrev.com/c8ad009e818cc08a36d5f6a4fe68c97ee3f88179
Cr-Commit-Position: refs/heads/master@{#317730}
Patch Set 1 #
Total comments: 2
Patch Set 2 : check the context type in StopChromeTracing #
Total comments: 4
Patch Set 3 : add test #
Total comments: 2
Patch Set 4 : use @decorators.Isolated #Patch Set 5 : disable test #
Messages
Total messages: 37 (7 generated)
horo@chromium.org changed reviewers: + chrishenry@google.com
chrishenry@ Could you please review this?
nednguyen@google.com changed reviewers: + nednguyen@google.com, sullivan@chromium.org
https://codereview.chromium.org/884573004/diff/1/tools/telemetry/telemetry/co... File tools/telemetry/telemetry/core/backends/chrome_inspector/devtools_client_backend.py (right): https://codereview.chromium.org/884573004/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/backends/chrome_inspector/devtools_client_backend.py:110: Branching the core library just for a single use case like this is not lgtm at all.
https://codereview.chromium.org/884573004/diff/1/tools/telemetry/telemetry/co... File tools/telemetry/telemetry/core/backends/chrome_inspector/devtools_client_backend.py (right): https://codereview.chromium.org/884573004/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/backends/chrome_inspector/devtools_client_backend.py:110: On 2015/02/12 20:52:48, nednguyen wrote: > Branching the core library just for a single use case like this is not lgtm at > all. Agreed we need to handle this better. Is the error you're trying to fix still the same callstack as in https://code.google.com/p/chromium/issues/detail?id=433943#c19 ?
On 2015/02/12 21:02:45, sullivan wrote: > https://codereview.chromium.org/884573004/diff/1/tools/telemetry/telemetry/co... > File > tools/telemetry/telemetry/core/backends/chrome_inspector/devtools_client_backend.py > (right): > > https://codereview.chromium.org/884573004/diff/1/tools/telemetry/telemetry/co... > tools/telemetry/telemetry/core/backends/chrome_inspector/devtools_client_backend.py:110: > > On 2015/02/12 20:52:48, nednguyen wrote: > > Branching the core library just for a single use case like this is not lgtm at > > all. > > Agreed we need to handle this better. > > Is the error you're trying to fix still the same callstack as in > https://code.google.com/p/chromium/issues/detail?id=433943#c19 ? No. This failure was caused by speedindex. And I think it is fixed by https://codereview.chromium.org/805393003. The error that I want to fix is this: --- horo@horo:~/chromium/src$ ./tools/perf/run_benchmark --browser-executable=~/chromium/src/out/Debug/chrome --also-run-disabled-tests service_worker.service_worker [ RUN ] https://jakearchibald.github.io/trained-to-thrill/ WARNING:root:Could not find Flash at ./third_party/adobe/flash/binaries/ppapi/linux/libpepflashplayer.so. Continuing without Flash. To run with Flash, check it out via http://go/read-src-internal Traceback (most recent call last): File "./tools/perf/../telemetry/telemetry/user_story/user_story_runner.py", line 100, in _RunUserStoryAndProcessErrorIfNeeded state.RunUserStory(results) File "./tools/perf/../telemetry/telemetry/page/shared_page_state.py", line 242, in RunUserStory self._test.RunPage(self._current_page, self._current_tab, results) File "./tools/perf/../telemetry/telemetry/page/page_test.py", line 239, in RunPage self.ValidateAndMeasurePage(page, tab, results) File "/usr/local/google/home/horo/chromium/src/tools/perf/benchmarks/service_worker.py", line 107, in ValidateAndMeasurePage self._timeline_controller.Stop(tab, results) File "/usr/local/google/home/horo/chromium/src/tools/perf/measurements/timeline_controller.py", line 53, in Stop timeline_data = tab.browser.platform.tracing_controller.Stop() File "./tools/perf/../telemetry/telemetry/core/platform/tracing_controller.py", line 32, in Stop return self._tracing_controller_backend.Stop() File "./tools/perf/../telemetry/telemetry/core/platform/tracing_controller_backend.py", line 68, in Stop agent.Stop(trace_data_builder) File "./tools/perf/../telemetry/telemetry/core/platform/tracing_agent/chrome_tracing_agent.py", line 97, in Stop devtools_client.StopChromeTracing(trace_data_builder) File "./tools/perf/../telemetry/telemetry/core/backends/chrome_inspector/devtools_client_backend.py", line 146, in StopChromeTracing backend = context_map.GetInspectorBackend(context_id) File "./tools/perf/../telemetry/telemetry/core/backends/chrome_inspector/devtools_client_backend.py", line 191, in GetInspectorBackend self._app_backend.app, self._devtools_client, context) File "./tools/perf/../telemetry/telemetry/core/backends/chrome_inspector/inspector_backend.py", line 52, in __init__ self._websocket, timeout=timeout) File "./tools/perf/../telemetry/telemetry/core/backends/chrome_inspector/inspector_page.py", line 24, in __init__ self._EnablePageNotifications(timeout=timeout) File "./tools/perf/../telemetry/telemetry/core/backends/chrome_inspector/inspector_page.py", line 81, in _EnablePageNotifications res = self._inspector_websocket.SyncRequest(request, timeout) File "./tools/perf/../telemetry/telemetry/core/backends/chrome_inspector/inspector_websocket.py", line 112, in SyncRequest res = self._Receive(timeout) File "./tools/perf/../telemetry/telemetry/core/backends/chrome_inspector/inspector_websocket.py", line 173, in _Receive self._error_handler(elapsed_time) File "./tools/perf/../telemetry/telemetry/core/backends/chrome_inspector/inspector_backend.py", line 215, in _HandleError 'Elapsed=%ds Error=%s' % (elapsed_time, sys.exc_info()[1])) DevtoolsTargetCrashException: Received a socket error in the browser connection and the tab still exists, assuming it timed out. Elapsed=60s Error=timed out --- In the current implementation, when DevToolsClientBackend.StopChromeTracing() is called, 1. DevToolsClientBackend gets the list of the inspactable contexts using GetUpdatedInspectableContexts() and 2. DevToolsClientBackend tries to connect to the all DevTools agents. In service_worker.service_worker test, there are "service_worker" context in the list of 1. But when DevToolsClientBackend tries to connect to the "service_worker" context, the ServiceWorker may have already terminated or may be terminating. So this causes the test flakiness. I have to fix this flakiness. Could you please let me know what is the better solution for this?
On 2015/02/13 01:39:46, horo wrote: > On 2015/02/12 21:02:45, sullivan wrote: > > > https://codereview.chromium.org/884573004/diff/1/tools/telemetry/telemetry/co... > > File > > > tools/telemetry/telemetry/core/backends/chrome_inspector/devtools_client_backend.py > > (right): > > > > > https://codereview.chromium.org/884573004/diff/1/tools/telemetry/telemetry/co... > > > tools/telemetry/telemetry/core/backends/chrome_inspector/devtools_client_backend.py:110: > > > > On 2015/02/12 20:52:48, nednguyen wrote: > > > Branching the core library just for a single use case like this is not lgtm > at > > > all. > > > > Agreed we need to handle this better. > > > > Is the error you're trying to fix still the same callstack as in > > https://code.google.com/p/chromium/issues/detail?id=433943#c19 ? > > No. This failure was caused by speedindex. > And I think it is fixed by https://codereview.chromium.org/805393003. > > The error that I want to fix is this: > --- > horo@horo:~/chromium/src$ ./tools/perf/run_benchmark > --browser-executable=~/chromium/src/out/Debug/chrome --also-run-disabled-tests > service_worker.service_worker > [ RUN ] https://jakearchibald.github.io/trained-to-thrill/ > WARNING:root:Could not find Flash at > ./third_party/adobe/flash/binaries/ppapi/linux/libpepflashplayer.so. Continuing > without Flash. > To run with Flash, check it out via http://go/read-src-internal > Traceback (most recent call last): > File "./tools/perf/../telemetry/telemetry/user_story/user_story_runner.py", > line 100, in _RunUserStoryAndProcessErrorIfNeeded > state.RunUserStory(results) > File "./tools/perf/../telemetry/telemetry/page/shared_page_state.py", line > 242, in RunUserStory > self._test.RunPage(self._current_page, self._current_tab, results) > File "./tools/perf/../telemetry/telemetry/page/page_test.py", line 239, in > RunPage > self.ValidateAndMeasurePage(page, tab, results) > File > "/usr/local/google/home/horo/chromium/src/tools/perf/benchmarks/service_worker.py", > line 107, in ValidateAndMeasurePage > self._timeline_controller.Stop(tab, results) > File > "/usr/local/google/home/horo/chromium/src/tools/perf/measurements/timeline_controller.py", > line 53, in Stop > timeline_data = tab.browser.platform.tracing_controller.Stop() > File > "./tools/perf/../telemetry/telemetry/core/platform/tracing_controller.py", line > 32, in Stop > return self._tracing_controller_backend.Stop() > File > "./tools/perf/../telemetry/telemetry/core/platform/tracing_controller_backend.py", > line 68, in Stop > agent.Stop(trace_data_builder) > File > "./tools/perf/../telemetry/telemetry/core/platform/tracing_agent/chrome_tracing_agent.py", > line 97, in Stop > devtools_client.StopChromeTracing(trace_data_builder) > File > "./tools/perf/../telemetry/telemetry/core/backends/chrome_inspector/devtools_client_backend.py", > line 146, in StopChromeTracing > backend = context_map.GetInspectorBackend(context_id) > File > "./tools/perf/../telemetry/telemetry/core/backends/chrome_inspector/devtools_client_backend.py", > line 191, in GetInspectorBackend > self._app_backend.app, self._devtools_client, context) > File > "./tools/perf/../telemetry/telemetry/core/backends/chrome_inspector/inspector_backend.py", > line 52, in __init__ > self._websocket, timeout=timeout) > File > "./tools/perf/../telemetry/telemetry/core/backends/chrome_inspector/inspector_page.py", > line 24, in __init__ > self._EnablePageNotifications(timeout=timeout) > File > "./tools/perf/../telemetry/telemetry/core/backends/chrome_inspector/inspector_page.py", > line 81, in _EnablePageNotifications > res = self._inspector_websocket.SyncRequest(request, timeout) > File > "./tools/perf/../telemetry/telemetry/core/backends/chrome_inspector/inspector_websocket.py", > line 112, in SyncRequest > res = self._Receive(timeout) > File > "./tools/perf/../telemetry/telemetry/core/backends/chrome_inspector/inspector_websocket.py", > line 173, in _Receive > self._error_handler(elapsed_time) > File > "./tools/perf/../telemetry/telemetry/core/backends/chrome_inspector/inspector_backend.py", > line 215, in _HandleError > 'Elapsed=%ds Error=%s' % (elapsed_time, sys.exc_info()[1])) > DevtoolsTargetCrashException: Received a socket error in the browser connection > and the tab still exists, assuming it timed out. Elapsed=60s Error=timed out > --- > In the current implementation, when DevToolsClientBackend.StopChromeTracing() is > called, > 1. DevToolsClientBackend gets the list of the inspactable contexts using > GetUpdatedInspectableContexts() > and > 2. DevToolsClientBackend tries to connect to the all DevTools agents. > > In service_worker.service_worker test, there are "service_worker" context in the > list of 1. > But when DevToolsClientBackend tries to connect to the "service_worker" context, > the ServiceWorker may have already terminated or may be terminating. > So this causes the test flakiness. > > I have to fix this flakiness. > Could you please let me know what is the better solution for this? Can you show me the stack trace? Also, why does service_worker context terminated?
On 2015/02/13 02:11:39, nednguyen wrote: > On 2015/02/13 01:39:46, horo wrote: > > On 2015/02/12 21:02:45, sullivan wrote: > > > > > > https://codereview.chromium.org/884573004/diff/1/tools/telemetry/telemetry/co... > > > File > > > > > > tools/telemetry/telemetry/core/backends/chrome_inspector/devtools_client_backend.py > > > (right): > > > > > > > > > https://codereview.chromium.org/884573004/diff/1/tools/telemetry/telemetry/co... > > > > > > tools/telemetry/telemetry/core/backends/chrome_inspector/devtools_client_backend.py:110: > > > > > > On 2015/02/12 20:52:48, nednguyen wrote: > > > > Branching the core library just for a single use case like this is not > lgtm > > at > > > > all. > > > > > > Agreed we need to handle this better. > > > > > > Is the error you're trying to fix still the same callstack as in > > > https://code.google.com/p/chromium/issues/detail?id=433943#c19 ? > > > > No. This failure was caused by speedindex. > > And I think it is fixed by https://codereview.chromium.org/805393003. > > > > The error that I want to fix is this: > > --- > > horo@horo:~/chromium/src$ ./tools/perf/run_benchmark > > --browser-executable=~/chromium/src/out/Debug/chrome --also-run-disabled-tests > > service_worker.service_worker > > [ RUN ] https://jakearchibald.github.io/trained-to-thrill/ > > WARNING:root:Could not find Flash at > > ./third_party/adobe/flash/binaries/ppapi/linux/libpepflashplayer.so. > Continuing > > without Flash. > > To run with Flash, check it out via http://go/read-src-internal > > Traceback (most recent call last): > > File "./tools/perf/../telemetry/telemetry/user_story/user_story_runner.py", > > line 100, in _RunUserStoryAndProcessErrorIfNeeded > > state.RunUserStory(results) > > File "./tools/perf/../telemetry/telemetry/page/shared_page_state.py", line > > 242, in RunUserStory > > self._test.RunPage(self._current_page, self._current_tab, results) > > File "./tools/perf/../telemetry/telemetry/page/page_test.py", line 239, in > > RunPage > > self.ValidateAndMeasurePage(page, tab, results) > > File > > > "/usr/local/google/home/horo/chromium/src/tools/perf/benchmarks/service_worker.py", > > line 107, in ValidateAndMeasurePage > > self._timeline_controller.Stop(tab, results) > > File > > > "/usr/local/google/home/horo/chromium/src/tools/perf/measurements/timeline_controller.py", > > line 53, in Stop > > timeline_data = tab.browser.platform.tracing_controller.Stop() > > File > > "./tools/perf/../telemetry/telemetry/core/platform/tracing_controller.py", > line > > 32, in Stop > > return self._tracing_controller_backend.Stop() > > File > > > "./tools/perf/../telemetry/telemetry/core/platform/tracing_controller_backend.py", > > line 68, in Stop > > agent.Stop(trace_data_builder) > > File > > > "./tools/perf/../telemetry/telemetry/core/platform/tracing_agent/chrome_tracing_agent.py", > > line 97, in Stop > > devtools_client.StopChromeTracing(trace_data_builder) > > File > > > "./tools/perf/../telemetry/telemetry/core/backends/chrome_inspector/devtools_client_backend.py", > > line 146, in StopChromeTracing > > backend = context_map.GetInspectorBackend(context_id) > > File > > > "./tools/perf/../telemetry/telemetry/core/backends/chrome_inspector/devtools_client_backend.py", > > line 191, in GetInspectorBackend > > self._app_backend.app, self._devtools_client, context) > > File > > > "./tools/perf/../telemetry/telemetry/core/backends/chrome_inspector/inspector_backend.py", > > line 52, in __init__ > > self._websocket, timeout=timeout) > > File > > > "./tools/perf/../telemetry/telemetry/core/backends/chrome_inspector/inspector_page.py", > > line 24, in __init__ > > self._EnablePageNotifications(timeout=timeout) > > File > > > "./tools/perf/../telemetry/telemetry/core/backends/chrome_inspector/inspector_page.py", > > line 81, in _EnablePageNotifications > > res = self._inspector_websocket.SyncRequest(request, timeout) > > File > > > "./tools/perf/../telemetry/telemetry/core/backends/chrome_inspector/inspector_websocket.py", > > line 112, in SyncRequest > > res = self._Receive(timeout) > > File > > > "./tools/perf/../telemetry/telemetry/core/backends/chrome_inspector/inspector_websocket.py", > > line 173, in _Receive > > self._error_handler(elapsed_time) > > File > > > "./tools/perf/../telemetry/telemetry/core/backends/chrome_inspector/inspector_backend.py", > > line 215, in _HandleError > > 'Elapsed=%ds Error=%s' % (elapsed_time, sys.exc_info()[1])) > > DevtoolsTargetCrashException: Received a socket error in the browser > connection > > and the tab still exists, assuming it timed out. Elapsed=60s Error=timed out > > --- > > In the current implementation, when DevToolsClientBackend.StopChromeTracing() > is > > called, > > 1. DevToolsClientBackend gets the list of the inspactable contexts using > > GetUpdatedInspectableContexts() > > and > > 2. DevToolsClientBackend tries to connect to the all DevTools agents. > > > > In service_worker.service_worker test, there are "service_worker" context in > the > > list of 1. > > But when DevToolsClientBackend tries to connect to the "service_worker" > context, > > the ServiceWorker may have already terminated or may be terminating. > > So this causes the test flakiness. > > > > I have to fix this flakiness. > > Could you please let me know what is the better solution for this? > > Can you show me the stack trace? Also, why does service_worker context > terminated? Nvm, I show the stack trace. Can you just answer the 2nd question?
> why does service_worker context terminated? ServiceWorker could be terminated in many reasons. 1. The procedure of updating the script is finished. 2. An error occurred while loading or evaluating the script. 3. 30 seconds have passed without any event.
chrishenry@google.com changed reviewers: - chrishenry@google.com
On 2015/02/13 03:40:37, horo wrote: > > why does service_worker context terminated? > > ServiceWorker could be terminated in many reasons. > 1. The procedure of updating the script is finished. > 2. An error occurred while loading or evaluating the script. > 3. 30 seconds have passed without any event. So is the flakiness cause by service_worker context terminated in between GetUpdatedInspectableContexts() call & context_map.GetInspectorBackend()? I think there are two possible fixes here: 1) Catch the exception and ignore the contexts that have been stale. 2) Whitelisting the contexts in to be evaluated in StopChromeTracing() rather than blacklisting them in ListInspectableContexts(). The StopChromeTracing iterates the list of contexts to make sure that console.time & console.timeEnd aren't modified by pages. I think we can do the check on 'page' type only.
> So is the flakiness cause by service_worker context terminated in between > GetUpdatedInspectableContexts() call & context_map.GetInspectorBackend()? Yes. > I think there are two possible fixes here: I like 2). I updated this CL. Please review it again. Thank you.
Can you add unittest for StopChromeTracing. i.e: create a page that modify console.time/console.timeEnd & assert that exception is raised. https://codereview.chromium.org/884573004/diff/20001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/backends/chrome_inspector/devtools_client_backend.py (right): https://codereview.chromium.org/884573004/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome_inspector/devtools_client_backend.py:137: if context['type'] != 'page': For some reason, i can't find any documentation on devtool that mentions these types. Could you please point me to one of them? :-)
> Can you add unittest for StopChromeTracing. i.e: create a page that modify > console.time/console.timeEnd & assert that exception is raised. Sorry. I am not familiar with the telemetry scripts. I don't know how to create a page for telemetry unit tests. Could you please point out some of existing unit tests that are similar to what you're offering? https://codereview.chromium.org/884573004/diff/20001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/backends/chrome_inspector/devtools_client_backend.py (right): https://codereview.chromium.org/884573004/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome_inspector/devtools_client_backend.py:137: if context['type'] != 'page': On 2015/02/16 15:56:16, nednguyen wrote: > For some reason, i can't find any documentation on devtool that mentions these > types. Could you please point me to one of them? :-) The types of DevToolsTarget are defined in devtools_target_impl.cc. const char DevToolsTargetImpl::kTargetTypeApp[] = "app"; const char DevToolsTargetImpl::kTargetTypeBackgroundPage[] = "background_page"; const char DevToolsTargetImpl::kTargetTypePage[] = "page"; const char DevToolsTargetImpl::kTargetTypeWorker[] = "worker"; const char DevToolsTargetImpl::kTargetTypeWebView[] = "webview"; const char DevToolsTargetImpl::kTargetTypeIFrame[] = "iframe"; const char DevToolsTargetImpl::kTargetTypeOther[] = "other"; const char DevToolsTargetImpl::kTargetTypeServiceWorker[] = "service_worker";
On 2015/02/16 17:34:27, horo wrote: > > Can you add unittest for StopChromeTracing. i.e: create a page that modify > > console.time/console.timeEnd & assert that exception is raised. > > Sorry. > I am not familiar with the telemetry scripts. > I don't know how to create a page for telemetry unit tests. > Could you please point out some of existing unit tests that are similar to what > you're offering? There are example unittest in tracing_backend_unittest already. If you want to create a test page, you can put them in unittest_data: https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/un... Example references here: https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te... > > https://codereview.chromium.org/884573004/diff/20001/tools/telemetry/telemetr... > File > tools/telemetry/telemetry/core/backends/chrome_inspector/devtools_client_backend.py > (right): > > https://codereview.chromium.org/884573004/diff/20001/tools/telemetry/telemetr... > tools/telemetry/telemetry/core/backends/chrome_inspector/devtools_client_backend.py:137: > if context['type'] != 'page': > On 2015/02/16 15:56:16, nednguyen wrote: > > For some reason, i can't find any documentation on devtool that mentions these > > types. Could you please point me to one of them? :-) > > The types of DevToolsTarget are defined in devtools_target_impl.cc. Thanks for the pointer. > > > const char DevToolsTargetImpl::kTargetTypeApp[] = "app"; > const char DevToolsTargetImpl::kTargetTypeBackgroundPage[] = "background_page"; > const char DevToolsTargetImpl::kTargetTypePage[] = "page"; > const char DevToolsTargetImpl::kTargetTypeWorker[] = "worker"; > const char DevToolsTargetImpl::kTargetTypeWebView[] = "webview"; > const char DevToolsTargetImpl::kTargetTypeIFrame[] = "iframe"; > const char DevToolsTargetImpl::kTargetTypeOther[] = "other"; > const char DevToolsTargetImpl::kTargetTypeServiceWorker[] = "service_worker";
https://codereview.chromium.org/884573004/diff/20001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/backends/chrome_inspector/devtools_client_backend.py (right): https://codereview.chromium.org/884573004/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome_inspector/devtools_client_backend.py:137: if context['type'] != 'page': On 2015/02/16 17:34:27, horo wrote: > On 2015/02/16 15:56:16, nednguyen wrote: > > For some reason, i can't find any documentation on devtool that mentions these > > types. Could you please point me to one of them? :-) > > The types of DevToolsTarget are defined in devtools_target_impl.cc. > > > const char DevToolsTargetImpl::kTargetTypeApp[] = "app"; > const char DevToolsTargetImpl::kTargetTypeBackgroundPage[] = "background_page"; > const char DevToolsTargetImpl::kTargetTypePage[] = "page"; > const char DevToolsTargetImpl::kTargetTypeWorker[] = "worker"; > const char DevToolsTargetImpl::kTargetTypeWebView[] = "webview"; > const char DevToolsTargetImpl::kTargetTypeIFrame[] = "iframe"; > const char DevToolsTargetImpl::kTargetTypeOther[] = "other"; > const char DevToolsTargetImpl::kTargetTypeServiceWorker[] = "service_worker"; Looks like this should also include 'iframe', 'webview'. What does 'app' mean?
> There are example unittest in tracing_backend_unittest already. Thank you. I added tracing_controller_unittest.py https://codereview.chromium.org/884573004/diff/20001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/backends/chrome_inspector/devtools_client_backend.py (right): https://codereview.chromium.org/884573004/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome_inspector/devtools_client_backend.py:137: if context['type'] != 'page': On 2015/02/16 19:18:34, nednguyen wrote: > On 2015/02/16 17:34:27, horo wrote: > > On 2015/02/16 15:56:16, nednguyen wrote: > > > For some reason, i can't find any documentation on devtool that mentions > these > > > types. Could you please point me to one of them? :-) > > > > The types of DevToolsTarget are defined in devtools_target_impl.cc. > > > > > > const char DevToolsTargetImpl::kTargetTypeApp[] = "app"; > > const char DevToolsTargetImpl::kTargetTypeBackgroundPage[] = > "background_page"; > > const char DevToolsTargetImpl::kTargetTypePage[] = "page"; > > const char DevToolsTargetImpl::kTargetTypeWorker[] = "worker"; > > const char DevToolsTargetImpl::kTargetTypeWebView[] = "webview"; > > const char DevToolsTargetImpl::kTargetTypeIFrame[] = "iframe"; > > const char DevToolsTargetImpl::kTargetTypeOther[] = "other"; > > const char DevToolsTargetImpl::kTargetTypeServiceWorker[] = "service_worker"; > > Looks like this should also include 'iframe', 'webview'. What does 'app' mean? Done. 'app' is for Chrome Apps. https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/dev... I don't know well about what type of DevToolsTarget should be supported by telemetry tests and should be added to trace_data as TAB_ID_PART. Should Chrome Apps and Chrome Extensions and Webview be supported?
On 2015/02/17 01:21:02, horo wrote: > > There are example unittest in tracing_backend_unittest already. > > Thank you. > I added tracing_controller_unittest.py > > https://codereview.chromium.org/884573004/diff/20001/tools/telemetry/telemetr... > File > tools/telemetry/telemetry/core/backends/chrome_inspector/devtools_client_backend.py > (right): > > https://codereview.chromium.org/884573004/diff/20001/tools/telemetry/telemetr... > tools/telemetry/telemetry/core/backends/chrome_inspector/devtools_client_backend.py:137: > if context['type'] != 'page': > On 2015/02/16 19:18:34, nednguyen wrote: > > On 2015/02/16 17:34:27, horo wrote: > > > On 2015/02/16 15:56:16, nednguyen wrote: > > > > For some reason, i can't find any documentation on devtool that mentions > > these > > > > types. Could you please point me to one of them? :-) > > > > > > The types of DevToolsTarget are defined in devtools_target_impl.cc. > > > > > > > > > const char DevToolsTargetImpl::kTargetTypeApp[] = "app"; > > > const char DevToolsTargetImpl::kTargetTypeBackgroundPage[] = > > "background_page"; > > > const char DevToolsTargetImpl::kTargetTypePage[] = "page"; > > > const char DevToolsTargetImpl::kTargetTypeWorker[] = "worker"; > > > const char DevToolsTargetImpl::kTargetTypeWebView[] = "webview"; > > > const char DevToolsTargetImpl::kTargetTypeIFrame[] = "iframe"; > > > const char DevToolsTargetImpl::kTargetTypeOther[] = "other"; > > > const char DevToolsTargetImpl::kTargetTypeServiceWorker[] = > "service_worker"; > > > > Looks like this should also include 'iframe', 'webview'. What does 'app' mean? > > Done. > 'app' is for Chrome Apps. > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/dev... > > I don't know well about what type of DevToolsTarget should be supported by > telemetry tests and should be added to trace_data as TAB_ID_PART. > Should Chrome Apps and Chrome Extensions and Webview be supported? Not sure about Chrome APps & Chrome Extensions but Webview is supported (https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te...)
nednguyen@google.com changed reviewers: + achuith@google.com
+achuith on questions about Chrome App & Chrome extensions tracing support. lgtm on my part but please wait for achuith.
https://codereview.chromium.org/884573004/diff/40001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/platform/tracing_controller_unittest.py (right): https://codereview.chromium.org/884573004/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/platform/tracing_controller_unittest.py:10: def testModifiedConsoleTime(self): Looking at the win_chromium_rel_ng's failure, I think you may need to add @decorators.Isolated here because telemetry unittest are run in parallel & start/stop tracing method are global.
New patchsets have been uploaded after l-g-t-m from nednguyen@google.com
Thank you very much for reviewing! https://codereview.chromium.org/884573004/diff/40001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/platform/tracing_controller_unittest.py (right): https://codereview.chromium.org/884573004/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/platform/tracing_controller_unittest.py:10: def testModifiedConsoleTime(self): On 2015/02/17 04:29:20, nednguyen wrote: > Looking at the win_chromium_rel_ng's failure, I think you may need to add > @decorators.Isolated here because telemetry unittest are run in parallel & > start/stop tracing method are global. Done.
On 2015/02/17 05:18:36, horo wrote: > Thank you very much for reviewing! > > https://codereview.chromium.org/884573004/diff/40001/tools/telemetry/telemetr... > File tools/telemetry/telemetry/core/platform/tracing_controller_unittest.py > (right): > > https://codereview.chromium.org/884573004/diff/40001/tools/telemetry/telemetr... > tools/telemetry/telemetry/core/platform/tracing_controller_unittest.py:10: def > testModifiedConsoleTime(self): > On 2015/02/17 04:29:20, nednguyen wrote: > > Looking at the win_chromium_rel_ng's failure, I think you may need to add > > @decorators.Isolated here because telemetry unittest are run in parallel & > > start/stop tracing method are global. > > Done. Now I am really frustrated by the failure on the win bot again (win_chromium_rel_ng -> win_chromium_x64_rel_ng). You clearly stopped tracing testModifiedConsoleTime. Maybe the implementation of tracing_controller_backend & chrome_tracing_agent is buggy? :-( Do you want to add some logging info into chrome_tracing_agent & tracing_controller_backend to figure out what is going on there? Several theory: + tracing_controller_backend fails to call stop tracing on ChromeTracingAgent. + ChromeTracingAgent messes up the _is_tracing_running_for_platform_backend map.
On 2015/02/17 17:00:27, nednguyen wrote: > On 2015/02/17 05:18:36, horo wrote: > > Thank you very much for reviewing! > > > > > https://codereview.chromium.org/884573004/diff/40001/tools/telemetry/telemetr... > > File tools/telemetry/telemetry/core/platform/tracing_controller_unittest.py > > (right): > > > > > https://codereview.chromium.org/884573004/diff/40001/tools/telemetry/telemetr... > > tools/telemetry/telemetry/core/platform/tracing_controller_unittest.py:10: def > > testModifiedConsoleTime(self): > > On 2015/02/17 04:29:20, nednguyen wrote: > > > Looking at the win_chromium_rel_ng's failure, I think you may need to add > > > @decorators.Isolated here because telemetry unittest are run in parallel & > > > start/stop tracing method are global. > > > > Done. > > Now I am really frustrated by the failure on the win bot again > (win_chromium_rel_ng -> win_chromium_x64_rel_ng). You clearly stopped tracing > testModifiedConsoleTime. Maybe the implementation of tracing_controller_backend > & chrome_tracing_agent is buggy? :-( > > Do you want to add some logging info into chrome_tracing_agent & > tracing_controller_backend to figure out what is going on there? > Several theory: > + tracing_controller_backend fails to call stop tracing on ChromeTracingAgent. > + ChromeTracingAgent messes up the _is_tracing_running_for_platform_backend map. Currently telemetry platform is a singleton. (http://crrev.com/436873003) telemetry.core.platform.GetHostPlatform() is using the global variable "_host_platform". This makes it difficult (impossible?) to write the unittest which checks the thrown exception with "typ" library. Typ library reuses the subprocess even if it is created for the isolated test. Typ library retries the failed tests after the isolated tests are executed. But after Exception('Page stomped on console.time') is raised in DevToolsClientBackend.StopChromeTracing, ChromeTracingAgent._is_active will be never updated. So every retried tests will fail. Is it OK to remove devtools_client_backend.py from this CL for now? I want to commit this CL and re-enable the ServiceWorker telemetry tests as soon as possible.
On 2015/02/18 09:39:21, horo wrote: > On 2015/02/17 17:00:27, nednguyen wrote: > > On 2015/02/17 05:18:36, horo wrote: > > > Thank you very much for reviewing! > > > > > > > > > https://codereview.chromium.org/884573004/diff/40001/tools/telemetry/telemetr... > > > File tools/telemetry/telemetry/core/platform/tracing_controller_unittest.py > > > (right): > > > > > > > > > https://codereview.chromium.org/884573004/diff/40001/tools/telemetry/telemetr... > > > tools/telemetry/telemetry/core/platform/tracing_controller_unittest.py:10: > def > > > testModifiedConsoleTime(self): > > > On 2015/02/17 04:29:20, nednguyen wrote: > > > > Looking at the win_chromium_rel_ng's failure, I think you may need to add > > > > @decorators.Isolated here because telemetry unittest are run in parallel & > > > > start/stop tracing method are global. > > > > > > Done. > > > > Now I am really frustrated by the failure on the win bot again > > (win_chromium_rel_ng -> win_chromium_x64_rel_ng). You clearly stopped tracing > > testModifiedConsoleTime. Maybe the implementation of > tracing_controller_backend > > & chrome_tracing_agent is buggy? :-( > > > > Do you want to add some logging info into chrome_tracing_agent & > > tracing_controller_backend to figure out what is going on there? > > Several theory: > > + tracing_controller_backend fails to call stop tracing on ChromeTracingAgent. > > + ChromeTracingAgent messes up the _is_tracing_running_for_platform_backend > map. > > Currently telemetry platform is a singleton. (http://crrev.com/436873003) > telemetry.core.platform.GetHostPlatform() is using the global variable > "_host_platform". > This makes it difficult (impossible?) to write the unittest which checks the > thrown exception with "typ" library. > > Typ library reuses the subprocess even if it is created for the isolated test. > Typ library retries the failed tests after the isolated tests are executed. > But after Exception('Page stomped on console.time') is raised in > DevToolsClientBackend.StopChromeTracing, ChromeTracingAgent._is_active will be > never updated. > So every retried tests will fail. Oh crap, I forgot about this. Thanks for the clear explanation :-) > > Is it OK to remove devtools_client_backend.py from this CL for now? > I want to commit this CL and re-enable the ServiceWorker telemetry tests as soon > as possible. You can add @decorators.Disabled to the unittest for now. After that, please file a bug to enable this unittest and assign it to me :-)
On 2015/02/18 16:01:12, nednguyen wrote: > On 2015/02/18 09:39:21, horo wrote: > > On 2015/02/17 17:00:27, nednguyen wrote: > > > On 2015/02/17 05:18:36, horo wrote: > > > > Thank you very much for reviewing! > > > > > > > > > > > > > > https://codereview.chromium.org/884573004/diff/40001/tools/telemetry/telemetr... > > > > File > tools/telemetry/telemetry/core/platform/tracing_controller_unittest.py > > > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/884573004/diff/40001/tools/telemetry/telemetr... > > > > tools/telemetry/telemetry/core/platform/tracing_controller_unittest.py:10: > > def > > > > testModifiedConsoleTime(self): > > > > On 2015/02/17 04:29:20, nednguyen wrote: > > > > > Looking at the win_chromium_rel_ng's failure, I think you may need to > add > > > > > @decorators.Isolated here because telemetry unittest are run in parallel > & > > > > > start/stop tracing method are global. > > > > > > > > Done. > > > > > > Now I am really frustrated by the failure on the win bot again > > > (win_chromium_rel_ng -> win_chromium_x64_rel_ng). You clearly stopped > tracing > > > testModifiedConsoleTime. Maybe the implementation of > > tracing_controller_backend > > > & chrome_tracing_agent is buggy? :-( > > > > > > Do you want to add some logging info into chrome_tracing_agent & > > > tracing_controller_backend to figure out what is going on there? > > > Several theory: > > > + tracing_controller_backend fails to call stop tracing on > ChromeTracingAgent. > > > + ChromeTracingAgent messes up the _is_tracing_running_for_platform_backend > > map. > > > > Currently telemetry platform is a singleton. (http://crrev.com/436873003) > > telemetry.core.platform.GetHostPlatform() is using the global variable > > "_host_platform". > > This makes it difficult (impossible?) to write the unittest which checks the > > thrown exception with "typ" library. > > > > Typ library reuses the subprocess even if it is created for the isolated test. > > Typ library retries the failed tests after the isolated tests are executed. > > But after Exception('Page stomped on console.time') is raised in > > DevToolsClientBackend.StopChromeTracing, ChromeTracingAgent._is_active will be > > never updated. > > So every retried tests will fail. > > Oh crap, I forgot about this. Thanks for the clear explanation :-) > > > > > Is it OK to remove devtools_client_backend.py from this CL for now? > > I want to commit this CL and re-enable the ServiceWorker telemetry tests as > soon > > as possible. > > You can add @decorators.Disabled to the unittest for now. After that, please > file a bug to enable this unittest and assign it to me :-) Done. http://crbug.com/459807
horo@chromium.org changed reviewers: + achuith@chromium.org
achuith@ Does telemetry test support Chrome Apps & Chrome Extensions? If so I have to add "app" and "background_page" in the white list. https://codereview.chromium.org/884573004/#msg18 Thank you!
On 2015/02/19 00:31:11, horo wrote: > achuith@ > > Does telemetry test support Chrome Apps & Chrome Extensions? > If so I have to add "app" and "background_page" in the white list. > https://codereview.chromium.org/884573004/#msg18 > > > Thank you! achuithb@ Ping?
On 2015/02/23 04:59:17, horo wrote: > On 2015/02/19 00:31:11, horo wrote: > > achuith@ > > > > Does telemetry test support Chrome Apps & Chrome Extensions? > > If so I have to add "app" and "background_page" in the white list. > > https://codereview.chromium.org/884573004/#msg18 > > > > > > Thank you! > > achuithb@ > Ping? +lafeenstra@. Because of the high turn around time, I would want horo@ to be able to land this patch even without testing this on cros. In case cros test break, we won't revert later because that is even longer turnaround time.
On 2015/02/23 16:29:05, nednguyen wrote: > On 2015/02/23 04:59:17, horo wrote: > > On 2015/02/19 00:31:11, horo wrote: > > > achuith@ > > > > > > Does telemetry test support Chrome Apps & Chrome Extensions? > > > If so I have to add "app" and "background_page" in the white list. > > > https://codereview.chromium.org/884573004/#msg18 > > > > > > > > > Thank you! > > > > achuithb@ > > Ping? > > +lafeenstra@. > Because of the high turn around time, I would want horo@ to be able to land this > patch even without testing this on cros. In case cros test break, we won't > revert later because that is even longer turnaround time. Sorry, not sure how I missed this. I think this should be lgtm. There is telemetry support for extensions/apps, but it's not used for any trace-related perf tests.
On 2015/02/24 00:23:02, achuithb wrote: > On 2015/02/23 16:29:05, nednguyen wrote: > > On 2015/02/23 04:59:17, horo wrote: > > > On 2015/02/19 00:31:11, horo wrote: > > > > achuith@ > > > > > > > > Does telemetry test support Chrome Apps & Chrome Extensions? > > > > If so I have to add "app" and "background_page" in the white list. > > > > https://codereview.chromium.org/884573004/#msg18 > > > > > > > > > > > > Thank you! > > > > > > achuithb@ > > > Ping? > > > > +lafeenstra@. > > Because of the high turn around time, I would want horo@ to be able to land > this > > patch even without testing this on cros. In case cros test break, we won't > > revert later because that is even longer turnaround time. > > Sorry, not sure how I missed this. I think this should be lgtm. There is > telemetry support for extensions/apps, but it's not used for any trace-related > perf tests. Thank you!
The CQ bit was checked by horo@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/884573004/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/c8ad009e818cc08a36d5f6a4fe68c97ee3f88179 Cr-Commit-Position: refs/heads/master@{#317730} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
