Cleanup code related to .json manifests (especially android) in mochitests

RESOLVED FIXED in Firefox 39

Status

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: vaibhav1994, Assigned: vaibhav1994)

Tracking

(Depends on 1 bug)

unspecified
mozilla39
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

No description provided.
Now that bug 1083347 is fixed, we should remove the code that was specific to .json manifests
Posted patch cleanup.patch (obsolete) — Splinter Review
Posted patch cleanup.patch (obsolete) — 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)
Posted patch cleanup.patch (obsolete) — 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+
Posted patch cleanup.patchSplinter 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: 4 years ago
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.