The default bug view has changed. See this FAQ.

Clean up the code for manifest parsing

RESOLVED FIXED in mozilla33

Status

Testing
Mochitest
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: vaibhav1994, Assigned: vaibhav1994)

Tracking

(Blocks: 1 bug)

Trunk
mozilla33
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
The code for manifest parsing is duplicated in runtests.py

BuildTestPath method: http://dxr.mozilla.org/mozilla-central/source/testing/mochitest/runtests.py#469

getDirectories method: http://dxr.mozilla.org/mozilla-central/source/testing/mochitest/runtests.py#1743

We need to clean this up and make a common method to parse manifests.
(Assignee)

Updated

3 years ago
Assignee: nobody → vaibhavmagarwal
Thanks for filing this!
(Assignee)

Updated

3 years ago
Blocks: 1036372
(Assignee)

Comment 2

3 years ago
Created attachment 8453857 [details] [diff] [review]
manifestcleanup.patch

Cleaned up the getDirectories() method.
Attachment #8453857 - Flags: review?(jmaher)
Comment on attachment 8453857 [details] [diff] [review]
manifestcleanup.patch

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

A couple of questions, but overall this looks like a great cleanup!

::: testing/mochitest/runtests.py
@@ -1879,4 @@
>      for test in tests:
> -      pathAbs = os.path.abspath(test['path'])
> -      assert pathAbs.startswith(self.testRootAbs)
> -      tp = pathAbs[len(self.testRootAbs):].replace('\\', '/').strip('/')

I had some issues on windows while developing runbydir, maybe this is not needed anymore!  I assume this works well on windows?

@@ -1885,4 @@
>        if rootdir not in dirlist:
>          dirlist.append(rootdir)
>  
> -    dirlist.sort()

why are you removing this?
Attachment #8453857 - Flags: review?(jmaher) → review+
(Assignee)

Comment 4

3 years ago
(In reply to Joel Maher (:jmaher) from comment #3)
> Comment on attachment 8453857 [details] [diff] [review]
> manifestcleanup.patch
> 
> Review of attachment 8453857 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> A couple of questions, but overall this looks like a great cleanup!
> 
> ::: testing/mochitest/runtests.py
> @@ -1879,4 @@
> >      for test in tests:
> > -      pathAbs = os.path.abspath(test['path'])
> > -      assert pathAbs.startswith(self.testRootAbs)
> > -      tp = pathAbs[len(self.testRootAbs):].replace('\\', '/').strip('/')
> 
> I had some issues on windows while developing runbydir, maybe this is not
> needed anymore!  I assume this works well on windows?

Yes this works on windows. Those lines change '\\' to '/' which is already done in the getActiveTests method: http://dxr.mozilla.org/mozilla-central/source/testing/mochitest/runtests.py#1356 , so need to do it again.

> 
> @@ -1885,4 @@
> >        if rootdir not in dirlist:
> >          dirlist.append(rootdir)
> >  
> > -    dirlist.sort()
> 
> why are you removing this?

We are already sorting the list in getActiveTests method: http://dxr.mozilla.org/mozilla-central/source/testing/mochitest/runtests.py#1376 , so this will be a repeat.
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
Do you have a Try link handy for this patch? :)
Keywords: checkin-needed
(Assignee)

Comment 6

3 years ago
Successful Try Run: https://tbpl.mozilla.org/?tree=Try&rev=1552708e3529
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/98dffc10c25c
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/98dffc10c25c
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.