1091267 - Move automationutils:parseKeyValue to mochitest's runtests.py

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

[Jonathan Griffin (:jgriffin)]

automationutils:parseKeyValue is used in only one file, mochitest's runtests.py:

http://dxr.mozilla.org/mozilla-central/search?q=parseKeyValue+ext%3Apy&case=true&redirect=true

Let's move it there and delete it from automationutils.py.

Comment 1

[Pankaj Malhotra (:bitgeeky)]

I'll like to work on this one.

Comment 2

[Pankaj Malhotra (:bitgeeky)]

I made a quick grep search and found that we can also do the same for automationutils:KeyValueParseError , so making the same change here too.

Comment 3

[Pankaj Malhotra (:bitgeeky)]

Attachment: Bug-1091267.patch

I thought more on this, and realised that automationutils:KeyValueParseError is better placed in utils only. Please let me know what do you think for this ? For the time being I made the rest of the suggested changes. Please Review.

Comment 4

[Andrew Halberstadt [:ahal]]

Comment on attachment 8515458 [details] [diff] [review]
Bug-1091267.patch

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

I think the approach looks good, but there is a bug. Please fix and re-upload.

Thanks for the patch!

::: testing/mochitest/runtests.py
@@ +1065,5 @@
>      self.counttodo = 0
>  
>      self.expectedError = {}
>      self.result = {}
> +	

nit: extra whitespace here

@@ +1066,5 @@
>  
>      self.expectedError = {}
>      self.result = {}
> +	
> +  def parseKeyValue(strings, separator='=', context='key, value: '):

This will cause an error, you'll either need to move this function outside of the class definition, or add a 'self' keyword as the first argument (and update callers to use self.parseKeyValue). I'd vote for moving it outside of the class definition and into global scope.

Comment 5

[Andrew Halberstadt [:ahal]]

Comment on attachment 8515458 [details] [diff] [review]
Bug-1091267.patch

Sorry, did that by mistake..

Comment 6

[Pankaj Malhotra (:bitgeeky)]

Attachment: Bug-1091267-v2.patch

Exactly ! Made the suggested change. Please review.

Comment 7

[Andrew Halberstadt [:ahal]]

Comment on attachment 8516066 [details] [diff] [review]
Bug-1091267-v2.patch

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

Thanks again! :)

Please upload a new patch that has the proper author information and a commit message formatted according to [1]. Also if KeyValueParseError is only used in runtests.py, I think it would be good to move it over as well. Feel free to do that in the same patch if you want, I don't mind re-reviewing. Let me know if you have any questions! 

[1] https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 8

[Pankaj Malhotra (:bitgeeky)]

Attachment: Bug-1091267-v3.patch

Moved `KeyValueParseError` also, here is a properly formatted patch. Please review :-)

Comment 9

[Andrew Halberstadt [:ahal]]

Comment on attachment 8518094 [details] [diff] [review]
Bug-1091267-v3.patch

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

Looks great, thanks!

Comment 10

[Andrew Halberstadt [:ahal]]

I think this is simple enough that a try run would just be a waste of resources.

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c0dd96aa8ba5

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c0dd96aa8ba5