Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
14757019:
Add browser controller and allow it to be used under a flag. (Closed)
|
Created:
7 years, 7 months ago by ricow1 Modified:
7 years, 7 months ago Reviewers:
kustermann CC:
reviews_dartlang.org Visibility:
Public. |
DescriptionAdd browser controller and allow it to be used under a flag.
This is currently chrome and firefox only.
I am still seeing a race condition occasionally, which is the reason
for the rather verbose prints I do when failing.
I have not added prefetching yet, I will do that as a next step.
R=kustermann@google.com
Committed: https://code.google.com/p/dart/source/detail?r=22675
Patch Set 1 #
Total comments: 54
Patch Set 2 : #
Total comments: 2
Patch Set 3 : #Patch Set 4 : #
Total comments: 18
Messages
Total messages: 5 (0 generated)
Mostly style comments, but two things are important a) If we have a timeout, kill the browser first, wait until it's gone and then start a new one. b) Make sure that the changes to test_controller.js still work with the old selenium runner. So we can do a step-wise migration to the new browser controller. And I've one more concern/question: Is test_controller.js only used by our testing scripts or is it also used by the unittest framework? Maybe we should just make a copy of test_controller.js and put it into 'tools/testing/dart/resources' or so? https://codereview.chromium.org/14757019/diff/1/pkg/unittest/lib/test_control... File pkg/unittest/lib/test_controller.js (right): https://codereview.chromium.org/14757019/diff/1/pkg/unittest/lib/test_control... pkg/unittest/lib/test_controller.js:37: if (window.opener) { I'm not 100% sure, but I think we do have 'window.opener' even if we use selenium to run our tests. Could this conflict with the selenium stuff? https://codereview.chromium.org/14757019/diff/1/tools/testing/dart/browser_co... File tools/testing/dart/browser_controler.dart (right): https://codereview.chromium.org/14757019/diff/1/tools/testing/dart/browser_co... tools/testing/dart/browser_controler.dart:1: // Copyright (c) 2012, the Dart project authors. Please see the AUTHORS file 2013 https://codereview.chromium.org/14757019/diff/1/tools/testing/dart/browser_co... tools/testing/dart/browser_controler.dart:8: import "dart:core"; You could sort them. https://codereview.chromium.org/14757019/diff/1/tools/testing/dart/browser_co... tools/testing/dart/browser_controler.dart:90: _killIt(nextKillFunction, --retries).then((success) { Why not 'retries -1'? https://codereview.chromium.org/14757019/diff/1/tools/testing/dart/browser_co... tools/testing/dart/browser_controler.dart:151: }); Indentation. https://codereview.chromium.org/14757019/diff/1/tools/testing/dart/browser_co... tools/testing/dart/browser_controler.dart:184: const String binary = "google-chrome"; We probably need to adjust the path to the binary on bots (in particular for windows). https://codereview.chromium.org/14757019/diff/1/tools/testing/dart/browser_co... tools/testing/dart/browser_controler.dart:313: int maxNumBrowsers; I think we should always use '_' for private members. It makes the code easier to read IMHO. https://codereview.chromium.org/14757019/diff/1/tools/testing/dart/browser_co... tools/testing/dart/browser_controler.dart:362: doubleReportingTests.add(testId); Maybe we should make the buildbot red in case of "double reports"? https://codereview.chromium.org/14757019/diff/1/tools/testing/dart/browser_co... tools/testing/dart/browser_controler.dart:401: browser.start(testingServer.getDriverUrl(id)).then((success) { IMHO this is not the way we should do it. We should first kill the old browser, wait until it's gone and then kick off a new browser. In the mobile case it just doesn't work [since we can't have multiple browsers open] and it's also not a clean way to do it this way. What if the old browser and the new browser request new tasks at the same time (with the same browserId)? https://codereview.chromium.org/14757019/diff/1/tools/testing/dart/browser_co... tools/testing/dart/browser_controler.dart:498: const String server = "127.0.0.1"; Please make a comment descripting the API (see for example http_server.dart) https://codereview.chromium.org/14757019/diff/1/tools/testing/dart/browser_co... tools/testing/dart/browser_controler.dart:535: request.response.write(textResponse); It is important to drain the request stream (even if it is just a GET request) in order for 'dart:io' to close the socket. https://codereview.chromium.org/14757019/diff/1/tools/testing/dart/browser_co... tools/testing/dart/browser_controler.dart:536: request.response.close(); I'm not 100% sure if we need to catch the future here as well? https://codereview.chromium.org/14757019/diff/1/tools/testing/dart/browser_co... tools/testing/dart/browser_controler.dart:537: request.response.done.catchError((error) { Indentation https://codereview.chromium.org/14757019/diff/1/tools/testing/dart/browser_co... tools/testing/dart/browser_controler.dart:555: request.transform(new StringDecoder()).listen((data) { You could use 'fold()'. https://codereview.chromium.org/14757019/diff/1/tools/testing/dart/browser_co... tools/testing/dart/browser_controler.dart:601: if(this.status == 200) { spaces after 'if' https://codereview.chromium.org/14757019/diff/1/tools/testing/dart/browser_co... tools/testing/dart/browser_controler.dart:609: var split = this.responseText.split('#'); Please document how the response text looks like. https://codereview.chromium.org/14757019/diff/1/tools/testing/dart/test_runne... File tools/testing/dart/test_runner.dart (right): https://codereview.chromium.org/14757019/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:21: import "browser_controler.dart"; browser_controler -> browser_controller + file rename. https://codereview.chromium.org/14757019/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:352: bool get usesBrowserControler => configuration['use_browser_controller']; Controler -> Controller https://codereview.chromium.org/14757019/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:1374: _browserTestRunners = new Map<String, BrowserTestRunner>(){ space before { https://codereview.chromium.org/14757019/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:1583: Future<BrowserTestRunner> getBrowserTestRunner(TestCase test) { You could make it private as well. https://codereview.chromium.org/14757019/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:1586: var testRunner = new BrowserTestRunner(runtime, 4); Make a constant. https://codereview.chromium.org/14757019/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:1592: return null; If we're unable to start the browser then we'll get a null pointer exception further down. Maybe we should just exit() and print a appropriate error message. https://codereview.chromium.org/14757019/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:1598: void startBrowserControllerTest(var test) { private? https://codereview.chromium.org/14757019/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:1611: output.codeUnits, Please use encodeUtf8String or so. This is supposed to be a list of bytes not a list of unicode points. https://codereview.chromium.org/14757019/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:1619: test.timeout); Does this fit on one line? https://codereview.chromium.org/14757019/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:1627: // We oversubscribe the cpus with the started browsers when we use our This comment seems to be wrong. https://codereview.chromium.org/14757019/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:1705: if (test.usesBrowserControler) { I'm not sure if we should make this an attribute of TestCase. It seems to me that 'usesBrowserControler' is a configuration wide setting. Maybe we should move this attribute to the configuration?
https://codereview.chromium.org/14757019/diff/1/tools/testing/dart/browser_co... File tools/testing/dart/browser_controler.dart (right): https://codereview.chromium.org/14757019/diff/1/tools/testing/dart/browser_co... tools/testing/dart/browser_controler.dart:1: // Copyright (c) 2012, the Dart project authors. Please see the AUTHORS file On 2013/05/13 16:00:00, kustermann wrote: > 2013 Done. https://codereview.chromium.org/14757019/diff/1/tools/testing/dart/browser_co... tools/testing/dart/browser_controler.dart:8: import "dart:core"; On 2013/05/13 16:00:00, kustermann wrote: > You could sort them. Done. https://codereview.chromium.org/14757019/diff/1/tools/testing/dart/browser_co... tools/testing/dart/browser_controler.dart:90: _killIt(nextKillFunction, --retries).then((success) { On 2013/05/13 16:00:00, kustermann wrote: > Why not 'retries -1'? Done. https://codereview.chromium.org/14757019/diff/1/tools/testing/dart/browser_co... tools/testing/dart/browser_controler.dart:151: }); On 2013/05/13 16:00:00, kustermann wrote: > Indentation. Done. https://codereview.chromium.org/14757019/diff/1/tools/testing/dart/browser_co... tools/testing/dart/browser_controler.dart:184: const String binary = "google-chrome"; On 2013/05/13 16:00:00, kustermann wrote: > We probably need to adjust the path to the binary on bots (in particular for > windows). Yes, we may https://codereview.chromium.org/14757019/diff/1/tools/testing/dart/browser_co... tools/testing/dart/browser_controler.dart:313: int maxNumBrowsers; On 2013/05/13 16:00:00, kustermann wrote: > I think we should always use '_' for private members. It makes the code easier > to read IMHO. That depends on how easy you want testing to be https://codereview.chromium.org/14757019/diff/1/tools/testing/dart/browser_co... tools/testing/dart/browser_controler.dart:362: doubleReportingTests.add(testId); On 2013/05/13 16:00:00, kustermann wrote: > Maybe we should make the buildbot red in case of "double reports"? I think that would make sense yes, but until the unit testing framework has been fixed I don't want to do this. Additionally, I think we should do it in the termination step, that way we can actually report all the double reporting tests in one go, and then exit(1) (or return false from the future) https://codereview.chromium.org/14757019/diff/1/tools/testing/dart/browser_co... tools/testing/dart/browser_controler.dart:401: browser.start(testingServer.getDriverUrl(id)).then((success) { On 2013/05/13 16:00:00, kustermann wrote: > IMHO this is not the way we should do it. We should first kill the old browser, > wait until it's gone and then kick off a new browser. > > In the mobile case it just doesn't work [since we can't have multiple browsers > open] and it's also not a clean way to do it this way. What if the old browser > and the new browser request new tasks at the same time (with the same > browserId)? I did not really think about the mobile case here, more on getting the next browser up and running as fast as possible so that we can continue testing. Given the issue on mobile and the relatively small gain from this I will change the order https://codereview.chromium.org/14757019/diff/1/tools/testing/dart/browser_co... tools/testing/dart/browser_controler.dart:498: const String server = "127.0.0.1"; On 2013/05/13 16:00:00, kustermann wrote: > Please make a comment descripting the API (see for example http_server.dart) Done. https://codereview.chromium.org/14757019/diff/1/tools/testing/dart/browser_co... tools/testing/dart/browser_controler.dart:535: request.response.write(textResponse); On 2013/05/13 16:00:00, kustermann wrote: > It is important to drain the request stream (even if it is just a GET request) > in order for 'dart:io' to close the socket. Done. https://codereview.chromium.org/14757019/diff/1/tools/testing/dart/browser_co... tools/testing/dart/browser_controler.dart:536: request.response.close(); On 2013/05/13 16:00:00, kustermann wrote: > I'm not 100% sure if we need to catch the future here as well? The checked in binary does not return a future on close (afaik), that is why it is done below on the done future. That future is the same as returned by close() https://codereview.chromium.org/14757019/diff/1/tools/testing/dart/browser_co... tools/testing/dart/browser_controler.dart:537: request.response.done.catchError((error) { On 2013/05/13 16:00:00, kustermann wrote: > Indentation Done. https://codereview.chromium.org/14757019/diff/1/tools/testing/dart/browser_co... tools/testing/dart/browser_controler.dart:537: request.response.done.catchError((error) { On 2013/05/13 16:00:00, kustermann wrote: > Indentation Done. https://codereview.chromium.org/14757019/diff/1/tools/testing/dart/browser_co... tools/testing/dart/browser_controler.dart:555: request.transform(new StringDecoder()).listen((data) { On 2013/05/13 16:00:00, kustermann wrote: > You could use 'fold()'. No can do. Checked in bin: r20101, fold introduced: r21258 https://codereview.chromium.org/14757019/diff/1/tools/testing/dart/browser_co... tools/testing/dart/browser_controler.dart:601: if(this.status == 200) { On 2013/05/13 16:00:00, kustermann wrote: > spaces after 'if' Done, plus above (no stealing my signature comment :-)) https://codereview.chromium.org/14757019/diff/1/tools/testing/dart/browser_co... tools/testing/dart/browser_controler.dart:609: var split = this.responseText.split('#'); On 2013/05/13 16:00:00, kustermann wrote: > Please document how the response text looks like. Done. https://codereview.chromium.org/14757019/diff/1/tools/testing/dart/test_runne... File tools/testing/dart/test_runner.dart (right): https://codereview.chromium.org/14757019/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:21: import "browser_controler.dart"; On 2013/05/13 16:00:00, kustermann wrote: > browser_controler -> browser_controller + file rename. Done. https://codereview.chromium.org/14757019/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:352: bool get usesBrowserControler => configuration['use_browser_controller']; On 2013/05/13 16:00:00, kustermann wrote: > Controler -> Controller Done. https://codereview.chromium.org/14757019/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:1374: _browserTestRunners = new Map<String, BrowserTestRunner>(){ On 2013/05/13 16:00:00, kustermann wrote: > space before { Done. https://codereview.chromium.org/14757019/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:1583: Future<BrowserTestRunner> getBrowserTestRunner(TestCase test) { On 2013/05/13 16:00:00, kustermann wrote: > You could make it private as well. Done. https://codereview.chromium.org/14757019/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:1586: var testRunner = new BrowserTestRunner(runtime, 4); On 2013/05/13 16:00:00, kustermann wrote: > Make a constant. Done. https://codereview.chromium.org/14757019/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:1592: return null; On 2013/05/13 16:00:00, kustermann wrote: > If we're unable to start the browser then we'll get a null pointer exception > further down. Maybe we should just exit() and print a appropriate error message. Done. https://codereview.chromium.org/14757019/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:1592: return null; On 2013/05/13 16:00:00, kustermann wrote: > If we're unable to start the browser then we'll get a null pointer exception > further down. Maybe we should just exit() and print a appropriate error message. Done https://codereview.chromium.org/14757019/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:1598: void startBrowserControllerTest(var test) { On 2013/05/13 16:00:00, kustermann wrote: > private? Done. https://codereview.chromium.org/14757019/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:1611: output.codeUnits, On 2013/05/13 16:00:00, kustermann wrote: > Please use encodeUtf8String or so. This is supposed to be a list of bytes not a > list of unicode points. Done. https://codereview.chromium.org/14757019/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:1619: test.timeout); On 2013/05/13 16:00:00, kustermann wrote: > Does this fit on one line? Yes https://codereview.chromium.org/14757019/diff/1/tools/testing/dart/test_runne... tools/testing/dart/test_runner.dart:1705: if (test.usesBrowserControler) { On 2013/05/13 16:00:00, kustermann wrote: > I'm not sure if we should make this an attribute of TestCase. It seems to me > that 'usesBrowserControler' is a configuration wide setting. Maybe we should > move this attribute to the configuration? It is part of the configuration, see how we return it from the test case. I do, however, think that this provides the best flexibility, we could change the way we do this right? Additionally, this is consistent with how we do it for other stuff (usesWebDriver etc)
lgtm with comments Hopefully I didn't miss anything (it was a long CL). https://codereview.chromium.org/14757019/diff/8001/pkg/unittest/lib/test_cont... File pkg/unittest/lib/test_controller.js (right): https://codereview.chromium.org/14757019/diff/8001/pkg/unittest/lib/test_cont... pkg/unittest/lib/test_controller.js:36: function sendBackData() { You could rename it to a more descriptive function: "sendDomToTestDriver()" or so. https://codereview.chromium.org/14757019/diff/8001/tools/testing/dart/test_ru... File tools/testing/dart/test_runner.dart (right): https://codereview.chromium.org/14757019/diff/8001/tools/testing/dart/test_ru... tools/testing/dart/test_runner.dart:1596: exit(1); We could use a utility function die/fatal in utils.py: void die(message) { print(message); exit(1); } This way we could easily add the 'post fatal errors to an external service'-functionality there as well. https://codereview.chromium.org/14757019/diff/11002/tools/testing/dart/browse... File tools/testing/dart/browser_controller.dart (right): https://codereview.chromium.org/14757019/diff/11002/tools/testing/dart/browse... tools/testing/dart/browser_controller.dart:27: Function _cleanup; We have a ton of callback in this code. In this case for example you could make it a completer (and rename this to 'browserDidExitFuture'). Everyone who is interested in it could register for the event. https://codereview.chromium.org/14757019/diff/11002/tools/testing/dart/browse... tools/testing/dart/browser_controller.dart:52: _usageLog.write(toLog); a) Is the 'debugPrint' still used? b) You don't write \n' here and the call sites don't do it either? https://codereview.chromium.org/14757019/diff/11002/tools/testing/dart/browse... tools/testing/dart/browser_controller.dart:83: completer.complete(false); What about calling utils.py:die("....")? https://codereview.chromium.org/14757019/diff/11002/tools/testing/dart/browse... tools/testing/dart/browser_controller.dart:100: completer.complete(true); You could set 'process = null' here. https://codereview.chromium.org/14757019/diff/11002/tools/testing/dart/browse... tools/testing/dart/browser_controller.dart:135: return Process.start(command, arguments).then((p) { I have no strong opinion about this, but you told me we don't abbreviate? (p, e ...) https://codereview.chromium.org/14757019/diff/11002/tools/testing/dart/browse... tools/testing/dart/browser_controller.dart:158: }); Indentation. https://codereview.chromium.org/14757019/diff/11002/tools/testing/dart/browse... tools/testing/dart/browser_controller.dart:171: String get usageLog => _usageLog.toString(); Where are these three variables used? https://codereview.chromium.org/14757019/diff/11002/tools/testing/dart/browse... tools/testing/dart/browser_controller.dart:272: BrowserTest currentlyRunning; 'currentlyRunning' feels like it's a boolean or so. You could rename it to 'currentTest'. https://codereview.chromium.org/14757019/diff/11002/tools/testing/dart/browse... tools/testing/dart/browser_controller.dart:276: BrowserTest last; last -> lastTest https://codereview.chromium.org/14757019/diff/11002/tools/testing/dart/browse... tools/testing/dart/browser_controller.dart:287: Function doneCallback; Again, we could make it a completer and let the test_runner just do "runTest().then((result) {})". https://codereview.chromium.org/14757019/diff/11002/tools/testing/dart/browse... tools/testing/dart/browser_controller.dart:319: new Map<String, BrowserTestingStatus>(); The type of the variable should be inferred -- no need to note it twice? https://codereview.chromium.org/14757019/diff/11002/tools/testing/dart/browse... tools/testing/dart/browser_controller.dart:398: var browser = getInstance(); The comment is outdated now + nove the 'getInstance()' inside 'status.browser.close().then((_) { ....}' https://codereview.chromium.org/14757019/diff/11002/tools/testing/dart/browse... tools/testing/dart/browser_controller.dart:409: browser.close().then((success) { If we're 'underTermination' then the 'browser.close()' is already called in the 'terminate()' function. https://codereview.chromium.org/14757019/diff/11002/tools/testing/dart/browse... tools/testing/dart/browser_controller.dart:415: return; indentation https://codereview.chromium.org/14757019/diff/11002/tools/testing/dart/browse... tools/testing/dart/browser_controller.dart:450: exit(1); utils.py:die() ? https://codereview.chromium.org/14757019/diff/11002/tools/testing/dart/browse... tools/testing/dart/browser_controller.dart:470: } Add a TODO to 'die()' in the future if we get double reportings. https://codereview.chromium.org/14757019/diff/11002/tools/testing/dart/browse... tools/testing/dart/browser_controller.dart:508: /// /reportPath: Used for shipping back the result of running a test. This is not the interface. Please document exactly how things work (it's even hard for me to figure it out). Something like GET /driver/BROWSER_ID -- ... GET /next_test/BROWSER_ID -- returns "WAIT" "TERMINATE" or ... "...#id' POST /report/BROWSER_ID?id=NUM -- ... https://codereview.chromium.org/14757019/diff/11002/tools/testing/dart/browse... tools/testing/dart/browser_controller.dart:616: } else if (this.responseText == 'TERMINATE') { Use the constant above (i.e. " == '$terminateSignal').
Message was sent while issue was closed.
Committed patchset #4 manually as r22675 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
