Closed
Bug 1144573
Opened 10 years ago
Closed 10 years ago
Cleanup code related to .json manifests (especially android) in mochitests
Categories
(Testing :: Mochitest, defect)
Testing
Mochitest
Tracking
(firefox39 fixed)
RESOLVED
FIXED
mozilla39
Tracking | Status | |
---|---|---|
firefox39 | --- | fixed |
People
(Reporter: vaibhav1994, Assigned: vaibhav1994)
References
(Depends on 1 open bug)
Details
Attachments
(1 file, 3 obsolete files)
5.91 KB,
patch
|
ahal
:
review+
jmaher
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•10 years ago
|
||
Now that bug 1083347 is fixed, we should remove the code that was specific to .json manifests
Assignee | ||
Comment 2•10 years ago
|
||
Assignee | ||
Comment 3•10 years ago
|
||
Assignee | ||
Comment 4•10 years ago
|
||
Another follow-up patch push to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ae7c104da852
Assignee | ||
Comment 5•10 years ago
|
||
I have combined the patches for the try pushes into one. :ahal, since you have written the chunking code, I would also like you to review this patch. This does not remove --test-manifest, I will be removing it in another patch, since options.testManifest is used in many places.
Attachment #8583169 -
Attachment is obsolete: true
Attachment #8583622 -
Flags: review?(jmaher)
Attachment #8583622 -
Flags: review?(ahalberstadt)
Assignee | ||
Comment 6•10 years ago
|
||
Corrected a small nit in the patch
Attachment #8583622 -
Attachment is obsolete: true
Attachment #8583622 -
Flags: review?(jmaher)
Attachment #8583622 -
Flags: review?(ahalberstadt)
Attachment #8583632 -
Flags: review?(jmaher)
Attachment #8583632 -
Flags: review?(ahalberstadt)
Assignee | ||
Comment 7•10 years ago
|
||
A follow-up patch push to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f8c7cf0e2d9a
Comment 8•10 years ago
|
||
Comment on attachment 8583632 [details] [diff] [review]
cleanup.patch
Review of attachment 8583632 [details] [diff] [review]:
-----------------------------------------------------------------
I think this is a good first pass. There are probably specific parts of the code we can clean up after this. like --test-manifest and some lines of code in manifestLibrary.js.
Attachment #8583632 -
Flags: review?(jmaher) → review+
Assignee | ||
Comment 9•10 years ago
|
||
Thanks :jmaher. I have updated the patch to include --test-manifest (tested in the latest try run), so will request you to go through the updated code. Unfortunately, I can't remove code from manifestLibrary, since ipc.json uses the code. So we need to first remove that and then cleanup manifestLibrary.js
Attachment #8583632 -
Attachment is obsolete: true
Attachment #8583632 -
Flags: review?(ahalberstadt)
Attachment #8583769 -
Flags: review?(jmaher)
Attachment #8583769 -
Flags: review?(ahalberstadt)
Comment 10•10 years ago
|
||
Comment on attachment 8583769 [details] [diff] [review]
cleanup.patch
Review of attachment 8583769 [details] [diff] [review]:
-----------------------------------------------------------------
Good point about ipc.json, lets tackle that one after this cleanup is done :)
Attachment #8583769 -
Flags: review?(jmaher) → review+
Comment 11•10 years ago
|
||
lets get a bug on file for the ipc.json and manifestlibrary.js work.
Comment 13•10 years ago
|
||
Comment on attachment 8583769 [details] [diff] [review]
cleanup.patch
Review of attachment 8583769 [details] [diff] [review]:
-----------------------------------------------------------------
Nice, thanks!
Attachment #8583769 -
Flags: review?(ahalberstadt) → review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → vaibhavmagarwal
Comment 14•10 years ago
|
||
Keywords: checkin-needed
Comment 15•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Comment 16•10 years ago
|
||
There are also a few places in mozharness that still have --test-manifest:
https://dxr.mozilla.org/build:mozharness/search?q=%22--test-manifest%22&case=true&redirect=true
Now that mozharness is pegged, removing them is safe.
Comment 17•10 years ago
|
||
I filed bug 1150175. It turns out this issue is actually blocking some work I'm doing, so I'll take it.
You need to log in
before you can comment on or make changes to this bug.
Description
•