Closed Bug 1131098 Opened 5 years ago Closed 5 years ago

Move chunking algorithms out of mochitest and into manifestparser

Categories

(Testing :: Mochitest, defect)

defect
Not set

Tracking

(firefox39 fixed)

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: ahal, Assigned: ahal)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Part of a Q1 goal is to move chunking out of our test harnesses and into manifestparser. As usual, we'll start with mochitest.
Depends on: 1132154
Depends on: 1134395
Attached file MozReview Request: bz://1131098/ahal (obsolete) —
/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
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
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
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 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
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 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
Attachment #8570165 - Flags: review?(jmaher)
Attachment #8570165 - Flags: review?(jmaher)
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.
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..
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 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!
Attachment #8570165 - Flags: feedback?(vaibhavmagarwal)
can you file a followup bug for the disabled test handling between runtests and runtestsremote?
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.
Attachment #8570165 - Flags: feedback?(vaibhavmagarwal) → feedback+
(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
Flags: needinfo?(jmaher)
yes, I was referring to the roobocop stuff.  Ideally if we are editing chunking and disabled tests, lets clean it up everywhere.
Flags: needinfo?(jmaher)
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.
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 on attachment 8570165 [details]
MozReview Request: bz://1131098/ahal

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

Ship It!
Attachment #8570165 - Flags: review+
Joel said all issues were addressed on irc, so changing to r+.

Landed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/187d5f6e0b03
https://hg.mozilla.org/mozilla-central/rev/187d5f6e0b03
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
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
Attachment #8570165 - Attachment is obsolete: true
Attachment #8619410 - Flags: review+
You need to log in before you can comment on or make changes to this bug.