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)

Product:
Testing
Component:
Mochitest
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox39 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla39
Tracking Flags:
Tracking Status
firefox39 --- fixed

People

(Reporter: vaibhav1994, Assigned: vaibhav1994)

References

(Depends on 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

cleanup.patch
(deleted), patch
ahal
: review+
jmaher
: review+
Details | Diff | Splinter Review
No description provided.
Now that bug 1083347 is fixed, we should remove the code that was specific to .json manifests
Attached patch cleanup.patch (obsolete) (deleted) — — Splinter Review
Attached patch cleanup.patch (obsolete) (deleted) — — Splinter Review
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)
Attached patch cleanup.patch (obsolete) (deleted) — — Splinter Review
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)
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+
Attached patch cleanup.patch (deleted) — — Splinter Review
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 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+
lets get a bug on file for the ipc.json and manifestlibrary.js work.
I have filed bug 1147841 for this.
Depends on: 1147841
Comment on attachment 8583769 [details] [diff] [review] cleanup.patch Review of attachment 8583769 [details] [diff] [review]: ----------------------------------------------------------------- Nice, thanks!
Attachment #8583769 - Flags: review?(ahalberstadt) → review+
Keywords: checkin-needed
Assignee: nobody → vaibhavmagarwal
https://hg.mozilla.org/mozilla-central/rev/37b979f09055
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
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.
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.

Attachment

General

Creator:
Created:
Updated:
Size: