cleanup chrome and a11y harnesses to simplify code

RESOLVED FIXED

Status

Testing
General
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: jmaher, Assigned: jmaher)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

7 years ago
Created attachment 529736 [details] [diff] [review]
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.
(Assignee)

Updated

7 years ago
Attachment #529736 - Flags: review?(ted.mielczarek)

Comment 1

7 years ago
Should this bug also block the remove .enablePrivilege bug? i.e. bug 462483?
Blocks: 451630
(Assignee)

Comment 2

7 years ago
Created attachment 530000 [details] [diff] [review]
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.
Assignee: nobody → jmaher
Attachment #529736 - Attachment is obsolete: true
Attachment #529736 - Flags: review?(ted.mielczarek)
Attachment #530000 - Flags: review?(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):
Attachment #530000 - Flags: review?(ted.mielczarek) → review+
(Assignee)

Comment 4

7 years ago
Created attachment 532819 [details] [diff] [review]
cleanup chrome and a11y harness code (2.0)

updated with feedback, green on try.
Attachment #530000 - Attachment is obsolete: true
Attachment #532819 - Flags: review+
(Assignee)

Comment 5

7 years ago
http://hg.mozilla.org/mozilla-central/rev/10c4c19e0868
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.