Closed Bug 502689 Opened 12 years ago Closed 2 years ago

Serializing the mochitest run by top level test directories

Categories

(Firefox Build System :: General, defect)

x86
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: murali, Assigned: murali)

References

Details

Attachments

(1 file, 3 obsolete files)

Attached patch Modified testsuite-targets.mk (obsolete) — Splinter Review
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
This patch overrides the old patch
Attachment #387029 - Attachment is obsolete: true
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.
Attached patch Optimized patch which is latest (obsolete) — Splinter Review
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.
Attachment #387141 - Attachment is obsolete: true
Assignee: nobody → mnandigama
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment on attachment 387142 [details] [diff] [review]
Optimized patch which is latest

Kindly provide review and approval.
Attachment #387142 - Flags: review?(ted.mielczarek)
moved the bug back to OPEN state
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Depends on: 494165
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 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.
Attachment #387142 - Flags: review?(ted.mielczarek) → review-
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
Patch based on Ted's review comments.
Attachment #387142 - Attachment is obsolete: true
Attachment #391955 - Flags: review?(ted.mielczarek)
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.
That would give me 9 more tests to augment code coverage , however benign they are :)
Attachment #391955 - Flags: review?(ted.mielczarek) → review+
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?
n/a anymore, old bug.
Status: REOPENED → RESOLVED
Closed: 12 years ago2 years ago
Resolution: --- → WONTFIX
Component: Build Config → General
Product: Firefox → Firefox Build System
You need to log in before you can comment on or make changes to this bug.