1131098 - Move chunking algorithms out of mochitest and into manifestparser

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[Andrew Halberstadt [:ahal]]

Part of a Q1 goal is to move chunking out of our test harnesses and into manifestparser. As usual, we'll start with mochitest.

Comment 1

[Andrew Halberstadt [:ahal]]

Attachment: MozReview Request: bz://1131098/ahal

/r/4395 - Bug 1131098 - Make mochitest use manifestparser's chunking algorithms and remove JS based ones, r=jmaher

Pull down this commit:

hg pull review -r 6aeaf4928cc98db320cf89dbf22a96715e356596

Comment 2

[Andrew Halberstadt [:ahal]]

The above is my first attempt at this. Works fairly well, though still some problems to fix:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3c228f35003e

Comment 3

[Andrew Halberstadt [:ahal]]

Getting there slowly. I think Android mochitest-gl and jetpack are the only two problems now:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=74755c9ba7cc

Comment 4

[Andrew Halberstadt [:ahal]]

I implemented the json manifests as a manifestparser filter which fixes the android failures. Unfortunately I can't remove the corresponding logic from javascript because the ipc tests still use it:
https://dxr.mozilla.org/mozilla-central/source/dom/devicestorage/ipc/test_ipc.html#147

Anyway, I think I have all the issues figured out now.. will do one last full try run before review.

Comment 5

[Andrew Halberstadt [:ahal]]

Comment on attachment 8570165 [details]
MozReview Request: bz://1131098/ahal

/r/4395 - Bug 1131098 - Make mochitest use manifestparser's chunking algorithms and remove JS based ones, r=jmaher

Pull down this commit:

hg pull review -r fb124c17bc43f937b8f1fa1eedf3d7573c803c67

Comment 6

[Andrew Halberstadt [:ahal]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=588d799860ba

The OSX red is infra bustage that affects all pushes, not just mine. The chunks have *slightly* different numbers with this patch (not sure why) but I summed up a couple of the suites and the total assertions still match, so I think it's ok.

Comment 7

[Andrew Halberstadt [:ahal]]

Comment on attachment 8570165 [details]
MozReview Request: bz://1131098/ahal

/r/4395 - Bug 1131098 - Make mochitest use manifestparser's chunking algorithms and remove JS based ones, r=jmaher

Pull down this commit:

hg pull review -r fb124c17bc43f937b8f1fa1eedf3d7573c803c67

Comment 8

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

Comment on attachment 8570165 [details]
MozReview Request: bz://1131098/ahal

https://reviewboard.mozilla.org/r/4393/#review4145

::: testing/mochitest/runtests.py
(Diff revision 2)
>                  del test['disabled']

here  we just delete the disabled test from the list- we should fix our handling of disabled tests (including in runtestsremote.py as we do it differently there.

::: testing/mochitest/runtests.py
(Diff revision 2)
> +                                                  options.totalChunks))

is this code for filtering on chunkbydir and totalchunks duplicated inside this massive else condition?  It is hard to tell is this review code, but I believe the else is the default condition and the if is for --test-path of a single test case.  Please do confirm.

::: testing/mochitest/bisection.py
(Diff revision 2)
> -        return return_tests

I am not sure we can directly remove this.  I suspect we can, but the bisection code chunks things up dynamically until it completes its round of work.  Something to keep in mind as this could cause odd failures.

Comment 9

[Andrew Halberstadt [:ahal]]

https://reviewboard.mozilla.org/r/4393/#review4149

::: testing/mochitest/runtests.py
(Diff revision 2)
>                  del test['disabled']

I agree, but I haven't made any changes to this part so I'd rather do that in a follow-up to get this landed sooner.

::: testing/mochitest/runtests.py
(Diff revision 2)
> +                                                  options.totalChunks))

Yes that is the case. It's not duplicated anywhere though..

Comment 10

[Andrew Halberstadt [:ahal]]

https://reviewboard.mozilla.org/r/4393/#review4147

> I am not sure we can directly remove this.  I suspect we can, but the bisection code chunks things up dynamically until it completes its round of work.  Something to keep in mind as this could cause odd failures.

I think this will be ok, but if you want I can flag vaibhav for feedback.

Comment 11

[Andrew Halberstadt [:ahal]]

Comment on attachment 8570165 [details]
MozReview Request: bz://1131098/ahal

Hi Vaibhav, I was wondering if you could take a quick look at the attached patch. It removes the 'get_test_chunk' from bisection.py as the list of tests bisection receives to begin with will have already been chunked.

It doesn't look like this change will break bisection to me, but I wanted to get your opinion first. Thanks!

Comment 12

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

can you file a followup bug for the disabled test handling between runtests and runtestsremote?

Comment 13

[Vaibhav (:vaibhav1994)]

Comment on attachment 8570165 [details]
MozReview Request: bz://1131098/ahal

Yes, the code removal in bisection.py looks good, and I had kept it there temporarily till the chunking would be implemented on the python side.

Comment 14

[Andrew Halberstadt [:ahal]]

(In reply to Joel Maher (:jmaher) from comment #12)
> can you file a followup bug for the disabled test handling between runtests
> and runtestsremote?

Could you clarify where you're talking about? I see robocop does it's own chunking, is that what you mean? Otherwise it looks like normal android mochitests follow the same code path.

The robocop issue seems a lot bigger than just chunking though, as practically half the harness is implemented in 'main' behind an if statement..
https://dxr.mozilla.org/mozilla-central/source/testing/mochitest/runtestsremote.py#796

Comment 15

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

yes, I was referring to the roobocop stuff.  Ideally if we are editing chunking and disabled tests, lets clean it up everywhere.

Comment 16

[Andrew Halberstadt [:ahal]]

Thanks for the clarification, filed bug 1142112. Doing this in a separate bug makes this bug more likely to land before the end of quarter.

Comment 17

[Andrew Halberstadt [:ahal]]

https://reviewboard.mozilla.org/r/4393/#review4219

> I think this will be ok, but if you want I can flag vaibhav for feedback.

Vaibhav gave it the f+

Comment 18

[Andrew Halberstadt [:ahal]]

Comment on attachment 8570165 [details]
MozReview Request: bz://1131098/ahal

https://reviewboard.mozilla.org/r/4393/#review4221

Ship It!

Comment 19

[Andrew Halberstadt [:ahal]]

Joel said all issues were addressed on irc, so changing to r+.

Landed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/187d5f6e0b03

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/187d5f6e0b03

Comment 21

[[github robot]]

Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/3394ad5d1d3eaa8e27ceea9b448ae39fa5cba0d9
Bug 1131098 - Make mochitest use manifestparser's chunking algorithms and remove JS based ones, r=jmaher

Comment 23

[Andrew Halberstadt [:ahal]]

Attachment: MozReview Request: Bug 1131098 - Make mochitest use manifestparser's chunking algorithms and remove JS based ones, r=jmaher