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

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 1

[Ryan VanderMeulen [:RyanVM]]

B2G appears to be OK still.

Comment 2

[Vaibhav (:vaibhav1994)]

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

Comment 3

[Vaibhav (:vaibhav1994)]

A test small fix to try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=079070659bbf

Comment 4

[Vaibhav (:vaibhav1994)]

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

Comment 5

[Vaibhav (:vaibhav1994)]

Attachment: fix.patch

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!

Comment 6

[Joel Maher ( :jmaher ) (UTC -8)]

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.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f8b29993a76c

Comment 8

[Ryan VanderMeulen [:RyanVM]]

This broke mochitest-bc/dt. Backed out.
https://hg.mozilla.org/integration/mozilla-inbound/rev/1acb63b9d6c6

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=3395138&repo=mozilla-inbound

Comment 9

[Vaibhav (:vaibhav1994)]

Attachment: fix.patch

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.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2ba76e4bc640

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2ba76e4bc640

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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?

Comment 13

[Vaibhav (:vaibhav1994)]

Attachment: fix_chunk.patch

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.

Comment 14

[Ryan VanderMeulen [: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 :)

Comment 15

[Joel Maher ( :jmaher ) (UTC -8)]

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.

Comment 16

[Vaibhav (:vaibhav1994)]

Attachment: fix_chunk.patch

Fixed the nits and carrying forward the r+.

Comment 17

[Vaibhav (:vaibhav1994)]

Attachment: fix_chunk.patch

Forgot to change the name of reviewer in commit message, changed that!

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/6740f4e7636a

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6740f4e7636a