654461 - cleanup chrome and a11y harnesses to simplify code

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: cleanup chrome and a11y harness code (1.0)

in our mochitest-chrome and a11y harnesses there is a lot we can do to reduce unnecessary code and conditions.  This patch does a good deal of cleanup, some of which would be required if we wanted to fix bug 451630.

Comment 1

[cmtalbert]

Should this bug also block the remove .enablePrivilege bug? i.e. bug 462483?

Comment 2

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: cleanup chrome and a11y harness code (1.1)

updated patch to fix a bug in mochitest-plain, this patch looks really good on try server.

Comment 3

[(not currently active) Ted Mielczarek]

Comment on attachment 530000 [details] [diff] [review]
cleanup chrome and a11y harness code (1.1)

Review of attachment 530000 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, I just have a few things I'd like you to fix.

::: testing/mochitest/chrome-harness.js
@@ +373,5 @@
> +  var str = sstream.read(4096);
> +  while (str.length > 0) {
> +    config += str;
> +    str = sstream.read(4096);
> +  }

I think you can replace most of this block with NetUtil.readInputStreamToString():
https://developer.mozilla.org/en/JavaScript_code_modules/NetUtil.jsm#readInputStreamToString%28%29

@@ +377,5 @@
> +  }
> +
> +  sstream.close();
> +  fileInStream.close();
> +  return eval(config);

Feels like we should use JSON.parse() here instead.

::: testing/mochitest/runtests.py
@@ +669,5 @@
> +      testRoot = 'a11y'
> + 
> +    content = "({"
> +    content += 'testRoot: "%s", ' % (testRoot) 
> +    for opt in options.__dict__.keys():

This seems..unwieldy. I would suggest just using json.dumps(options.__dict__), but the json module isn't in Python 2.5. :-/ Maybe leave a comment here suggesting that we switch this to use the json module when we can use Python 2.6?

@@ +672,5 @@
> +    content += 'testRoot: "%s", ' % (testRoot) 
> +    for opt in options.__dict__.keys():
> +      val = options.__dict__[opt]
> +      content += opt + ": "
> +      if type(val).__name__ == 'bool':

This is really ugly. You should write this like:

if isinstance(val, bool):

@@ +674,5 @@
> +      val = options.__dict__[opt]
> +      content += opt + ": "
> +      if type(val).__name__ == 'bool':
> +        content += boolString(val) + ", "
> +      elif type(val).__name__ == 'NoneType':

elif val is None:

@@ +676,5 @@
> +      if type(val).__name__ == 'bool':
> +        content += boolString(val) + ", "
> +      elif type(val).__name__ == 'NoneType':
> +        content += '"", '
> +      elif type(val).__name__ == 'str':

elif isinstance(val, basestring):

@@ +678,5 @@
> +      elif type(val).__name__ == 'NoneType':
> +        content += '"", '
> +      elif type(val).__name__ == 'str':
> +        content += '"%s", ' % (val)
> +      elif type(val).__name__ == 'int':

elif isinstance(val, int):

@@ +680,5 @@
> +      elif type(val).__name__ == 'str':
> +        content += '"%s", ' % (val)
> +      elif type(val).__name__ == 'int':
> +        content += '%s, ' % (val)
> +      elif type(val).__name__ == 'list':

elif isinstance(val, list):

Comment 4

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: cleanup chrome and a11y harness code (2.0)

updated with feedback, green on try.

Comment 5

[Joel Maher ( :jmaher ) (UTC -8)]

http://hg.mozilla.org/mozilla-central/rev/10c4c19e0868