Closed Bug 1091270 Opened 5 years ago Closed 5 years ago

Move isURL out of automationutils

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

Replace the call sites for automationutils.py ( http://dxr.mozilla.org/mozilla-central/search?q=isURL+ext%3Apy&case=true ) with the actual function, then delete it from automationutils.

Attachments

(1 file, 2 obsolete files)

isURL is a single-line function used in only three places:

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

We should remove it from automationutils and replace it with the actual function in those places.
User Story: (updated)
Whiteboard: [good first bug][lang=python]
Hi, I contribute to the Web QA Automation projects and now willing to learn Firefox Automation too. I will like to work on this one.
Assignee: nobody → mozpankaj1994
Attached patch Bug-1091270.patch (obsolete) — Splinter Review
Made the suggested changes, Please let me know if it looks fine.
Attachment #8515390 - Flags: review?(jgriffin)
Attachment #8515390 - Flags: review?(ahalberstadt)
Comment on attachment 8515390 [details] [diff] [review]
Bug-1091270.patch

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

Looks good!  Can you just make the adjustment below in all places the method was copied to?

::: layout/tools/reftest/runreftest.py
@@ +788,5 @@
>  
>    if options.xrePath is None:
>      options.xrePath = os.path.dirname(options.app)
>  
> +  if options.symbolsPath and not len(urlparse(options.symbolsPath).scheme) >= 2:

I think it's a bit more readable to use "len() < 2", rather than "not len() >= 2"
Attachment #8515390 - Flags: review?(jgriffin) → review-
Attachment #8515390 - Flags: review?(ahalberstadt)
Attached patch Bug-1091270-v2.patch (obsolete) — Splinter Review
Agreed ! Made the suggested change. Please review.
Attachment #8515390 - Attachment is obsolete: true
Attachment #8516035 - Flags: review?(jgriffin)
Comment on attachment 8516035 [details] [diff] [review]
Bug-1091270-v2.patch

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

This looks great, but it needs to be rebased; it doesn't apply cleanly to mozilla-central any longer.  Can you attach a rebased version, please?  Thanks.
Attachment #8516035 - Flags: review?(jgriffin) → review-
Sure Thing :-) The last one was just a diff file, here is a properly formatted patch. Please review.
Attachment #8516035 - Attachment is obsolete: true
Attachment #8516624 - Flags: review?(jgriffin)
Comment on attachment 8516624 [details] [diff] [review]
Bug-1091270-v3.patch

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

lgtm, thanks!  pushed to try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=69598c0781bc
Attachment #8516624 - Flags: review?(jgriffin) → review+
https://hg.mozilla.org/mozilla-central/rev/585a22940ba7
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.