Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10933051:
WebDriver bindings. (Closed)
|
Created:
8 years, 3 months ago by gram Modified:
8 years, 3 months ago CC:
reviews_dartlang.org Visibility:
Public. |
DescriptionWebDriver bindings.
Committed: https://code.google.com/p/dart/source/detail?r=12409
Patch Set 1 #Patch Set 2 : #
Total comments: 36
Patch Set 3 : #
Total comments: 1
Messages
Total messages: 6 (0 generated)
+efortuna - adding Emily since she is very familiar with the webdriver APIs.
this is really exciting! Mostly minor comments. https://chromiumcodereview.appspot.com/10933051/diff/3001/pkg/webdriver/webdr... File pkg/webdriver/webdriver.dart (right): https://chromiumcodereview.appspot.com/10933051/diff/3001/pkg/webdriver/webdr... pkg/webdriver/webdriver.dart:24: * WebDriver. To get the id for an element yuou would first need to do typo: you https://chromiumcodereview.appspot.com/10933051/diff/3001/pkg/webdriver/webdr... pkg/webdriver/webdriver.dart:42: * }).chain((_) { Can we have synchronous versions of these calls instead or in addition? Right now as this is, this is way less readable (and cumbersome) than even the Java API. (yikes!) https://chromiumcodereview.appspot.com/10933051/diff/3001/pkg/webdriver/webdr... pkg/webdriver/webdriver.dart:65: String type; One of the biggest challenges with the current WebDriver bindings options is basically all the options are poorly documented, with the exception of Java, where you can find a number of example use cases online. You're off to a really great start on bucking that trend, given the documentation further on down the file. Let's document just a little more some of the other user-facing classes (like Errors). This will help make Dart an even more attractive language to use for browser testing! https://chromiumcodereview.appspot.com/10933051/diff/3001/pkg/webdriver/webdr... pkg/webdriver/webdriver.dart:81: type = [ consider making this type array a static constant rather than constructing it each time you need to access it, since we just use one string from it. https://chromiumcodereview.appspot.com/10933051/diff/3001/pkg/webdriver/webdr... pkg/webdriver/webdriver.dart:123: class WebDriverBase { The Documentation Santa should visit here. https://chromiumcodereview.appspot.com/10933051/diff/3001/pkg/webdriver/webdr... pkg/webdriver/webdriver.dart:152: _url = 'http://${_host}:${_port}${_path}'; nit, but I'm pretty sure this can just be written like: _url = 'http://$_host:$_port$_path'; or at the very least _url = 'http://${_host}:$_port$_path'; https://chromiumcodereview.appspot.com/10933051/diff/3001/pkg/webdriver/webdr... pkg/webdriver/webdriver.dart:159: * command If not defined in methods() this function will throw. use markdown syntax to link to the parameters: [command] . Also, methinks this indentation will be odd when we generate apidoc (the second line will be indented much more than the first) (?) You should probably check this. https://chromiumcodereview.appspot.com/10933051/diff/3001/pkg/webdriver/webdr... pkg/webdriver/webdriver.dart:169: successCodes = [ 200, 204 ]; can you move this up to the first line in the function like you did one line 134? [List successCodes = [200, 204], etc] (or have I been writing too much python?) https://chromiumcodereview.appspot.com/10933051/diff/3001/pkg/webdriver/webdr... pkg/webdriver/webdriver.dart:234: writeStringToFile('debug.txt', results); // TODO - remove change to TODO(gram): Remove. https://chromiumcodereview.appspot.com/10933051/diff/3001/pkg/webdriver/webdr... pkg/webdriver/webdriver.dart:295: * of {chrome|firefox|htmlunit|internet explorer|iphone}. safari or opera? https://chromiumcodereview.appspot.com/10933051/diff/3001/pkg/webdriver/webdr... pkg/webdriver/webdriver.dart:538: * StaleElementReference - If one of the script arguments is a WebElement These comments have a very javadoc-y feel. I think if you want separate paragraphs in the comments above, you need to separate the text with an empty line: * client. * * The script argument .... etc. Also if you want the errors listed, I think you need to indent them at least 4 spaces to form a separate block, and then you can add bullets if you want. https://chromiumcodereview.appspot.com/10933051/diff/3001/pkg/webdriver/webdr... pkg/webdriver/webdriver.dart:548: * asynchronous and must signal that is done by invoking the provided missing word "that it is" https://chromiumcodereview.appspot.com/10933051/diff/3001/pkg/webdriver/webdr... pkg/webdriver/webdriver.dart:601: */ noob question: can we spell out the acronym for IME? https://chromiumcodereview.appspot.com/10933051/diff/3001/pkg/webdriver/webdr... pkg/webdriver/webdriver.dart:750: * locator strategies that each server supports. "The table below lists possible values for [strategy] for how the server locates an element." (or something --- just make the connection to the "strategy" parameter a little more obvious) https://chromiumcodereview.appspot.com/10933051/diff/3001/pkg/webdriver/webdr... pkg/webdriver/webdriver.dart:806: findElementFromId(String id, String strategy, String searchValue) => this has crossed the => limit. I'd make it a function with { } https://chromiumcodereview.appspot.com/10933051/diff/3001/pkg/webdriver/webdr... pkg/webdriver/webdriver.dart:826: * Click on an element. "Click on an element that has the specified [id] name." https://chromiumcodereview.appspot.com/10933051/diff/3001/pkg/webdriver/webdr... pkg/webdriver/webdriver.dart:1353: delete extra lines down here
ptal https://chromiumcodereview.appspot.com/10933051/diff/3001/pkg/webdriver/webdr... File pkg/webdriver/webdriver.dart (right): https://chromiumcodereview.appspot.com/10933051/diff/3001/pkg/webdriver/webdr... pkg/webdriver/webdriver.dart:24: * WebDriver. To get the id for an element yuou would first need to do On 2012/09/12 20:15:53, Emily Fortuna wrote: > typo: you Done. https://chromiumcodereview.appspot.com/10933051/diff/3001/pkg/webdriver/webdr... pkg/webdriver/webdriver.dart:42: * }).chain((_) { On 2012/09/12 20:15:53, Emily Fortuna wrote: > Can we have synchronous versions of these calls instead or in addition? Right > now as this is, this is way less readable (and cumbersome) than even the Java > API. (yikes!) I don't believe so. The HTTP client APIs that Dart provides are async only; as (almost) every command here is a request/response, they are all async too. https://chromiumcodereview.appspot.com/10933051/diff/3001/pkg/webdriver/webdr... pkg/webdriver/webdriver.dart:65: String type; On 2012/09/12 20:15:53, Emily Fortuna wrote: > One of the biggest challenges with the current WebDriver bindings options is > basically all the options are poorly documented, with the exception of Java, > where you can find a number of example use cases online. You're off to a really > great start on bucking that trend, given the documentation further on down the > file. Let's document just a little more some of the other user-facing classes > (like Errors). > > This will help make Dart an even more attractive language to use for browser > testing! I added an error details field which should help. https://chromiumcodereview.appspot.com/10933051/diff/3001/pkg/webdriver/webdr... pkg/webdriver/webdriver.dart:81: type = [ On 2012/09/12 20:15:53, Emily Fortuna wrote: > consider making this type array a static constant rather than constructing it > each time you need to access it, since we just use one string from it. As far as I can tell Dart can't do this (yet) with things like lists. I worked around it by making them static and then initializing them on first use. https://chromiumcodereview.appspot.com/10933051/diff/3001/pkg/webdriver/webdr... pkg/webdriver/webdriver.dart:123: class WebDriverBase { On 2012/09/12 20:15:53, Emily Fortuna wrote: > The Documentation Santa should visit here. Done. https://chromiumcodereview.appspot.com/10933051/diff/3001/pkg/webdriver/webdr... pkg/webdriver/webdriver.dart:152: _url = 'http://${_host}:${_port}${_path}'; On 2012/09/12 20:15:53, Emily Fortuna wrote: > nit, but I'm pretty sure this can just be written like: > _url = 'http://$_host:$_port$_path'; > or at the very least > _url = 'http://${_host}:$_port$_path'; I'll change it, although I find the use of {} makes string interpolations more readable personally. https://chromiumcodereview.appspot.com/10933051/diff/3001/pkg/webdriver/webdr... pkg/webdriver/webdriver.dart:159: * command If not defined in methods() this function will throw. On 2012/09/12 20:15:53, Emily Fortuna wrote: > use markdown syntax to link to the parameters: [command] . Also, methinks this > indentation will be odd when we generate apidoc (the second line will be > indented much more than the first) (?) You should probably check this. Done. https://chromiumcodereview.appspot.com/10933051/diff/3001/pkg/webdriver/webdr... pkg/webdriver/webdriver.dart:169: successCodes = [ 200, 204 ]; On 2012/09/12 20:15:53, Emily Fortuna wrote: > can you move this up to the first line in the function like you did one line > 134? > [List successCodes = [200, 204], etc] (or have I been writing too much python?) It would be nice, but the Dart parser barfs on that. https://chromiumcodereview.appspot.com/10933051/diff/3001/pkg/webdriver/webdr... pkg/webdriver/webdriver.dart:234: writeStringToFile('debug.txt', results); // TODO - remove On 2012/09/12 20:15:53, Emily Fortuna wrote: > change to TODO(gram): Remove. Done. https://chromiumcodereview.appspot.com/10933051/diff/3001/pkg/webdriver/webdr... pkg/webdriver/webdriver.dart:295: * of {chrome|firefox|htmlunit|internet explorer|iphone}. On 2012/09/12 20:15:53, Emily Fortuna wrote: > safari or opera? I'm not sure. I am just quoting the WebDriver docs here. https://chromiumcodereview.appspot.com/10933051/diff/3001/pkg/webdriver/webdr... pkg/webdriver/webdriver.dart:538: * StaleElementReference - If one of the script arguments is a WebElement On 2012/09/12 20:15:53, Emily Fortuna wrote: > These comments have a very javadoc-y feel. I think if you want separate > paragraphs in the comments above, you need to separate the text with an empty > line: > * client. > * > * The script argument .... > etc. > Also if you want the errors listed, I think you need to indent them at least 4 > spaces to form a separate block, and then you can add bullets if you want. I've tried to reformat things in a way that will work with Dartdoc, wven if not always that elegantly. I also stripped down the comments on Potential Errors, as these are highly repetitive and don't really need the long generic explanations. https://chromiumcodereview.appspot.com/10933051/diff/3001/pkg/webdriver/webdr... pkg/webdriver/webdriver.dart:548: * asynchronous and must signal that is done by invoking the provided On 2012/09/12 20:15:53, Emily Fortuna wrote: > missing word "that it is" Done. https://chromiumcodereview.appspot.com/10933051/diff/3001/pkg/webdriver/webdr... pkg/webdriver/webdriver.dart:601: */ On 2012/09/12 20:15:53, Emily Fortuna wrote: > noob question: can we spell out the acronym for IME? Done. https://chromiumcodereview.appspot.com/10933051/diff/3001/pkg/webdriver/webdr... pkg/webdriver/webdriver.dart:750: * locator strategies that each server supports. On 2012/09/12 20:15:53, Emily Fortuna wrote: > "The table below lists possible values for [strategy] for how the server locates > an element." (or something --- just make the connection to the "strategy" > parameter a little more obvious) Done. https://chromiumcodereview.appspot.com/10933051/diff/3001/pkg/webdriver/webdr... pkg/webdriver/webdriver.dart:806: findElementFromId(String id, String strategy, String searchValue) => On 2012/09/12 20:15:53, Emily Fortuna wrote: > this has crossed the => limit. I'd make it a function with { } Done. https://chromiumcodereview.appspot.com/10933051/diff/3001/pkg/webdriver/webdr... pkg/webdriver/webdriver.dart:826: * Click on an element. On 2012/09/12 20:15:53, Emily Fortuna wrote: > "Click on an element that has the specified [id] name." Done.
lgtm, with the exception that I hope we can make some sync option available for people. :-P This is awesome! Are you going to submit a patch to selenium as well? I think you should -- it will really help make dart a good option for web testing, and if we can provide good examples and use cases online of how to use it, it would be a big win for dart+selenium. https://chromiumcodereview.appspot.com/10933051/diff/3001/pkg/webdriver/webdr... File pkg/webdriver/webdriver.dart (right): https://chromiumcodereview.appspot.com/10933051/diff/3001/pkg/webdriver/webdr... pkg/webdriver/webdriver.dart:42: * }).chain((_) { On 2012/09/12 22:42:09, gram wrote: > On 2012/09/12 20:15:53, Emily Fortuna wrote: > > Can we have synchronous versions of these calls instead or in addition? Right > > now as this is, this is way less readable (and cumbersome) than even the Java > > API. (yikes!) > > I don't believe so. The HTTP client APIs that Dart provides are async only; as > (almost) every command here is a request/response, they are all async too. I understand, but inside our implementation of setUrl, we could not return until we get a response, providing the illusion of a synchronous api. https://chromiumcodereview.appspot.com/10933051/diff/3001/pkg/webdriver/webdr... pkg/webdriver/webdriver.dart:152: _url = 'http://${_host}:${_port}${_path}'; On 2012/09/12 22:42:09, gram wrote: > On 2012/09/12 20:15:53, Emily Fortuna wrote: > > nit, but I'm pretty sure this can just be written like: > > _url = 'http://$_host:$_port$_path'; > > or at the very least > > _url = 'http://${_host}:$_port$_path'; > > I'll change it, although I find the use of {} makes string interpolations more > readable personally. Okay. Feel free to disregard that advice of mine if you prefer. https://chromiumcodereview.appspot.com/10933051/diff/3001/pkg/webdriver/webdr... pkg/webdriver/webdriver.dart:538: * StaleElementReference - If one of the script arguments is a WebElement On 2012/09/12 22:42:09, gram wrote: > On 2012/09/12 20:15:53, Emily Fortuna wrote: > > These comments have a very javadoc-y feel. I think if you want separate > > paragraphs in the comments above, you need to separate the text with an empty > > line: > > * client. > > * > > * The script argument .... > > etc. > > Also if you want the errors listed, I think you need to indent them at least 4 > > spaces to form a separate block, and then you can add bullets if you want. > > I've tried to reformat things in a way that will work with Dartdoc, wven if not > always that elegantly. I also stripped down the comments on Potential Errors, as > these are highly repetitive and don't really need the long generic explanations. awesome! https://chromiumcodereview.appspot.com/10933051/diff/6001/pkg/webdriver/test/... File pkg/webdriver/test/webdrivertest.dart (right): https://chromiumcodereview.appspot.com/10933051/diff/6001/pkg/webdriver/test/... pkg/webdriver/test/webdrivertest.dart:241: Future f = web_driver.newSession('htmlunit', { 'webStorageEnabled': true }); 80 char :-P |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
