Made changes to the test architecture so that we can run frameworks other than

Made changes to the test architecture so that we can run frameworks other than
just Firefox from WebDriver.
Committed: https://code.google.com/p/dart/source/detail?r=1555

[Siggi Cherem (dart-lang), 2011-11-14 23:56:06]

http://codereview.chromium.org/8566021/diff/2001/tools/test.py
File tools/test.py (right):

http://codereview.chromium.org/8566021/diff/2001/tools/test.py#newcode1223
tools/test.py:1223: 'webdriver, webdriverff, webdriverie, webdriverchrome)',
I'm not sure about adding so many components. Maybe we can use '--flag' for
this. For example:

./tools/test.py -c webdriver --flag=chrome ...

http://codereview.chromium.org/8566021/diff/2001/tools/testing/architecture.py
File tools/testing/architecture.py (right):

http://codereview.chromium.org/8566021/diff/2001/tools/testing/architecture.p...
tools/testing/architecture.py:446: def GetRunCommand(self, browser='chrome',
fatal_static_type_errors=False):
this will possibly be not compatible with calls that pass these optional
arguments positionally (which the style guide disallows, but it's still
possible). I'd move this as the last argument.

Alternatively, you can make the constructor of WebDriverArchitecture take the
browser as an argument and store it in the object.

http://codereview.chromium.org/8566021/diff/2001/tools/testing/architecture.p...
tools/testing/architecture.py:457: class
FirefoxDriverArchitecture(WebDriverArchitecture):
if you end up adding the parameter to the constructor, there is little value in
adding these 2 types and you might want to use allocate
WebDriverArchitecture(..., browser = 'ff') directly in line 582.

http://codereview.chromium.org/8566021/diff/2001/tools/testing/architecture.p...
tools/testing/architecture.py:581: elif component == 'webdriverff':
it might be nice to use - or _ on these names, as long as it doesn't break the
commandline usage
(e.g. webdriver-ff)

http://codereview.chromium.org/8566021/diff/2001/tools/testing/run_selenium.py
File tools/testing/run_selenium.py (right):

http://codereview.chromium.org/8566021/diff/2001/tools/testing/run_selenium.p...
tools/testing/run_selenium.py:38: browser = ''
browser = ''
 ==>
browser = None

http://codereview.chromium.org/8566021/diff/2001/tools/testing/run_selenium.p...
tools/testing/run_selenium.py:41: if sys.argv[2] == 'ff':
if -> elif

http://codereview.chromium.org/8566021/diff/2001/tools/testing/run_selenium.p...
tools/testing/run_selenium.py:44: browser = selenium.webdriver.Ie()
else error?

[Emily Fortuna, 2011-11-15 00:38:07]

http://codereview.chromium.org/8566021/diff/2001/tools/test.py
File tools/test.py (right):

http://codereview.chromium.org/8566021/diff/2001/tools/test.py#newcode1223
tools/test.py:1223: 'webdriver, webdriverff, webdriverie, webdriverchrome)',
Sounds good. I like that better.

On 2011/11/14 23:56:06, sigmund wrote:
> I'm not sure about adding so many components. Maybe we can use '--flag' for
> this. For example:
> 
> ./tools/test.py -c webdriver --flag=chrome ...
>

http://codereview.chromium.org/8566021/diff/2001/tools/testing/run_selenium.py
File tools/testing/run_selenium.py (right):

http://codereview.chromium.org/8566021/diff/2001/tools/testing/run_selenium.p...
tools/testing/run_selenium.py:38: browser = ''
On 2011/11/14 23:56:06, sigmund wrote:
> browser = ''
>  ==>
> browser = None

Done.

http://codereview.chromium.org/8566021/diff/2001/tools/testing/run_selenium.p...
tools/testing/run_selenium.py:41: if sys.argv[2] == 'ff':
On 2011/11/14 23:56:06, sigmund wrote:
> if -> elif

Done.

http://codereview.chromium.org/8566021/diff/2001/tools/testing/run_selenium.p...
tools/testing/run_selenium.py:44: browser = selenium.webdriver.Ie()
On 2011/11/14 23:56:06, sigmund wrote:
> else error?

Done.

[Siggi Cherem (dart-lang), 2011-11-15 02:16:18]

lgtm (with suggestion below)

http://codereview.chromium.org/8566021/diff/4008/tools/testing/architecture.py
File tools/testing/architecture.py (right):

http://codereview.chromium.org/8566021/diff/4008/tools/testing/architecture.p...
tools/testing/architecture.py:544: def GetArchitecture(arch, mode, component,
test, flags):
One suggestion: we might be able to do this without this parameter. 

Since TestCase updates the architecture already to store the flag in
'self.vm_options', we could use that directly in 'GetRunCommand'. (vm_options
might be empty when allocating the architecture, but I believe it should have
the flag when 'GetRunCommand' is called). 

Note: if you decide to keep the flag, make sure to update run.py as well.