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

RESOLVED FIXED in Firefox 39

Status

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)

Comment hidden (empty)
(Assignee)

Comment 1

4 years ago
Now that bug 1083347 is fixed, we should remove the code that was specific to .json manifests
(Assignee)

Comment 3

4 years ago
Created attachment 8583169 [details] [diff] [review]
cleanup.patch
(Assignee)

Comment 5

4 years ago
Created attachment 8583622 [details] [diff] [review]
cleanup.patch

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

4 years ago
Created attachment 8583632 [details] [diff] [review]
cleanup.patch

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+
(Assignee)

Comment 9

4 years ago
Created attachment 8583769 [details] [diff] [review]
cleanup.patch

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.
(Assignee)

Comment 12

4 years ago
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+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
(Assignee)

Updated

4 years ago
Assignee: nobody → vaibhavmagarwal
https://hg.mozilla.org/mozilla-central/rev/37b979f09055
Status: NEW → RESOLVED
Last Resolved: 4 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.