Closed Bug 1091267 Opened 5 years ago Closed 5 years ago

Move automationutils:parseKeyValue to mochitest's runtests.py

Categories

(Testing :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla36

People

(Reporter: jgriffin, Assigned: bitgeeky, Mentored)

References

Details

(Whiteboard: [good first bug][lang=python])

User Story

We want to move the method parseKeyValue out of http://dxr.mozilla.org/mozilla-central/source/build/automationutils.py and into http://dxr.mozilla.org/mozilla-central/source/testing/mochitest/runtests.py.

Attachments

(1 file, 2 obsolete files)

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.
User Story: (updated)
I'll like to work on this one.
Assignee: nobody → mozpankaj1994
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.
Attached patch Bug-1091267.patch (obsolete) — Splinter Review
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.
Attachment #8515458 - Flags: review?(jgriffin)
Attachment #8515458 - Flags: review?(ahalberstadt)
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.
Attachment #8515458 - Flags: review?(jgriffin)
Attachment #8515458 - Flags: review?(ahalberstadt)
Attachment #8515458 - Flags: review+
Comment on attachment 8515458 [details] [diff] [review]
Bug-1091267.patch

Sorry, did that by mistake..
Attachment #8515458 - Flags: review+ → review-
Attached patch Bug-1091267-v2.patch (obsolete) — Splinter Review
Exactly ! Made the suggested change. Please review.
Attachment #8515458 - Attachment is obsolete: true
Attachment #8516066 - Flags: review?(ahalberstadt)
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
Attachment #8516066 - Flags: review?(ahalberstadt) → review+
Moved `KeyValueParseError` also, here is a properly formatted patch. Please review :-)
Attachment #8516066 - Attachment is obsolete: true
Attachment #8518094 - Flags: review?(ahalberstadt)
Comment on attachment 8518094 [details] [diff] [review]
Bug-1091267-v3.patch

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

Looks great, thanks!
Attachment #8518094 - Flags: review?(ahalberstadt) → review+
I think this is simple enough that a try run would just be a waste of resources.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c0dd96aa8ba5
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.