Closed Bug 1089771 Opened 7 years ago Closed 7 years ago

--chunk-by-dir mode is broken on Android

Categories

(Testing :: Mochitest, defect)

ARM
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla36

People

(Reporter: RyanVM, Assigned: vaibhav1994)

Details

Attachments

(2 files, 3 obsolete files)

I attempted a Try push today with --chunk-by-dir mode enabled on Android today, and all I got was this lousy^C^C^C^C^C^C^C^C^C^C a pile of 330s timeouts.
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=4c61763f7464

Joel suspects that this might be a regression from the recent Android manifest work.
B2G appears to be OK still.
Only the changes to androidx86.json have landed till now, so I think this is not related to changes in the manifest (bug 1083347), as the failures are occurring on android 2.3 opt and android 4.0 opt/debug. The fail-if work might be causing the issue, since there are some changes done to chunkifyTests.js: 
https://hg.mozilla.org/mozilla-central/rev/102371151dc6#l7.13
In the push, I see this error:

12:33:53     INFO -  10-28 12:27:54.632 I/GeckoDump( 2247): TEST-UNEXPECTED-FAIL: setup.js | error parsing http://mochi.test:8888/android.json (TypeError: invalid 'in' operand tests[i])
12:33:53     INFO -  10-28 12:27:54.640 W/GeckoConsole( 2247): [JavaScript Error: "TypeError: invalid 'in' operand tests[i]" {file: "http://mochi.test:8888/chunkifyTests.js" line: 18}]

So another push to fix this error: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=4f02279cc87e
Attached patch fix.patch (obsolete) — Splinter Review
The android jobs are green in the try push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=4f02279cc87e

:jmaher, could you review this patch used in the try push? Thanks!
Attachment #8513273 - Flags: review?(jmaher)
Assignee: nobody → vaibhavmagarwal
Comment on attachment 8513273 [details] [diff] [review]
fix.patch

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

thanks, this is how I believe it should be, can you look at bug 1085729 also, I suspect it should be a similar fix, another set of eyes would be good.
Attachment #8513273 - Flags: review?(jmaher) → review+
Keywords: checkin-needed
Attached patch fix.patchSplinter Review
Try push for the patch: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=889ad46ab9c3

:RyanVM, apologies for the noise. This patch is passing for both android and linux jobs. r+ from before (turns out a line that was changed was unnecessary). Please checkin if you feel everything is ok.
Attachment #8513273 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/2ba76e4bc640
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Still broken AFAICT.
https://treeherder.mozilla.org/ui/logviewer.html#?job_id=2863992&repo=try

Looks like your Try run didn't actually enable it?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla36 → ---
Attached patch fix_chunk.patch (obsolete) — Splinter Review
Oops I forgot to enable chunk-by-dir in the try push, sorry about that!

The new try push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=4a4fb0811ca4

:RyanVM, adding you as the reviewer.
Attachment #8517930 - Flags: review?(ryanvm)
Comment on attachment 8517930 [details] [diff] [review]
fix_chunk.patch

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

LGTM, but I'm not a peer for this code. Over to Joel :)
Attachment #8517930 - Flags: review?(ryanvm)
Attachment #8517930 - Flags: review?(jmaher)
Attachment #8517930 - Flags: feedback+
Comment on attachment 8517930 [details] [diff] [review]
fix_chunk.patch

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

::: testing/mochitest/chunkifyTests.js
@@ +17,5 @@
>      for (var i = 0; i < tests.length; ++i) {
>        if ((tests[i] instanceof Object) && ('test' in tests[i])) {
>          var test_path = tests[i]['test']['url'];
> +      }
> +      // This condition is needed to run --chunk-by-dir on mochitest bc and dt.

nit: I would prefer these comments to be inside the else block

@@ +24,5 @@
>        }
> +      // This condition is needed to run --chunk-by-dir on android chunks.
> +      else {
> +        var test_path = tests[i];
> +      }  

nit: trailing whitespace here.
Attachment #8517930 - Flags: review?(jmaher) → review+
Attached patch fix_chunk.patch (obsolete) — Splinter Review
Fixed the nits and carrying forward the r+.
Attachment #8517930 - Attachment is obsolete: true
Attachment #8518131 - Flags: review+
Attached patch fix_chunk.patchSplinter Review
Forgot to change the name of reviewer in commit message, changed that!
Attachment #8518131 - Attachment is obsolete: true
Attachment #8518133 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6740f4e7636a
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.