Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
180273015:
Simple HTTP server for Chromoting End-to-End tests (Closed)
|
Created:
6 years, 9 months ago by chaitali Modified:
6 years, 9 months ago CC:
chromium-reviews, jamiewalch+watch_chromium.org, dcaiafa+watch_chromium.org, hclam+watch_chromium.org, wez+watch_chromium.org, amit, sanjeevr, garykac+watch_chromium.org, lambroslambrou+watch_chromium.org, rmsousa+watch_chromium.org, weitaosu+watch_chromium.org, alexeypa+watch_chromium.org, sergeyu+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionSimple HTTP server for Chromoting End-to-End tests
BUG=341526
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=255771
Patch Set 1 #
Total comments: 29
Patch Set 2 : Addressing initial code review comments #
Total comments: 42
Patch Set 3 : Addressing Jamie's comments #
Total comments: 2
Patch Set 4 : Fixing lint issues reported by gjslint #
Messages
Total messages: 33 (0 generated)
HTTP server for the E2E tests. This is using JQuery because it was actually more readable and easier to do things like polling and keypress in it than raw javascript. It will also make future E2E tests easier to write and maintain I believe. The server uses CherryPy (http://www.cherrypy.org/) and is a fairly lightweight server.
I'm not convinced there's a lot of value in using jQuery. It's not a framework we're familiar with, so we'd need to be able justify the cost of learning it, which seems difficult given that I don't expect these pages ever to be particularly complex. I have similar feelings about cherrypy, though I'm less familiar with Python, and so less qualified. Does it give us anything that can't easily be achieved with Python's built-in HTTP server libraries? https://codereview.chromium.org/180273015/diff/1/chrome/test/remoting/http_se... File chrome/test/remoting/http_server/clientpage.html (right): https://codereview.chromium.org/180273015/diff/1/chrome/test/remoting/http_se... chrome/test/remoting/http_server/clientpage.html:4: <link rel="stylesheet" href="//yui.yahooapis.com/pure/0.4.2/pure-min.css"> Is this CSS essential for the test? If not, I would prefer not to depend on anything external. https://codereview.chromium.org/180273015/diff/1/chrome/test/remoting/http_se... chrome/test/remoting/http_server/clientpage.html:8: <script> Please pull this script out into a separate .js file and pull it in in the <head>. It makes for easier editing (and syntax highlighting in code-reviews), and we have tools to type-check JS source files, which have proven very useful in catching bugs. https://codereview.chromium.org/180273015/diff/1/chrome/test/remoting/http_se... chrome/test/remoting/http_server/clientpage.html:32: })(); I don't think there's much value in using this declare-and-call syntax, given that the function is not anonymous. It's less readable than just explicitly calling poll() after declaring the function. https://codereview.chromium.org/180273015/diff/1/chrome/test/remoting/http_se... File chrome/test/remoting/http_server/hostpage.html (right): https://codereview.chromium.org/180273015/diff/1/chrome/test/remoting/http_se... chrome/test/remoting/http_server/hostpage.html:4: <link rel="stylesheet" href="//yui.yahooapis.com/pure/0.4.2/pure-min.css"> See above re: external URLs. https://codereview.chromium.org/180273015/diff/1/chrome/test/remoting/http_se... chrome/test/remoting/http_server/hostpage.html:15: <script> As with the client page, please pull out the JS into a separate file. https://codereview.chromium.org/180273015/diff/1/chrome/test/remoting/http_se... chrome/test/remoting/http_server/hostpage.html:17: if ( e.which == 13 ) { Nit: No space after '(' or before ')'. https://codereview.chromium.org/180273015/diff/1/chrome/test/remoting/http_se... chrome/test/remoting/http_server/hostpage.html:25: console.log("Sent XHR!"); Maybe remove these console logs, or replace them with something testable (like setting a variable).
Thanks chaitali@. PTAL. https://codereview.chromium.org/180273015/diff/1/chrome/test/remoting/http_se... File chrome/test/remoting/http_server/clientpage.html (right): https://codereview.chromium.org/180273015/diff/1/chrome/test/remoting/http_se... chrome/test/remoting/http_server/clientpage.html:11: var keypressText = ""; Single-braces for strings. http://dev.chromium.org/developers/web-development-style-guide https://codereview.chromium.org/180273015/diff/1/chrome/test/remoting/http_se... chrome/test/remoting/http_server/clientpage.html:13: (function poll(){ pollForKeyPress() Also, space before opening-brace. http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showone... https://codereview.chromium.org/180273015/diff/1/chrome/test/remoting/http_se... File chrome/test/remoting/http_server/hostpage.html (right): https://codereview.chromium.org/180273015/diff/1/chrome/test/remoting/http_se... chrome/test/remoting/http_server/hostpage.html:8: <div class="splash"> Indent by 2 spaces: http://google-styleguide.googlecode.com/svn/trunk/htmlcssguide.xml?showone=In... Here and elsewhere. https://codereview.chromium.org/180273015/diff/1/chrome/test/remoting/http_se... chrome/test/remoting/http_server/hostpage.html:17: if ( e.which == 13 ) { Comment indicating 13 == carriage return https://codereview.chromium.org/180273015/diff/1/chrome/test/remoting/http_se... File chrome/test/remoting/http_server/http_server.py (right): https://codereview.chromium.org/180273015/diff/1/chrome/test/remoting/http_se... chrome/test/remoting/http_server/http_server.py:1: import cherrypy Would the BaseHttpServer module that comes with Python work for the purposes of this module's functionality? See https://code.google.com/p/chromium/codesearch#search&q=package:%5Echromium$%2... https://codereview.chromium.org/180273015/diff/1/chrome/test/remoting/http_se... chrome/test/remoting/http_server/http_server.py:5: Please run gpylint on this module. That should identify some import and indentation cleanup.
https://codereview.chromium.org/180273015/diff/1/chrome/test/remoting/http_se... File chrome/test/remoting/http_server/hostpage.html (right): https://codereview.chromium.org/180273015/diff/1/chrome/test/remoting/http_se... chrome/test/remoting/http_server/hostpage.html:11: <center><textarea id="testtext" rows="10" cols="50" ></textarea></center> Since you rely on tabbing to select the input control (see your other CL), please add tab order to make it more explicit.
https://codereview.chromium.org/180273015/diff/1/chrome/test/remoting/http_se... File chrome/test/remoting/http_server/http_server.py (right): https://codereview.chromium.org/180273015/diff/1/chrome/test/remoting/http_se... chrome/test/remoting/http_server/http_server.py:1: import cherrypy On 2014/03/03 21:10:03, anandc wrote: > Would the BaseHttpServer module that comes with Python work for the purposes of > this module's functionality? > See > https://code.google.com/p/chromium/codesearch#search&q=package:%255Echromium%... > Just noticed that the link doesn't work. This does: https://code.google.com/p/chromium/codesearch#chromium/tools/third_party/pyth...
Addressed all comments. Not using JQuery anymore and also removed the CSS. Had an offline discussion with Jamie about CherryPy and we found it already in the Chromium tree so continuing to use it. https://codereview.chromium.org/180273015/diff/1/chrome/test/remoting/http_se... File chrome/test/remoting/http_server/clientpage.html (right): https://codereview.chromium.org/180273015/diff/1/chrome/test/remoting/http_se... chrome/test/remoting/http_server/clientpage.html:4: <link rel="stylesheet" href="//yui.yahooapis.com/pure/0.4.2/pure-min.css"> On 2014/03/03 20:56:16, Jamie wrote: > Is this CSS essential for the test? If not, I would prefer not to depend on > anything external. Done. https://codereview.chromium.org/180273015/diff/1/chrome/test/remoting/http_se... chrome/test/remoting/http_server/clientpage.html:8: <script> On 2014/03/03 20:56:16, Jamie wrote: > Please pull this script out into a separate .js file and pull it in in the > <head>. It makes for easier editing (and syntax highlighting in code-reviews), > and we have tools to type-check JS source files, which have proven very useful > in catching bugs. Done. https://codereview.chromium.org/180273015/diff/1/chrome/test/remoting/http_se... chrome/test/remoting/http_server/clientpage.html:11: var keypressText = ""; On 2014/03/03 21:10:03, anandc wrote: > Single-braces for strings. > http://dev.chromium.org/developers/web-development-style-guide Done. https://codereview.chromium.org/180273015/diff/1/chrome/test/remoting/http_se... chrome/test/remoting/http_server/clientpage.html:13: (function poll(){ Added a TODO for myself. I would like it to poll for all test vars in each poll and keep polling till all are true. On 2014/03/03 21:10:03, anandc wrote: > pollForKeyPress() > Also, space before opening-brace. > http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showone... https://codereview.chromium.org/180273015/diff/1/chrome/test/remoting/http_se... chrome/test/remoting/http_server/clientpage.html:32: })(); On 2014/03/03 20:56:16, Jamie wrote: > I don't think there's much value in using this declare-and-call syntax, given > that the function is not anonymous. It's less readable than just explicitly > calling poll() after declaring the function. Done. https://codereview.chromium.org/180273015/diff/1/chrome/test/remoting/http_se... File chrome/test/remoting/http_server/hostpage.html (right): https://codereview.chromium.org/180273015/diff/1/chrome/test/remoting/http_se... chrome/test/remoting/http_server/hostpage.html:4: <link rel="stylesheet" href="//yui.yahooapis.com/pure/0.4.2/pure-min.css"> On 2014/03/03 20:56:16, Jamie wrote: > See above re: external URLs. Done. https://codereview.chromium.org/180273015/diff/1/chrome/test/remoting/http_se... chrome/test/remoting/http_server/hostpage.html:8: <div class="splash"> On 2014/03/03 21:10:03, anandc wrote: > Indent by 2 spaces: > http://google-styleguide.googlecode.com/svn/trunk/htmlcssguide.xml?showone=In... > Here and elsewhere. Done. https://codereview.chromium.org/180273015/diff/1/chrome/test/remoting/http_se... chrome/test/remoting/http_server/hostpage.html:11: <center><textarea id="testtext" rows="10" cols="50" ></textarea></center> On 2014/03/03 21:33:42, weitaosu wrote: > Since you rely on tabbing to select the input control (see your other CL), > please add tab order to make it more explicit. Done. https://codereview.chromium.org/180273015/diff/1/chrome/test/remoting/http_se... chrome/test/remoting/http_server/hostpage.html:15: <script> On 2014/03/03 20:56:16, Jamie wrote: > As with the client page, please pull out the JS into a separate file. Done. https://codereview.chromium.org/180273015/diff/1/chrome/test/remoting/http_se... chrome/test/remoting/http_server/hostpage.html:17: if ( e.which == 13 ) { On 2014/03/03 20:56:16, Jamie wrote: > Nit: No space after '(' or before ')'. Done. https://codereview.chromium.org/180273015/diff/1/chrome/test/remoting/http_se... chrome/test/remoting/http_server/hostpage.html:17: if ( e.which == 13 ) { On 2014/03/03 21:10:03, anandc wrote: > Comment indicating 13 == carriage return Done. https://codereview.chromium.org/180273015/diff/1/chrome/test/remoting/http_se... chrome/test/remoting/http_server/hostpage.html:25: console.log("Sent XHR!"); On 2014/03/03 20:56:16, Jamie wrote: > Maybe remove these console logs, or replace them with something testable (like > setting a variable). These are strictly for debugging if the test fails. The variable setting happens on the client page for the test to read. I can remove them if you feel strongly about it but I think they will be useful for debugging when the test fails. https://codereview.chromium.org/180273015/diff/1/chrome/test/remoting/http_se... File chrome/test/remoting/http_server/http_server.py (right): https://codereview.chromium.org/180273015/diff/1/chrome/test/remoting/http_se... chrome/test/remoting/http_server/http_server.py:1: import cherrypy Not for our purposes. Its pretty hard to code path handlers with the default module. Since this component will be touched everytime anyone adds a new test, I'd like it to be as easy as possible. Using a WSGI framework like CherryPy will help. CherryPy is already in Chromium here so we can use it too - https://code.google.com/p/chromium/codesearch#chromium/tools/build/third_part... On 2014/03/03 21:36:37, anandc wrote: > On 2014/03/03 21:10:03, anandc wrote: > > Would the BaseHttpServer module that comes with Python work for the purposes > of > > this module's functionality? > > See > > > https://code.google.com/p/chromium/codesearch#search&q=package:%25255Echromiu... > > > > Just noticed that the link doesn't work. This does: > https://code.google.com/p/chromium/codesearch#chromium/tools/third_party/pyth... https://codereview.chromium.org/180273015/diff/1/chrome/test/remoting/http_se... chrome/test/remoting/http_server/http_server.py:5: Done on most things. It still complains about invalid method names in the classes but thats cherrypy specific so can't be changed On 2014/03/03 21:10:03, anandc wrote: > Please run gpylint on this module. That should identify some import and > indentation cleanup.
Can you run jscompile over the Javascript? It's a bit of pain to add all the type annotations, but it has caught enough bugs for us in the past that I think it's worthwhile and it's much easier to code for it from the outset than it is to try to retro-fit it to existing code once it becomes complex. https://codereview.chromium.org/180273015/diff/20001/chrome/test/remoting/htt... File chrome/test/remoting/http_server/clientpage.html (right): https://codereview.chromium.org/180273015/diff/20001/chrome/test/remoting/htt... chrome/test/remoting/http_server/clientpage.html:1: <html> Please add a standard HTML header, including copyright: <!doctype html> <!-- Copyright (c) 2012 The Chromium Authors. All rights reserved. Use of this source code is governed by a BSD-style license that can be found in the LICENSE file. --> https://codereview.chromium.org/180273015/diff/20001/chrome/test/remoting/htt... chrome/test/remoting/http_server/clientpage.html:2: <head> Nit: 2-space indentation for <head> and <body> relative to <html> https://codereview.chromium.org/180273015/diff/20001/chrome/test/remoting/htt... chrome/test/remoting/http_server/clientpage.html:4: <script src="/static/clientpage.js"></script> Assuming the scripts are going to be collocated with the pages, this should be a relative path, i.e. just "clientpage.js". https://codereview.chromium.org/180273015/diff/20001/chrome/test/remoting/htt... File chrome/test/remoting/http_server/clientpage.js (right): https://codereview.chromium.org/180273015/diff/20001/chrome/test/remoting/htt... chrome/test/remoting/http_server/clientpage.js:12: // TODO(chaitali):Update this method to poll and get status of all Nit: Space before "Update". https://codereview.chromium.org/180273015/diff/20001/chrome/test/remoting/htt... chrome/test/remoting/http_server/clientpage.js:17: request.open('GET', '/poll?test=keytest', true); Can we use a relative URL here too? https://codereview.chromium.org/180273015/diff/20001/chrome/test/remoting/htt... chrome/test/remoting/http_server/clientpage.js:19: request.onreadystatechange = function() { Nit: Indentation. https://codereview.chromium.org/180273015/diff/20001/chrome/test/remoting/htt... chrome/test/remoting/http_server/clientpage.js:20: if(request.readyState == 4 && request.status == 200) { Nit: Space after "if". https://codereview.chromium.org/180273015/diff/20001/chrome/test/remoting/htt... chrome/test/remoting/http_server/clientpage.js:22: data = JSON.parse(request.responseText); This should be in a try...catch block, or there's a jsonParseSafe method that we use in remoting.js that you can copy if you're going to be parsing a lot of JSON. https://codereview.chromium.org/180273015/diff/20001/chrome/test/remoting/htt... chrome/test/remoting/http_server/clientpage.js:31: setTimeout(poll, 3000); 3s seems like quite a long time to wait. I think we could avoid a wait altogether using a hanging GET, but in the interim, maybe just reduce this to 1s? https://codereview.chromium.org/180273015/diff/20001/chrome/test/remoting/htt... chrome/test/remoting/http_server/clientpage.js:40: request.send(); Nit: Indentation. https://codereview.chromium.org/180273015/diff/20001/chrome/test/remoting/htt... chrome/test/remoting/http_server/clientpage.js:45: }); I think you're missing a 'false' parameter for addEventListener (technically, it's optional in Chrome, but you've used it elsewhere, and I think jscompile treats it as mandatory). https://codereview.chromium.org/180273015/diff/20001/chrome/test/remoting/htt... File chrome/test/remoting/http_server/hostpage.html (right): https://codereview.chromium.org/180273015/diff/20001/chrome/test/remoting/htt... chrome/test/remoting/http_server/hostpage.html:1: <html> The comments for clientpage.html all apply to this file too. https://codereview.chromium.org/180273015/diff/20001/chrome/test/remoting/htt... File chrome/test/remoting/http_server/hostpage.js (right): https://codereview.chromium.org/180273015/diff/20001/chrome/test/remoting/htt... chrome/test/remoting/http_server/hostpage.js:12: // then process the text in the textarea. Nit: This comment is wrapped in an odd place. https://codereview.chromium.org/180273015/diff/20001/chrome/test/remoting/htt... chrome/test/remoting/http_server/hostpage.js:18: request.open('POST', '/keytest/test', true); Can we use a relative URL here too? https://codereview.chromium.org/180273015/diff/20001/chrome/test/remoting/htt... chrome/test/remoting/http_server/hostpage.js:41: }); Missing 'false'. https://codereview.chromium.org/180273015/diff/20001/chrome/test/remoting/htt... File chrome/test/remoting/http_server/http_server.py (right): https://codereview.chromium.org/180273015/diff/20001/chrome/test/remoting/htt... chrome/test/remoting/http_server/http_server.py:23: print ('CNS requires CherryPy v3 or higher to be installed. Please install\n' What is CNS? Maybe just "This script requires..."? https://codereview.chromium.org/180273015/diff/20001/chrome/test/remoting/htt... chrome/test/remoting/http_server/http_server.py:47: I think these blank lines at the start of the methods make it harder to parse the structure of the file. https://codereview.chromium.org/180273015/diff/20001/chrome/test/remoting/htt... chrome/test/remoting/http_server/http_server.py:52: """Build the JSON message that will be conveyed to the client."""\ Nit: Stray backslash? https://codereview.chromium.org/180273015/diff/20001/chrome/test/remoting/htt... chrome/test/remoting/http_server/http_server.py:56: message['keypressText'] = self.keytest_text I think this can be more succinctly declared similarly to TEST_DICT below (ie, as one statement, not three). https://codereview.chromium.org/180273015/diff/20001/chrome/test/remoting/htt... chrome/test/remoting/http_server/http_server.py:59: if self.keytest_succeeded: Is this server single-threaded? If not, then you'll need some guards around these members since they might be accessed concurrently by different requests. Better still, configure it so that it is single-threaded.
Addressed comments. Also ran jscompile over the js files. It didn't give me any errors or warnings except initially complaining about console being undeclared. That got fixed with using the remoting/webapp/js_proto externs. Wanted to confirm if I did it right though. Were there any annotations you found missing? https://codereview.chromium.org/180273015/diff/20001/chrome/test/remoting/htt... File chrome/test/remoting/http_server/clientpage.html (right): https://codereview.chromium.org/180273015/diff/20001/chrome/test/remoting/htt... chrome/test/remoting/http_server/clientpage.html:1: <html> On 2014/03/04 21:40:35, Jamie wrote: > Please add a standard HTML header, including copyright: > > <!doctype html> > <!-- > Copyright (c) 2012 The Chromium Authors. All rights reserved. > Use of this source code is governed by a BSD-style license that can be > found in the LICENSE file. > --> > Done. https://codereview.chromium.org/180273015/diff/20001/chrome/test/remoting/htt... chrome/test/remoting/http_server/clientpage.html:2: <head> On 2014/03/04 21:40:35, Jamie wrote: > Nit: 2-space indentation for <head> and <body> relative to <html> Done. https://codereview.chromium.org/180273015/diff/20001/chrome/test/remoting/htt... chrome/test/remoting/http_server/clientpage.html:4: <script src="/static/clientpage.js"></script> On 2014/03/04 21:40:35, Jamie wrote: > Assuming the scripts are going to be collocated with the pages, this should be a > relative path, i.e. just "clientpage.js". Done. https://codereview.chromium.org/180273015/diff/20001/chrome/test/remoting/htt... File chrome/test/remoting/http_server/clientpage.js (right): https://codereview.chromium.org/180273015/diff/20001/chrome/test/remoting/htt... chrome/test/remoting/http_server/clientpage.js:12: // TODO(chaitali):Update this method to poll and get status of all On 2014/03/04 21:40:35, Jamie wrote: > Nit: Space before "Update". Done. https://codereview.chromium.org/180273015/diff/20001/chrome/test/remoting/htt... chrome/test/remoting/http_server/clientpage.js:17: request.open('GET', '/poll?test=keytest', true); Removed the /. The rest of it needs to remain so cherrypy can direct this to the poll handler. Is that what you meant by relative URL? On 2014/03/04 21:40:35, Jamie wrote: > Can we use a relative URL here too? https://codereview.chromium.org/180273015/diff/20001/chrome/test/remoting/htt... chrome/test/remoting/http_server/clientpage.js:19: request.onreadystatechange = function() { On 2014/03/04 21:40:35, Jamie wrote: > Nit: Indentation. Done. https://codereview.chromium.org/180273015/diff/20001/chrome/test/remoting/htt... chrome/test/remoting/http_server/clientpage.js:20: if(request.readyState == 4 && request.status == 200) { On 2014/03/04 21:40:35, Jamie wrote: > Nit: Space after "if". Done. https://codereview.chromium.org/180273015/diff/20001/chrome/test/remoting/htt... chrome/test/remoting/http_server/clientpage.js:22: data = JSON.parse(request.responseText); On 2014/03/04 21:40:35, Jamie wrote: > This should be in a try...catch block, or there's a jsonParseSafe method that we > use in remoting.js that you can copy if you're going to be parsing a lot of > JSON. Done. https://codereview.chromium.org/180273015/diff/20001/chrome/test/remoting/htt... chrome/test/remoting/http_server/clientpage.js:31: setTimeout(poll, 3000); Changed to 1s. I thought about long polling but what I want from the framework is that it polls and gets a list of status vars from the server. As the host completes actions, statuses will change. So it is possible that keypressSucceeded is true but mouseSucceeded is false at some given time. With regular polling we'll receive regular status of all vars and tests can complete as the statuses come in. With long polling we'll have to wait till all the vars are true on the server and block tests. What I would really like is Server Sent Events but not sure how to do that here. On 2014/03/04 21:40:35, Jamie wrote: > 3s seems like quite a long time to wait. I think we could avoid a wait > altogether using a hanging GET, but in the interim, maybe just reduce this to > 1s? https://codereview.chromium.org/180273015/diff/20001/chrome/test/remoting/htt... chrome/test/remoting/http_server/clientpage.js:40: request.send(); On 2014/03/04 21:40:35, Jamie wrote: > Nit: Indentation. Done. https://codereview.chromium.org/180273015/diff/20001/chrome/test/remoting/htt... chrome/test/remoting/http_server/clientpage.js:45: }); On 2014/03/04 21:40:35, Jamie wrote: > I think you're missing a 'false' parameter for addEventListener (technically, > it's optional in Chrome, but you've used it elsewhere, and I think jscompile > treats it as mandatory). Done. https://codereview.chromium.org/180273015/diff/20001/chrome/test/remoting/htt... File chrome/test/remoting/http_server/hostpage.html (right): https://codereview.chromium.org/180273015/diff/20001/chrome/test/remoting/htt... chrome/test/remoting/http_server/hostpage.html:1: <html> On 2014/03/04 21:40:35, Jamie wrote: > The comments for clientpage.html all apply to this file too. Done. https://codereview.chromium.org/180273015/diff/20001/chrome/test/remoting/htt... File chrome/test/remoting/http_server/hostpage.js (right): https://codereview.chromium.org/180273015/diff/20001/chrome/test/remoting/htt... chrome/test/remoting/http_server/hostpage.js:12: // then process the text in the textarea. On 2014/03/04 21:40:35, Jamie wrote: > Nit: This comment is wrapped in an odd place. Done. https://codereview.chromium.org/180273015/diff/20001/chrome/test/remoting/htt... chrome/test/remoting/http_server/hostpage.js:18: request.open('POST', '/keytest/test', true); Same comment. Removed the initial / if thats what you meant. But the rest is needed for cherrypy to route the POST properly. On 2014/03/04 21:40:35, Jamie wrote: > Can we use a relative URL here too? https://codereview.chromium.org/180273015/diff/20001/chrome/test/remoting/htt... chrome/test/remoting/http_server/hostpage.js:41: }); On 2014/03/04 21:40:35, Jamie wrote: > Missing 'false'. Done. https://codereview.chromium.org/180273015/diff/20001/chrome/test/remoting/htt... File chrome/test/remoting/http_server/http_server.py (right): https://codereview.chromium.org/180273015/diff/20001/chrome/test/remoting/htt... chrome/test/remoting/http_server/http_server.py:23: print ('CNS requires CherryPy v3 or higher to be installed. Please install\n' On 2014/03/04 21:40:35, Jamie wrote: > What is CNS? Maybe just "This script requires..."? Done. https://codereview.chromium.org/180273015/diff/20001/chrome/test/remoting/htt... chrome/test/remoting/http_server/http_server.py:47: On 2014/03/04 21:40:35, Jamie wrote: > I think these blank lines at the start of the methods make it harder to parse > the structure of the file. Done. https://codereview.chromium.org/180273015/diff/20001/chrome/test/remoting/htt... chrome/test/remoting/http_server/http_server.py:52: """Build the JSON message that will be conveyed to the client."""\ On 2014/03/04 21:40:35, Jamie wrote: > Nit: Stray backslash? Done. https://codereview.chromium.org/180273015/diff/20001/chrome/test/remoting/htt... chrome/test/remoting/http_server/http_server.py:56: message['keypressText'] = self.keytest_text On 2014/03/04 21:40:35, Jamie wrote: > I think this can be more succinctly declared similarly to TEST_DICT below (ie, > as one statement, not three). Done. https://codereview.chromium.org/180273015/diff/20001/chrome/test/remoting/htt... chrome/test/remoting/http_server/http_server.py:59: if self.keytest_succeeded: On 2014/03/04 21:40:35, Jamie wrote: > Is this server single-threaded? If not, then you'll need some guards around > these members since they might be accessed concurrently by different requests. > Better still, configure it so that it is single-threaded. Done. Added server.threadpool = 1 in config below.
LGTM, thanks. https://codereview.chromium.org/180273015/diff/20001/chrome/test/remoting/htt... File chrome/test/remoting/http_server/clientpage.js (right): https://codereview.chromium.org/180273015/diff/20001/chrome/test/remoting/htt... chrome/test/remoting/http_server/clientpage.js:17: request.open('GET', '/poll?test=keytest', true); On 2014/03/05 22:02:22, chaitali wrote: > Removed the /. The rest of it needs to remain so cherrypy can direct this to the > poll handler. Is that what you meant by relative URL? > > On 2014/03/04 21:40:35, Jamie wrote: > > Can we use a relative URL here too? > Yes, that's what I had in mind. https://codereview.chromium.org/180273015/diff/20001/chrome/test/remoting/htt... chrome/test/remoting/http_server/clientpage.js:31: setTimeout(poll, 3000); On 2014/03/05 22:02:22, chaitali wrote: > Changed to 1s. > > I thought about long polling but what I want from the framework is that it polls > and gets a list of status vars from the server. As the host completes actions, > statuses will change. So it is possible that keypressSucceeded is true but > mouseSucceeded is false at some given time. With regular polling we'll receive > regular status of all vars and tests can complete as the statuses come in. With > long polling we'll have to wait till all the vars are true on the server and > block tests. > > What I would really like is Server Sent Events but not sure how to do that here. > > On 2014/03/04 21:40:35, Jamie wrote: > > 3s seems like quite a long time to wait. I think we could avoid a wait > > altogether using a hanging GET, but in the interim, maybe just reduce this to > > 1s? > This is fine for now, but let's talk off-line about improving it. It's possible I've missed something obvious, but a hanging GET approach seems to be better than polling, even for the scenario you describe. https://codereview.chromium.org/180273015/diff/40001/chrome/test/remoting/htt... File chrome/test/remoting/http_server/clientpage.js (right): https://codereview.chromium.org/180273015/diff/40001/chrome/test/remoting/htt... chrome/test/remoting/http_server/clientpage.js:53: function(){ Nit: space before '{'. Or just use poll.bind(null) instead of declaring an anonymous function.
Fixed lint issues reported by gjslint and addressed additional comment. Leaving the polling as is for now. Will change to a better method after offline discussion with Jamie. https://codereview.chromium.org/180273015/diff/40001/chrome/test/remoting/htt... File chrome/test/remoting/http_server/clientpage.js (right): https://codereview.chromium.org/180273015/diff/40001/chrome/test/remoting/htt... chrome/test/remoting/http_server/clientpage.js:53: function(){ On 2014/03/06 00:15:01, Jamie wrote: > Nit: space before '{'. Or just use poll.bind(null) instead of declaring an > anonymous function. Done.
The CQ bit was checked by chaitali@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/chaitali@chromium.org/180273015/50001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel Retried try job too often on win_rel for step(s) base_unittests, browser_tests, interactive_ui_tests, net_unittests, unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
The CQ bit was checked by jamiewalch@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/chaitali@chromium.org/180273015/50001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel
The CQ bit was checked by chaitali@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/chaitali@chromium.org/180273015/50001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/chaitali@chromium.org/180273015/50001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel
The CQ bit was checked by chaitali@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/chaitali@chromium.org/180273015/50001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel
The CQ bit was checked by chaitali@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/chaitali@chromium.org/180273015/50001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/chaitali@chromium.org/180273015/50001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/chaitali@chromium.org/180273015/50001
Message was sent while issue was closed.
Change committed as 255771
Message was sent while issue was closed.
Drive-by (chrome/test/OWNERS): why yet another HTTP server? Please note we have net/test/embedded_test_server and spawned_test_server (and net/tools/testserver if you're not using C++ but want to use python directly; otherwise spawned_test_server is a wrapper around that python code). I recommend embedded_test_server for all new code. These test servers are not perfect, but I'd rather fix them as needed rather that have everyone create their own server. It's not hard to imagine where that would lead to. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
