502689 - Serializing the mochitest run by top level test directories

Status: RESOLVED WONTFIX

(Firefox Build System :: General, defect)

Description

[Murali Nandigama [:murali]]

Attachment: Modified testsuite-targets.mk

This is a known issue and we have had discussions on this before. 

Instrumented build is prone to more crashes on the bleeding edge trunk. Mochitest-plain is the place where we see [ usually but not always ] crashes or hangs in instrumented build run, if it ever happens. In order for the coverage results to be complete, we need the mochitest to continue running after the crash.
So, one way to accomplish this is to serialize the mochitest runs by the top directories like, browser,dom,layout etc., so that failure is contained to that level.

To accomplish this task without any changes to the buildbotcustom factory.py , the following modifications are required.

Please review and approve the changes at your earliest convenience.

Thanks and regards
Murali

Comment 1

[Murali Nandigama [:murali]]

Attachment: new patch that overrides the old one

This patch overrides the old patch

Comment 2

[Murali Nandigama [:murali]]

We need to make one very simple modification to the factory.py @ line number 1952

from this

self.addStep(unittest_steps.MozillaMochitest, warnOnWarnings=True,
         test_name="mochitest-plain",
         workdir="build/%s" % self.objdir,
         leakThreshold=mochitest_leak_threshold,
         timeout=5*60,
        )

to this

self.addStep(unittest_steps.MozillaMochitest, warnOnWarnings=True,
         test_name="mochitest-serial",
         workdir="build/%s" % self.objdir,
         leakThreshold=mochitest_leak_threshold,
         timeout=5*60,
        )


catlee, can you kindly do the favor on the factory.py file.

Comment 3

[Murali Nandigama [:murali]]

Attachment: Optimized patch which is latest

I have added a new target called mochitest-serial which in turn calls the mochitest-plain target for each directory under the tests directory of mochitest harness. This makefile change is the minimal change that can be made to facilitate serialized mochitest suite run.

Comment 4

[Murali Nandigama [:murali]]

Comment on attachment 387142 [details] [diff] [review]
Optimized patch which is latest

Kindly provide review and approval.

Comment 5

[Murali Nandigama [:murali]]

moved the bug back to OPEN state

Comment 6

[Murali Nandigama [:murali]]

Here is the reasoning for why this patch is required approval and integration ASAP.

There are THREE parties that are required to be changed for enabling either serialization or parallelization of mochitest harness.

runtests.py  [which runs the mochitests ]
factory.py [ the buildbotcustom script that makes call to various make files ]
testsuite-targets.mk [ the actual make file that does testing using make mochitest* command ]

In my approach runtests.py is NOT modified at all.
In catlee's approach runtests.py is heavily modified [ rightfully so .. to enable parallelism ]

In my approach a universal change [ of a single line ] as described in comment #2 would enable serialization.
In catlee's approach [ which is TBD ], he needs to make two different steps, one for all other builds to run parallel and one for  codecov builds to run serially [ on the same box ].

In my approach a clean 'mochitest-serial' target is added to enable serial running.
In catlee's approach [ which is TBD ] he needs to add two targets, one for parallel and one for serial.

If my patch is approved and checked-in , then catlee can continue to work at his own pace to implement parallel run logic. 

Making serial run wait on parallel logic to be in place [ and execute serial as a subtask ] is going to delay QA's goal of achieving consistency in codecov run results.

Specially, parallel runs for codecov builds is a NO-GO as we have to heavily re-engineer the data collection and practically have to throw away all the set-up in place to collect and display codecov results.

Comment 7

[(not currently active) Ted Mielczarek]

Comment on attachment 387142 [details] [diff] [review]
Optimized patch which is latest

I would like this target to use the parameters that catlee is adding in bug 494165, but we can use your current patch for expediency. Just some comments:

+DIRS_MOCHI_LIST := ${shell cd _tests/testing/mochitest/tests; ls -d *}
+export $(DIRS_MOCHI_LIST)

You don't need to export this variable. |export| in makefiles just means that recursive invocations of make will still see the value of this variable.
Also, just use =, not :=. := means that that shell command will be executed every time this makefile is read, which means every time anyone builds anything, since this is included in the top-level Makefile. With =, it will be lazily evaluated. Minor nit, we always use $(), not ${} in our makefiles. Finally, I think using just 'ls' there instead of 'ls -d *' will have the same effect, no?

+mochitest-serial:

I feel like maybe this should be named "mochitest-plain-serial", to be more explicit.

Comment 8

[Murali Nandigama [:murali]]

I agree with the review suggestions. 

Do you want me to make the changes or leave it in your expert hands. You are going to check in the code any way. So, it is your call.


Thanks
Murali

Comment 9

[Murali Nandigama [:murali]]

Attachment: pacth based on Ted's review

Patch based on Ted's review comments.

Comment 10

[Chris AtLee [:catlee]]

Comment on attachment 391955 [details] [diff] [review]
pacth based on Ted's review

I think we need to be filtering out the SimpleTest directory as well.

Comment 11

[Murali Nandigama [:murali]]

That would give me 9 more tests to augment code coverage , however benign they are :)

Comment 12

[Chris AtLee [:catlee]]

I believe the mochitest harness already skips them, doesn't it?

http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/server.js#332

Comment 13

[(not currently active) Ted Mielczarek]

Comment on attachment 391955 [details] [diff] [review]
pacth based on Ted's review

This looks fine for now. Can you file a follow-up bug on changing this to use the commands that catlee is adding in bug 494165?

Comment 14

[Kim Moir [:kmoir] ET]

n/a anymore, old bug.