Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10736055:
Smoke test for tracing infrastructure in PyAuto (Closed)
|
Created:
8 years, 5 months ago by Russ Harmon Modified:
8 years, 4 months ago CC:
Sonny, chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionIntroduce an API and associated smoke test for writing PyAuto tests which collect and analyze trace events.
BUG=137356
TEST=manually
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=149345
Patch Set 1 #
Total comments: 15
Patch Set 2 : Example pyauto test for getting trace events. #Patch Set 3 : Example pyauto test for getting trace events. #Patch Set 4 : Example pyauto test for getting trace events. #Patch Set 5 : Smoke test for tracing infrastructure in PyAuto #Patch Set 6 : Smoke test for tracing infrastructure in PyAuto #
Total comments: 24
Patch Set 7 : Introduce a decorator around the TraceModel which has all the support code. #Patch Set 8 : Use argument slices rather than explicit argument lists where appropriate. #Patch Set 9 : Use argument slices rather than explicit argument lists where appropriate. #
Total comments: 13
Patch Set 10 : Enable multiple traces per test. #Patch Set 11 : Rename the last few variables from decorator to shim. #Patch Set 12 : Remove changes to pyauto.py which I didn't mean to put here. #Patch Set 13 : Minor cleanup #Patch Set 14 : Fix a bug preventing you from using one tracer to perform multiple traces. #Patch Set 15 : Minor cleanup. #Patch Set 16 : Clean up js TimelineModel when the python TimelineModelProxy is deleted. #
Total comments: 6
Patch Set 17 : Fix up style issues. Add suite to PYAUTO_TESTS #
Total comments: 15
Patch Set 18 : Add more docstrings, fixup more style issues. #Patch Set 19 : Add more docstrings. #
Total comments: 32
Patch Set 20 : Fix more style issues. #Patch Set 21 : More style fixes. #Patch Set 22 : Add an explicit test for the validity of a TimelineModel after it's producing Tracer is deleted. #Patch Set 23 : Remove unused unittest import. #
Total comments: 6
Patch Set 24 : Fix more stylistic issues. #Messages
Total messages: 27 (0 generated)
Please disregard. This is a work in progress.
a http://codereview.chromium.org/10736055/diff/1/chrome/test/functional/tracing.py File chrome/test/functional/tracing.py (right): http://codereview.chromium.org/10736055/diff/1/chrome/test/functional/tracing... chrome/test/functional/tracing.py:14: class TimelineModel: TimelineModelProxy http://codereview.chromium.org/10736055/diff/1/chrome/test/functional/tracing... chrome/test/functional/tracing.py:15: def __init__(self, test): test -> automationController? So you're initializing this with the assumption that the model is remotely stored in profilingView. Its cleaner to separate these concerns up front: class TimelineModelProxy: def __init__(self, automationController, remoteObjectName): self._remoteObjectName = remoteObjectName; self._automationController = automationController; # Alias TimelineModel to TimelineModelProxy for convenience TimelineModel = TimelineModelProxy http://codereview.chromium.org/10736055/diff/1/chrome/test/functional/tracing... chrome/test/functional/tracing.py:20: def __js_call__(self, name, *args): python convention say def _js_call(self, name, *args) sanity says name -> method_name http://codereview.chromium.org/10736055/diff/1/chrome/test/functional/tracing... chrome/test/functional/tracing.py:23: var res = self['%s'].apply(self, JSON.parse('%s')); why [%s]? var res = %s.%s(%s)" % (self._remoteObjectName, methodName, ",".join([stringifyForJavascript(x) for x in args])) http://codereview.chromium.org/10736055/diff/1/chrome/test/functional/tracing... chrome/test/functional/tracing.py:25: JSON.stringify(JSON.stringify(res)) Probably worth setting up some basic error handling here so things dont explode mysteriously if on you. try { var res = model.method(args); sendstringified({success: res); } catch(e) { stringify e send failure } and, on the python side, if you get fail throw Exception(js error); To stringify a javascript exception you need to handle these three different exception "flavors": if (typeof(e) == "string") { return e; } else if(e.stack) { return e.stack; } else { return e.message; } http://codereview.chromium.org/10736055/diff/1/chrome/test/functional/tracing... chrome/test/functional/tracing.py:29: string.replace(name, "'", "\\'"), make this a method called escapeForJavascriptExecution so that you can deal with complications that arise later.... and so you can do the right thing to method args, as noted above. :) http://codereview.chromium.org/10736055/diff/1/chrome/test/functional/tracing... chrome/test/functional/tracing.py:34: return self.__js_call__(inspect.stack()[0][3]); Too magic. Hard code it. Also, does PyAuto not follow google python style guide? http://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Naming get_all_threads is what style guide say http://codereview.chromium.org/10736055/diff/1/chrome/test/functional/tracing... chrome/test/functional/tracing.py:50: self.NavigateToURL('chrome://tracing', self.trace_win) do we know if NavigateToURL waits for the DOMContentLoaded event? E.g. are we sure that executejavascript runs with the tracing scripts already loaded? http://codereview.chromium.org/10736055/diff/1/chrome/test/functional/tracing... chrome/test/functional/tracing.py:52: tracingController.addEventListener("traceEnded", function() { single quotes in javascript http://codereview.chromium.org/10736055/diff/1/chrome/test/functional/tracing... chrome/test/functional/tracing.py:53: window.domAutomationController.send(""); So if you pair this with the codereview I just posted (http://codereview.appspot.com/6343106/) you should be able to do the following from here: window.pyauto_timeline_model = new TimleineModel(); var events = [tracingController.traceData]; if (tracingController.supportsSystemTracing) events.push(tracingControler.systemTraceData); window.pyauto_timeline_model.importTraces(events); Then, you can say this.model = new TimeineModelProxy(self, "window.pyauto_timeline_model") http://codereview.chromium.org/10736055/diff/1/chrome/test/functional/tracing... chrome/test/functional/tracing.py:60: self.trace_win = None presumably this is _trace_win since you probably dont want folks subclassing this test to use that member http://codereview.chromium.org/10736055/diff/1/chrome/test/functional/tracing... chrome/test/functional/tracing.py:75: return TimelineModel(self); What in the test does this cause us to wait on the send inside traceEnded?
http://codereview.chromium.org/10736055/diff/1/chrome/test/functional/tracing.py File chrome/test/functional/tracing.py (right): http://codereview.chromium.org/10736055/diff/1/chrome/test/functional/tracing... chrome/test/functional/tracing.py:9: import pyauto Lets split this into timeline_model.pyand tracing_smoke_test.py Tracing smoke test should just record a trace and verify that there is at least one process named CrBrowserMain. Sg?
http://codereview.chromium.org/10736055/diff/1/chrome/test/functional/tracing.py File chrome/test/functional/tracing.py (right): http://codereview.chromium.org/10736055/diff/1/chrome/test/functional/tracing... chrome/test/functional/tracing.py:50: self.NavigateToURL('chrome://tracing', self.trace_win) On 2012/07/13 09:47:56, nduca wrote: > do we know if NavigateToURL waits for the DOMContentLoaded event? E.g. are we > sure that executejavascript runs with the tracing scripts already loaded? Other tests do it this way, so I'd assume yes. http://codereview.chromium.org/10736055/diff/1/chrome/test/functional/tracing... chrome/test/functional/tracing.py:75: return TimelineModel(self); On 2012/07/13 09:47:56, nduca wrote: > What in the test does this cause us to wait on the send inside traceEnded? I hope I understand this question correctly. ExecuteJavascript won't return until window.domAutomationController.send is called. The endTracing() call will asynchronously cause the eventListener registered on line 52 to be invoked, then causing this call to return.
So this should be ready for review, but it won't work without Nat's changes here http://codereview.appspot.com/6343106/ There's also one more change to trace-viewer to introduce findAllThreadsNamed which I can't upload until Nat's code is merged. Also, see my comment about traceData in tracing_base.py http://codereview.chromium.org/10736055/diff/13001/chrome/test/functional/tra... File chrome/test/functional/tracing/tracing_base.py (right): http://codereview.chromium.org/10736055/diff/13001/chrome/test/functional/tra... chrome/test/functional/tracing/tracing_base.py:22: var events = [tracingController.traceEvents_]; I wasn't able to use tracingController.traceData as that only gets set when you read from a file.
http://codereview.chromium.org/10736055/diff/13001/chrome/test/functional/tra... File chrome/test/functional/tracing/pyauto_tracing.py (right): http://codereview.chromium.org/10736055/diff/13001/chrome/test/functional/tra... chrome/test/functional/tracing/pyauto_tracing.py:19: I'm confused what this file does? If you look at e.g. perf.py, it just directly imports pyauto_functional? http://codereview.chromium.org/10736055/diff/13001/chrome/test/functional/tra... File chrome/test/functional/tracing/timeline_model.js (right): http://codereview.chromium.org/10736055/diff/13001/chrome/test/functional/tra... chrome/test/functional/tracing/timeline_model.js:2: var sendToPython = function(obj) { How about this being tracing_test_shim.js -- and fusing the javascript code that this does with the javascript code for getting a recording? And have it define a TimelineModelShim object that is the shim between the python model and the real model. Then, define methods on that shim object that are methods you need for providing certain data out to python. http://codereview.chromium.org/10736055/diff/13001/chrome/test/functional/tra... chrome/test/functional/tracing/timeline_model.js:2: var sendToPython = function(obj) { This is a private method, only to be called by the code below. So, use _sendToPython to denote it, or put the entire code in base.defineModule('timeline_model_proxy_support') .exporstTo('tracing', function() { .... this code return { InvokeOnTimelineModel: InvokeOnTimelineModel }; }); That way you get module-local vars and a clearly defined export. http://codereview.chromium.org/10736055/diff/13001/chrome/test/functional/tra... chrome/test/functional/tracing/timeline_model.js:5: // JSON and invokes sendJSON. The JSON conversion is what fails. This Why does the json conversion fail? http://codereview.chromium.org/10736055/diff/13001/chrome/test/functional/tra... chrome/test/functional/tracing/timeline_model.js:10: JSON.stringify( I'm so confused. This makes no sense. Also, does sendJSON convert to json internally?! So you're json-ing three times?! http://codereview.chromium.org/10736055/diff/13001/chrome/test/functional/tra... chrome/test/functional/tracing/timeline_model.js:15: try { Why are you sourcing this file every time? You could just make this be a function, e.g. InvokeOnTimelineModel, and load it at test start? http://codereview.chromium.org/10736055/diff/13001/chrome/test/functional/tra... chrome/test/functional/tracing/timeline_model.js:16: sendToPython({ Can't you break this up into individual lines instead of crazy method calls inside method calls? Temp vars for the win, bro! Also, why is sendToPython inside your exception handler? http://codereview.chromium.org/10736055/diff/13001/chrome/test/functional/tra... chrome/test/functional/tracing/timeline_model.js:32: // doesn't work, try sending without the exception. What? Why? http://codereview.chromium.org/10736055/diff/13001/chrome/test/functional/tra... File chrome/test/functional/tracing/timeline_model.py (right): http://codereview.chromium.org/10736055/diff/13001/chrome/test/functional/tra... chrome/test/functional/tracing/timeline_model.py:9: class TimelineModelProxy: Seems to me you can just read the js here one-off, exeucte it, then execute the js once during load? http://codereview.chromium.org/10736055/diff/13001/chrome/test/functional/tra... chrome/test/functional/tracing/timeline_model.py:12: self._remote_object_js = remote_object_js _remote_shim_name http://codereview.chromium.org/10736055/diff/13001/chrome/test/functional/tra... chrome/test/functional/tracing/timeline_model.py:14: def _escape_for_quoted_javascript_execution(self, js): this should be a global var. It doesn't need anything on self, so it should be a member. http://codereview.chromium.org/10736055/diff/13001/chrome/test/functional/tra... chrome/test/functional/tracing/timeline_model.py:20: def _js_call(self, method_name, *args): I think this is overcomplex. I think that you should make this be "callMethodOnThis(method_name, args). If you need complex js that does stuff on the js side, it should be a method on the shim that you dispatch through. If you eventually need something specific that allows foo.bar[x].y[3], then you should make ANOTHER method called _execute_in_context_of_model() that evals the provided string to get a function, then runs the function. But, dont do that until you need it, and keep it separate from the basic use case. http://codereview.chromium.org/10736055/diff/13001/chrome/test/functional/tra... chrome/test/functional/tracing/timeline_model.py:34: windex = self._tracing_js_host._trace_win) Is accessing _trace_win a normative pracice in pyauto tests? This seems pretty wacky since _ denotes private. http://codereview.chromium.org/10736055/diff/13001/chrome/test/functional/tra... File chrome/test/functional/tracing/tracing_base.py (right): http://codereview.chromium.org/10736055/diff/13001/chrome/test/functional/tra... chrome/test/functional/tracing/tracing_base.py:13: class TracingTestBase(pyauto.PyUITest): Typically, python code names the file after the class it contains. So this file would be TracingTestBase. Though it looks like pyauto uses the conventin XXXTest. So this would just be TracingTest, inside tracing_test.py http://codereview.chromium.org/10736055/diff/13001/chrome/test/functional/tra... chrome/test/functional/tracing/tracing_base.py:20: tracingController.addEventListener('traceEnded', function() { This feels like it could be in the shim code. E.g. have the shim define an export "recordTrace." http://codereview.chromium.org/10736055/diff/13001/chrome/test/functional/tra... chrome/test/functional/tracing/tracing_base.py:21: window.pyauto_timeline_model = new tracing.TimelineModel(); Instead of creating a timeline model directly, create a shim that wraps or subclasses the timeline model. This gives you a clean place where you can add functions to the TimelineModel that are specific to its use from python. http://codereview.chromium.org/10736055/diff/13001/chrome/test/functional/tra... chrome/test/functional/tracing/tracing_base.py:21: window.pyauto_timeline_model = new tracing.TimelineModel(); We use window.fooBar convention in js for globals. Its enough to drive you mad :) http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml#Naming http://codereview.chromium.org/10736055/diff/13001/chrome/test/functional/tra... chrome/test/functional/tracing/tracing_base.py:33: self._trace_win = None Is this conventional, to assign the trace_win to none here? Its fine if other tests do this, but it just smells funny to me. http://codereview.chromium.org/10736055/diff/13001/chrome/test/functional/tra... chrome/test/functional/tracing/tracing_base.py:36: def _BeginTracing(self, system_tracing=True): Where's this get called from? http://codereview.chromium.org/10736055/diff/13001/chrome/test/functional/tra... chrome/test/functional/tracing/tracing_base.py:46: def _EndTracing(self): Where's this called from? http://codereview.chromium.org/10736055/diff/13001/chrome/test/pyautolib/pyau... File chrome/test/pyautolib/pyauto.py (right): http://codereview.chromium.org/10736055/diff/13001/chrome/test/pyautolib/pyau... chrome/test/pyautolib/pyauto.py:3208: def ExecuteJavascriptWithFile(self, js, fileName, tab_index=0, Feels like this shoudl be a standalone changelist that is posted to nirimesh or some other pyauto owner. http://codereview.chromium.org/10736055/diff/13001/chrome/test/pyautolib/pyau... chrome/test/pyautolib/pyauto.py:3210: """Executes two scripts, one from a file, and one provided as an argument. Personally, this whole entry point feels kinda strange. Why not just do this in the code that needs this? I really dont grok why this is a commmon use case. http://codereview.chromium.org/10736055/diff/13001/chrome/test/pyautolib/pyau... chrome/test/pyautolib/pyauto.py:3233: if( fd != None ): if fd: fd.close() shoudl work
http://codereview.chromium.org/10736055/diff/19013/chrome/test/functional/tra... File chrome/test/functional/tracing/timeline_model.py (right): http://codereview.chromium.org/10736055/diff/19013/chrome/test/functional/tra... chrome/test/functional/tracing/timeline_model.py:13: def __init__(self, js_executor, uuid): uuid -> something more obvious. decorator_id, if thats the name we stick with http://codereview.chromium.org/10736055/diff/19013/chrome/test/functional/tra... chrome/test/functional/tracing/timeline_model.py:17: # Do note that the JSON serialization process removes cyclic references. # NB: The json serialization process ..., or # Warning: The json serialization process removs balbah Probaly also say, callers should take care to regenerate these references when needed. http://codereview.chromium.org/10736055/diff/19013/chrome/test/functional/tra... chrome/test/functional/tracing/timeline_model.py:29: if( result["success"] ): python style if x: never if (x): http://codereview.chromium.org/10736055/diff/19013/chrome/test/functional/tra... chrome/test/functional/tracing/timeline_model.py:31: # TODO(eatnumber) Make these exceptions more reader friendly Comments end with periods because they are complete sentences. http://codereview.chromium.org/10736055/diff/19013/chrome/test/functional/tra... File chrome/test/functional/tracing/timeline_model_decorator.js (right): http://codereview.chromium.org/10736055/diff/19013/chrome/test/functional/tra... chrome/test/functional/tracing/timeline_model_decorator.js:8: I'm struggling with naming here. Decorator is not quite the thing thats going on here. Decorators usually augment rather than subclass... I like this approach though, of subclassing. Its clean. We just gotta name it. :) I'd almost just say this is is a shim? TimelineModelShim then you have shim_id instad of uuid http://codereview.chromium.org/10736055/diff/19013/chrome/test/functional/tra... chrome/test/functional/tracing/timeline_model_decorator.js:12: invokeOnTimelineModel: function(methodName, args) { since you've made this a subclass of TimelineModel, name it invokeMethod? http://codereview.chromium.org/10736055/diff/19013/chrome/test/functional/tra... chrome/test/functional/tracing/timeline_model_decorator.js:66: window.domAutomationController.send(''); Put a comment about what this does? It doesn't make sense until you understand how brain damaged pyauto is wrt asynchronous communication. http://codereview.chromium.org/10736055/diff/19013/chrome/test/functional/tra... File chrome/test/functional/tracing/tracing_js_executor.py (right): http://codereview.chromium.org/10736055/diff/19013/chrome/test/functional/tra... chrome/test/functional/tracing/tracing_js_executor.py:8: def __init__(self, js_host, win_idx = 0, tab_idx = 0): I think you should just put this file into the test file, and give the model the ExecuteJavascript method. Nothing else. http://codereview.chromium.org/10736055/diff/19013/chrome/test/functional/tra... File chrome/test/functional/tracing/tracing_smoke_test.py (right): http://codereview.chromium.org/10736055/diff/19013/chrome/test/functional/tra... chrome/test/functional/tracing/tracing_smoke_test.py:14: self.BeginTracing() naming convention! here and throughout module_name, package_name, ClassName, method_name, ExceptionName, function_name, GLOBAL_CONSTANT_NAME, global_var_name, instance_var_name, function_parameter_name, local_var_name. http://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Naming http://codereview.chromium.org/10736055/diff/19013/chrome/test/functional/tra... File chrome/test/functional/tracing/tracing_test.js (right): http://codereview.chromium.org/10736055/diff/19013/chrome/test/functional/tra... chrome/test/functional/tracing/tracing_test.js:5: window.pyautoRecordTrace = function(systemTracing, uuid) { systemTracing -> systemTracingEnabled, copy the style of tracingController http://codereview.chromium.org/10736055/diff/19013/chrome/test/functional/tra... chrome/test/functional/tracing/tracing_test.js:7: TimelineModelDecorator.recordTrace(function(model) { I think this record stuff should be in this file, not in the TimelineModelDecorator. http://codereview.chromium.org/10736055/diff/19013/chrome/test/functional/tra... File chrome/test/functional/tracing/tracing_test.py (right): http://codereview.chromium.org/10736055/diff/19013/chrome/test/functional/tra... chrome/test/functional/tracing/tracing_test.py:22: self._js_executor = TracingJsExecutor( I'm not clear we need an executor. Why not just put the methods of the executor on this, and give the TimelineModel an ExecuteJavascript method? Thats all it needs... In particular, I think you can just write the code to read and exeucte javascript as a one off. You only need to do it... twice? And only in this file. http://codereview.chromium.org/10736055/diff/19013/chrome/test/functional/tra... chrome/test/functional/tracing/tracing_test.py:51: uuid = self._js_executor.ExecuteJavascript( Let javascript come up with its own id, rather than giving one to it. Dont use something as complex as a uuid. Have the test.js file install a counter on the window that is set to zero. e.g. _next_shim_id = 0; Then bump it each time you instantiate a shim. Makes for much easier debugging!
craigdh@ gave permission for the changes to pyauto.py
On 2012/07/26 00:24:04, Russ Harmon wrote: > craigdh@ gave permission for the changes to pyauto.py Whoops, ignore that. I didn't mean to put those changes here.
LGTM. This is nice work.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/eatnumber@chromium.org/10736055/39001
Presubmit check for 10736055-39001 failed and returned exit status 1.
Running presubmit commit checks ...
** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
chrome/test/functional
Add a description.
Presubmit checks took 1.1s to calculate.
1. There are many python style issues with this CL. Please follow the python-specific notes at: http://dev.chromium.org/developers/coding-style and update the CL accordingly. 2. Please include an entry in chrome/test/functional/PYAUTO_TESTS -- add a new suite which you'll need to schedule the tests with autotest. https://chromiumcodereview.appspot.com/10736055/diff/39001/chrome/test/functi... File chrome/test/functional/tracing/tab_tracker.py (right): https://chromiumcodereview.appspot.com/10736055/diff/39001/chrome/test/functi... chrome/test/functional/tracing/tab_tracker.py:7: # The TabTracker class allows the creation of tabs whose indices can be This sounds like a docstring. Move inside the class as docstring. https://chromiumcodereview.appspot.com/10736055/diff/39001/chrome/test/functi... chrome/test/functional/tracing/tab_tracker.py:11: def __init__(self, browser, visible = False): remove parents around = https://chromiumcodereview.appspot.com/10736055/diff/39001/chrome/test/functi... chrome/test/functional/tracing/tab_tracker.py:24: def createTab(self, url="about:blank"): createTab -> CreateTab Please repeat for all methods. https://chromiumcodereview.appspot.com/10736055/diff/39001/chrome/test/functi... chrome/test/functional/tracing/tab_tracker.py:24: def createTab(self, url="about:blank"): use single quote char ' instead of " for consistency https://chromiumcodereview.appspot.com/10736055/diff/39001/chrome/test/functi... chrome/test/functional/tracing/tab_tracker.py:24: def createTab(self, url="about:blank"): Add docstring, with an Args: section.
https://chromiumcodereview.appspot.com/10736055/diff/39001/chrome/test/functi... File chrome/test/functional/tracing/tab_tracker.py (right): https://chromiumcodereview.appspot.com/10736055/diff/39001/chrome/test/functi... chrome/test/functional/tracing/tab_tracker.py:11: def __init__(self, browser, visible = False): On 2012/07/30 18:57:19, Nirnimesh wrote: > remove parents around = err.. parents -> space
Ok, I think I've fixed all the style issues, added appropriate docstrings, and added a suite to PYAUTO_TESTS.
More style issues remaining. I'm pointing each one out only once, but please apply throughout. https://chromiumcodereview.appspot.com/10736055/diff/31011/chrome/test/functi... File chrome/test/functional/tracing/tab_tracker.py (right): https://chromiumcodereview.appspot.com/10736055/diff/31011/chrome/test/functi... chrome/test/functional/tracing/tab_tracker.py:8: """The TabTracker class allows the creation of tabs whose indices can be Remove "The TabTracker class" prefix. It's redundant. The first sentence should be a one-liner (and should fit within 80 chars). Details go in the next para. https://chromiumcodereview.appspot.com/10736055/diff/31011/chrome/test/functi... File chrome/test/functional/tracing/timeline_model.py (right): https://chromiumcodereview.appspot.com/10736055/diff/31011/chrome/test/functi... chrome/test/functional/tracing/timeline_model.py:18: # TODO(eatnumber) regenerate these cyclic references on deserialization. Need : after TODO(eatnumber) https://chromiumcodereview.appspot.com/10736055/diff/31011/chrome/test/functi... chrome/test/functional/tracing/timeline_model.py:58: TimelineModel = TimelineModelProxy Why not change line 12 instead? https://chromiumcodereview.appspot.com/10736055/diff/31011/chrome/test/functi... File chrome/test/functional/tracing/tracer.py (right): https://chromiumcodereview.appspot.com/10736055/diff/31011/chrome/test/functi... chrome/test/functional/tracing/tracer.py:11: from timeline_model import TimelineModel This need not be in a separate section. Move to line 10 https://chromiumcodereview.appspot.com/10736055/diff/31011/chrome/test/functi... chrome/test/functional/tracing/tracer.py:31: # I'm not happy with importing timeline_model_shim.js Add a TODO? https://chromiumcodereview.appspot.com/10736055/diff/31011/chrome/test/functi... chrome/test/functional/tracing/tracer.py:92: browser: an instance of a PyUITest""" move """ to next line https://chromiumcodereview.appspot.com/10736055/diff/31011/chrome/test/functi... File chrome/test/functional/tracing/tracing_smoke_test.py (right): https://chromiumcodereview.appspot.com/10736055/diff/31011/chrome/test/functi... chrome/test/functional/tracing/tracing_smoke_test.py:11: Need 2 blank lines at global scope https://chromiumcodereview.appspot.com/10736055/diff/31011/chrome/test/functi... chrome/test/functional/tracing/tracing_smoke_test.py:12: class TracingSmokeTest(pyauto.PyUITest): Docstring please https://chromiumcodereview.appspot.com/10736055/diff/31011/chrome/test/functi... chrome/test/functional/tracing/tracing_smoke_test.py:17: def testGetData(self): Docstring plz. This is the text that'll get printed out when the test runs. If you don't have anything, it prints "None" which doesn't look good. https://chromiumcodereview.appspot.com/10736055/diff/31011/chrome/test/functi... chrome/test/functional/tracing/tracing_smoke_test.py:30: self.assertEqual(1, len(model1.FindAllThreadsNamed("CrBrowserMain"))) use ' instead of " https://chromiumcodereview.appspot.com/10736055/diff/31011/chrome/test/functi... chrome/test/functional/tracing/tracing_smoke_test.py:48: add another blank line here
Ok. Nirnimesh, want to have a look again? https://chromiumcodereview.appspot.com/10736055/diff/31011/chrome/test/functi... File chrome/test/functional/tracing/timeline_model.py (right): https://chromiumcodereview.appspot.com/10736055/diff/31011/chrome/test/functi... chrome/test/functional/tracing/timeline_model.py:58: TimelineModel = TimelineModelProxy On 2012/07/30 21:03:51, Nirnimesh wrote: > Why not change line 12 instead? Because while I don't want authors of tests to know that this is a proxy, it _is_ a proxy, and so naming the class as such is the most logical and readable thing to do.
There still are several style issues remaining in this CL. Please fix them all. I'm not sure tracking the tab indices is a good idea. Why can't the tracer script supply the right index directly? https://chromiumcodereview.appspot.com/10736055/diff/31011/chrome/test/functi... File chrome/test/functional/tracing/timeline_model.py (right): https://chromiumcodereview.appspot.com/10736055/diff/31011/chrome/test/functi... chrome/test/functional/tracing/timeline_model.py:58: TimelineModel = TimelineModelProxy On 2012/07/31 01:11:16, Russ Harmon wrote: > On 2012/07/30 21:03:51, Nirnimesh wrote: > > Why not change line 12 instead? > > Because while I don't want authors of tests to know that this is a proxy, it > _is_ a proxy, and so naming the class as such is the most logical and readable > thing to do. That seems like a good note to put in the docstring. "This is a proxy" The style guide discourages global vars. In essence that's what you're doing here. It can be avoided. https://chromiumcodereview.appspot.com/10736055/diff/26021/chrome/test/functi... File chrome/test/functional/tracing/tab_tracker.py (right): https://chromiumcodereview.appspot.com/10736055/diff/26021/chrome/test/functi... chrome/test/functional/tracing/tab_tracker.py:19: TabTracker is instantiated, this TabTracker will lose track of it's window""" it's -> its https://chromiumcodereview.appspot.com/10736055/diff/26021/chrome/test/functi... chrome/test/functional/tracing/tab_tracker.py:24: browser: an instance of a PyUITest remove 'a' https://chromiumcodereview.appspot.com/10736055/diff/26021/chrome/test/functi... chrome/test/functional/tracing/tab_tracker.py:40: """Create a tracked tab and return it's uuid. its https://chromiumcodereview.appspot.com/10736055/diff/26021/chrome/test/functi... File chrome/test/functional/tracing/timeline_model.py (right): https://chromiumcodereview.appspot.com/10736055/diff/26021/chrome/test/functi... chrome/test/functional/tracing/timeline_model.py:1: # Copyright (c) 2011 The Chromium Authors. All rights reserved. 2012 https://chromiumcodereview.appspot.com/10736055/diff/26021/chrome/test/functi... chrome/test/functional/tracing/timeline_model.py:8: def escape_for_quoted_javascript_execution(js): CapitalizeAndRemoveSpaces Prefix _ for private functions https://chromiumcodereview.appspot.com/10736055/diff/26021/chrome/test/functi... chrome/test/functional/tracing/timeline_model.py:14: """A proxy for the TimelineModel class located in V8 in a chrome://tracing Make it fit within 1 line please. https://chromiumcodereview.appspot.com/10736055/diff/26021/chrome/test/functi... chrome/test/functional/tracing/timeline_model.py:25: """ Can be moved to previous line https://chromiumcodereview.appspot.com/10736055/diff/26021/chrome/test/functi... chrome/test/functional/tracing/timeline_model.py:27: """ % ( indent 2 more chars to the right https://chromiumcodereview.appspot.com/10736055/diff/26021/chrome/test/functi... chrome/test/functional/tracing/timeline_model.py:33: if result["success"]: use ' for quoting https://chromiumcodereview.appspot.com/10736055/diff/26021/chrome/test/functi... chrome/test/functional/tracing/timeline_model.py:36: raise Exception(result) RuntimeError https://chromiumcodereview.appspot.com/10736055/diff/26021/chrome/test/functi... chrome/test/functional/tracing/timeline_model.py:40: """ Needs to be indented 4 chars wrt previous line https://chromiumcodereview.appspot.com/10736055/diff/26021/chrome/test/functi... File chrome/test/functional/tracing/tracer.py (right): https://chromiumcodereview.appspot.com/10736055/diff/26021/chrome/test/functi... chrome/test/functional/tracing/tracer.py:16: """Allows you to start and stop chrome and system tracing, First sentence should be a one-liner. Details in next para. https://chromiumcodereview.appspot.com/10736055/diff/26021/chrome/test/functi... chrome/test/functional/tracing/tracer.py:34: # TODO(eatnumber): Find a way to import timeline_model_shim.js from within Move TODO to line 31 https://chromiumcodereview.appspot.com/10736055/diff/26021/chrome/test/functi... chrome/test/functional/tracing/tracer.py:36: files = ["timeline_model_shim.js", "tracer.js"] use ' https://chromiumcodereview.appspot.com/10736055/diff/26021/chrome/test/functi... chrome/test/functional/tracing/tracer.py:38: fileJs, fd = (None, None) fileJs -> file_js https://chromiumcodereview.appspot.com/10736055/diff/26021/chrome/test/functi... chrome/test/functional/tracing/tracer.py:39: try: do not use try block. with open(...) as fd: fileJS = fd.read() has the same effect https://chromiumcodereview.appspot.com/10736055/diff/26021/chrome/test/functi... chrome/test/functional/tracing/tracer.py:65: """ This needs to be indented by 4 spaces wrt previous line. Please fix everywhere https://chromiumcodereview.appspot.com/10736055/diff/26021/chrome/test/functi... chrome/test/functional/tracing/tracer.py:70: "true" if system_tracing else "false" use ' (I'm not pointing it out anymore. Please fix it everywhere in this CL.) https://chromiumcodereview.appspot.com/10736055/diff/26021/chrome/test/functi... chrome/test/functional/tracing/tracer.py:83: """tracingController.endTracing();""" why """? https://chromiumcodereview.appspot.com/10736055/diff/26021/chrome/test/functi... chrome/test/functional/tracing/tracer.py:86: Need another blank line https://chromiumcodereview.appspot.com/10736055/diff/26021/chrome/test/functi... chrome/test/functional/tracing/tracer.py:88: """A TracerFactory is used to produce a Tracer End with . for consistency https://chromiumcodereview.appspot.com/10736055/diff/26021/chrome/test/functi... File chrome/test/functional/tracing/tracing_smoke_test.py (right): https://chromiumcodereview.appspot.com/10736055/diff/26021/chrome/test/functi... chrome/test/functional/tracing/tracing_smoke_test.py:6: import unittest unused? https://chromiumcodereview.appspot.com/10736055/diff/26021/chrome/test/functi... chrome/test/functional/tracing/tracing_smoke_test.py:33: del tracer Why do you have to explicitly destroy it? https://chromiumcodereview.appspot.com/10736055/diff/26021/chrome/test/functi... chrome/test/functional/tracing/tracing_smoke_test.py:42: # I don't know what will happen if you try to nest calls to beginTracing. Do not use first person language in code. ie rephrase to remove "I" (since it's not clear who it refers to)
https://chromiumcodereview.appspot.com/10736055/diff/31011/chrome/test/functi... File chrome/test/functional/tracing/timeline_model.py (right): https://chromiumcodereview.appspot.com/10736055/diff/31011/chrome/test/functi... chrome/test/functional/tracing/timeline_model.py:58: TimelineModel = TimelineModelProxy On 2012/07/31 20:05:57, Nirnimesh wrote: > On 2012/07/31 01:11:16, Russ Harmon wrote: > > On 2012/07/30 21:03:51, Nirnimesh wrote: > > > Why not change line 12 instead? > > > > Because while I don't want authors of tests to know that this is a proxy, it > > _is_ a proxy, and so naming the class as such is the most logical and readable > > thing to do. > > That seems like a good note to put in the docstring. "This is a proxy" > The style guide discourages global vars. In essence that's what you're doing > here. It can be avoided. How can it be avoided while both keeping a name for the TimelineModelProxy class illustrative of the fact that this is a proxy, and avoiding having test authors know that this class is a proxy? https://chromiumcodereview.appspot.com/10736055/diff/26021/chrome/test/functi... File chrome/test/functional/tracing/timeline_model.py (right): https://chromiumcodereview.appspot.com/10736055/diff/26021/chrome/test/functi... chrome/test/functional/tracing/timeline_model.py:40: """ On 2012/07/31 20:05:57, Nirnimesh wrote: > Needs to be indented 4 chars wrt previous line It doesn't make sense to further indent this as there is no additional enclosing scope. From PEP8: "Continuation lines should align wrapped elements either vertically using Python's implicit line joining inside parentheses, brackets and braces, or using a hanging indent. When using a hanging indent the following considerations should be applied; there should be no arguments on the first line and further indentation should be used to clearly distinguish itself as a continuation line." This code uses a single hanging indent as PEP8 recommends. Since the next non-continuation line has a lesser indentation level, it is clearly distinguished. https://chromiumcodereview.appspot.com/10736055/diff/26021/chrome/test/functi... File chrome/test/functional/tracing/tracer.py (right): https://chromiumcodereview.appspot.com/10736055/diff/26021/chrome/test/functi... chrome/test/functional/tracing/tracer.py:65: """ On 2012/07/31 20:05:57, Nirnimesh wrote: > This needs to be indented by 4 spaces wrt previous line. > Please fix everywhere See my previous comment in timeline_model.py https://chromiumcodereview.appspot.com/10736055/diff/26021/chrome/test/functi... chrome/test/functional/tracing/tracer.py:70: "true" if system_tracing else "false" On 2012/07/31 20:05:57, Nirnimesh wrote: > use ' > > (I'm not pointing it out anymore. Please fix it everywhere in this CL.) Sorry about that. I'll go through and fix every occurrence. My mistake. https://chromiumcodereview.appspot.com/10736055/diff/26021/chrome/test/functi... chrome/test/functional/tracing/tracer.py:83: """tracingController.endTracing();""" On 2012/07/31 20:05:57, Nirnimesh wrote: > why """? For consistency with everywhere else I call ExecuteJavascript. https://chromiumcodereview.appspot.com/10736055/diff/26021/chrome/test/functi... File chrome/test/functional/tracing/tracing_smoke_test.py (right): https://chromiumcodereview.appspot.com/10736055/diff/26021/chrome/test/functi... chrome/test/functional/tracing/tracing_smoke_test.py:33: del tracer On 2012/07/31 20:05:57, Nirnimesh wrote: > Why do you have to explicitly destroy it? In order to implicitly test that a TraceModel will stay valid after the Tracer which created it is deleted.
Please respond to my comment regarding tab indices. https://chromiumcodereview.appspot.com/10736055/diff/31011/chrome/test/functi... File chrome/test/functional/tracing/timeline_model.py (right): https://chromiumcodereview.appspot.com/10736055/diff/31011/chrome/test/functi... chrome/test/functional/tracing/timeline_model.py:58: TimelineModel = TimelineModelProxy On 2012/07/31 20:54:18, Russ Harmon wrote: > On 2012/07/31 20:05:57, Nirnimesh wrote: > > On 2012/07/31 01:11:16, Russ Harmon wrote: > > > On 2012/07/30 21:03:51, Nirnimesh wrote: > > > > Why not change line 12 instead? > > > > > > Because while I don't want authors of tests to know that this is a proxy, it > > > _is_ a proxy, and so naming the class as such is the most logical and > readable > > > thing to do. > > > > That seems like a good note to put in the docstring. "This is a proxy" > > The style guide discourages global vars. In essence that's what you're doing > > here. It can be avoided. > > How can it be avoided while both keeping a name for the TimelineModelProxy class > illustrative of the fact that this is a proxy, and avoiding having test authors > know that this class is a proxy? I don't understand your motivation. You want test authors to not know it's a proxy, yet you want it in the name. Doesn't calling this out in the class docstring help? https://chromiumcodereview.appspot.com/10736055/diff/26021/chrome/test/functi... File chrome/test/functional/tracing/timeline_model.py (right): https://chromiumcodereview.appspot.com/10736055/diff/26021/chrome/test/functi... chrome/test/functional/tracing/timeline_model.py:40: """ On 2012/07/31 20:54:18, Russ Harmon wrote: > On 2012/07/31 20:05:57, Nirnimesh wrote: > > Needs to be indented 4 chars wrt previous line > > It doesn't make sense to further indent this as there is no additional enclosing > scope. > > From PEP8: "Continuation lines should align wrapped elements either vertically > using Python's implicit line joining inside parentheses, brackets and braces, or > using a hanging indent. When using a hanging indent the following considerations > should be applied; there should be no arguments on the first line and further > indentation should be used to clearly distinguish itself as a continuation > line." > > This code uses a single hanging indent as PEP8 recommends. Since the next > non-continuation line has a lesser indentation level, it is clearly > distinguished. Chromium python style is based loosely on google python style guide. For hanging indent, it specifically calls for indentation by 4 spaces http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Indent... https://chromiumcodereview.appspot.com/10736055/diff/26021/chrome/test/functi... File chrome/test/functional/tracing/tracer.py (right): https://chromiumcodereview.appspot.com/10736055/diff/26021/chrome/test/functi... chrome/test/functional/tracing/tracer.py:83: """tracingController.endTracing();""" On the contrary, it breaks consistency. """ is supposed to be used for multi-line strings and docstrings only. In fact, using it for even multi-line strings is discouraged.
On 2012/07/31 20:05:57, Nirnimesh wrote: > There still are several style issues remaining in this CL. Please fix them all. > > I'm not sure tracking the tab indices is a good idea. Why can't the tracer > script supply the right index directly? > I used to have it this way. Unfortunately, if the tab index isn't tracked, when Tracers are created and destroyed, the tab index can become invalid. Specifically, the "testMultipleTracers" test at https://chromiumcodereview.appspot.com/10736055/patch/32004/39017 would break because when you deleted tracer1, it's tab would close. Then, tracer2's tab index would be invalid. Also, I fixed all of the style issues you raised with the code.
https://chromiumcodereview.appspot.com/10736055/diff/26021/chrome/test/functi... File chrome/test/functional/tracing/tracing_smoke_test.py (right): https://chromiumcodereview.appspot.com/10736055/diff/26021/chrome/test/functi... chrome/test/functional/tracing/tracing_smoke_test.py:33: del tracer On 2012/07/31 20:54:18, Russ Harmon wrote: > On 2012/07/31 20:05:57, Nirnimesh wrote: > > Why do you have to explicitly destroy it? > > In order to implicitly test that a TraceModel will stay valid after the Tracer > which created it is deleted. I just added an explicit test for this validity condition and removed the del statements from the other tests.
A few more minor comments. Then LGTM. https://chromiumcodereview.appspot.com/10736055/diff/39018/chrome/test/functi... File chrome/test/functional/tracing/tab_tracker.py (right): https://chromiumcodereview.appspot.com/10736055/diff/39018/chrome/test/functi... chrome/test/functional/tracing/tab_tracker.py:19: TabTracker is instantiated, this TabTracker will lose track of its window""" Move """ to next line https://chromiumcodereview.appspot.com/10736055/diff/39018/chrome/test/functi... File chrome/test/functional/tracing/timeline_model.py (right): https://chromiumcodereview.appspot.com/10736055/diff/39018/chrome/test/functi... chrome/test/functional/tracing/timeline_model.py:8: def _EscapeForQuotedJavascriptExecution(js): Move this inside the class as @staticmethod https://chromiumcodereview.appspot.com/10736055/diff/39018/chrome/test/functi... chrome/test/functional/tracing/timeline_model.py:40: window.timelineModelShims['%s'] = undefined; You didn't need to indent this block further, if you move """ on line 39 to line 38 instead. https://chromiumcodereview.appspot.com/10736055/diff/39018/chrome/test/functi... File chrome/test/functional/tracing/tracer.py (right): https://chromiumcodereview.appspot.com/10736055/diff/39018/chrome/test/functi... chrome/test/functional/tracing/tracer.py:13: class Tracer: I think the convention is to have all classes derive from object. Please repeat for all other classes. https://chromiumcodereview.appspot.com/10736055/diff/39018/chrome/test/functi... chrome/test/functional/tracing/tracer.py:33: file_js = None Not necessary. Either line 34 fails and throws an exception or line 35 succeeds. https://chromiumcodereview.appspot.com/10736055/diff/39018/chrome/test/functi... chrome/test/functional/tracing/tracer.py:61: 'true' if system_tracing else 'false' This can fit in previous line. Also the next line
On 2012/07/31 23:39:10, Nirnimesh wrote: > A few more minor comments. Then LGTM. > > https://chromiumcodereview.appspot.com/10736055/diff/39018/chrome/test/functi... > File chrome/test/functional/tracing/tab_tracker.py (right): > > https://chromiumcodereview.appspot.com/10736055/diff/39018/chrome/test/functi... > chrome/test/functional/tracing/tab_tracker.py:19: TabTracker is instantiated, > this TabTracker will lose track of its window""" > Move """ to next line > > https://chromiumcodereview.appspot.com/10736055/diff/39018/chrome/test/functi... > File chrome/test/functional/tracing/timeline_model.py (right): > > https://chromiumcodereview.appspot.com/10736055/diff/39018/chrome/test/functi... > chrome/test/functional/tracing/timeline_model.py:8: def > _EscapeForQuotedJavascriptExecution(js): > Move this inside the class as @staticmethod > > https://chromiumcodereview.appspot.com/10736055/diff/39018/chrome/test/functi... > chrome/test/functional/tracing/timeline_model.py:40: > window.timelineModelShims['%s'] = undefined; > You didn't need to indent this block further, if you move """ on line 39 to line > 38 instead. > > https://chromiumcodereview.appspot.com/10736055/diff/39018/chrome/test/functi... > File chrome/test/functional/tracing/tracer.py (right): > > https://chromiumcodereview.appspot.com/10736055/diff/39018/chrome/test/functi... > chrome/test/functional/tracing/tracer.py:13: class Tracer: > I think the convention is to have all classes derive from object. > > Please repeat for all other classes. > > https://chromiumcodereview.appspot.com/10736055/diff/39018/chrome/test/functi... > chrome/test/functional/tracing/tracer.py:33: file_js = None > Not necessary. Either line 34 fails and throws an exception or line 35 succeeds. > > https://chromiumcodereview.appspot.com/10736055/diff/39018/chrome/test/functi... > chrome/test/functional/tracing/tracer.py:61: 'true' if system_tracing else > 'false' > This can fit in previous line. Also the next line This should be fixed now. LGTM?
My previous LGTM holds.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/eatnumber@chromium.org/10736055/28015
Change committed as 149345 |
