643607 - tegra browser-chrome showing T-FAIL rather than the number of pass/fail/todo results

Status: RESOLVED FIXED

(Release Engineering :: General, defect, P3)

Description

[Mike Taylor [:bear]]

They seem to be starting but when you go to look at the log output buildbot returns "No Child Resource"

It happens for the following tests every time:

Mochitest-plain (dom/src/json/test)
Mochitest-plain (dom/tests/mochitest/dom-level0)
Mochitest-plain (content/xml/document/test) mochitest-plain (content/xul/templates/tests)

Comment 1

[Mike Taylor [:bear]]

cc'ing bhearsum on this to see if this is the new code or an ateam issue

Comment 2

[[e-mail] (:bhearsum)]

I didn't experience any Tegras going offline consistently when I was testing these patches. During testing, I did many, many runs of all of the listed suites. I haven't been paying attention to any production runs, though.

Comment 3

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

running locally with the latest fennec-4.1a1*.apk build on my tegra, I only see problems with:
content/xul/templates/tests

the rest run over and over again with no problems.  Looking into it and trying to see if I can reproduce on the other directories as well.

Comment 4

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

ok, no problems with the above directory, it was just a typo in the test-path on my end

Comment 5

[Mike Taylor [:bear]]

after asking jmaher to look at this, he found that the android mochi tests are
being set as T-FAIL at a lower threshold than what similar n900 tests are.

see
http://tinderbox.mozilla.org/showlog.cgi?log=Mobile/1300797128.1300798450.27776.gz
vs
http://tinderbox.mozilla.org/showlog.cgi?log=Mobile/1300790334.1300791072.23285.gz&fulltext=1

Comment 6

[Aki Sasaki (not active)]

browser-chrome is probably using the wrong unit test parser type.

Comment 7

[Aki Sasaki (not active)]

Ok.  If comment 5 is the thing, this bug should be morphed to "tegra browser-chrome tests have a bug in log parsing" or something.

Are there still mochitest issues?
Are those issues, if any, still causing tegras to go offline? If so, that seems to be a separate issue than using the wrong parser.

Comment 8

[Mike Taylor [:bear]]

after another conversation with jmaher (with an assist from bhearsum) it was determined I was barking up the wrong tree.  The problem isn't with how orange is triggered but rather how the error summary text is created for browser chrome.

Specifically how the createSummary() method works for android MochitestMixin vs n900's version.

This isn't a tegra stability issue so i'm unassigning it from me to focus on that

Comment 9

[Aki Sasaki (not active)]

Should they even be using the mochitest mixin? I suspect the answer is no.

Comment 10

[Aki Sasaki (not active)]

To clarify:

INFO TEST-START | Shutdown
Browser Chrome Test Summary
	Passed: 227
	Failed: 19
	Todo: 1

*** End BrowserChrome Test Results ***
INFO | automation.py | Application ran for: 0:05:21.884597
INFO | automation.py | Reading PID log: /tmp/tmp1p5QGmpidlog
WARNING | automationutils.processLeakLog() | refcount logging is off, so leaks can't be detected!

INFO | runtests.py | Running tests: end.
removing file: /mnt/sdcard/tests/mochitest.log
program finished with exit code 0
elapsedTime=334.231080
TinderboxPrint: mochitest-browser-chrome (mobile)<br/><em class="testfail">T-FAIL</em>

This should be TinderboxPrinting 227/19/1, not T-FAIL.

Comment 11

[[e-mail] (:bhearsum)]

(In reply to comment #9)
> Should they even be using the mochitest mixin? I suspect the answer is no.

They require it for evaluateCommand() and getSuiteOptions(). If it isn't parsing browser-chrome results properly, we should fix it to. AFAICT runtestsremote.py doesn't modify the test result output in any way, so I can't see any reason we can't have something that supports parsing the logs from both Desktop and Remote tests.

Comment 12

[Chris Cooper [:coop] (he/him)]

Based on comment #8, it sounds like we just need to subclass our log parsing. 

Assigning to bhearsum since he's been working on unittesting on tegras.

Comment 13

[[e-mail] (:bhearsum)]

I won't have to look at this in the next month between my existing work, the all hands, and planned vacation. Unassigning from me for now, but I can pick this up when I'm back in May if it hasn't been addressed before then.

Comment 14

[Aki Sasaki (not active)]

Bear: are you going to take this, or should we find a different assignee?

Comment 15

[Mike Taylor [:bear]]

If it needs to be fixed sooner than later, then assign it back to the pool - the current task for me this week means I am not even going to be looking at it till later in the week

Comment 16

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

mobile dev team is indicating to turn off browser-chrome on device.
crashtests are not failing any tests, need to investigate a bit more
jsreftests there is a patch on  bug 649475, this should get us reporting correctly

Comment 17

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

I cannot reporoduce the timeout that is happening with the crashtests. 

if I run them normally:
python remotereftest.py --deviceIP=192.168.1.103 --xre-path=../../bin --app=org.mozilla.fennec tests/testing/crashtest/crashtests.list

I get completion from the reftest harness in ~8 minutes.  The logfiles on tinderbox indicate this as well.

if I do this:
time python remotereftest.py --deviceIP=192.168.1.103 --xre-path=../../bin --app=org.mozilla.fennec tests/testing/crashtest/crashtests.list

I get about 30 seconds of harness overhead, definitely not in the timeout arena.

We need to investigate what is going on with the machines or buildbot scripts.

Comment 18

[Martijn Wargers (dead)]

It was mentioned on the meeting today ( https://wiki.mozilla.org/Mobile/Notes/13-Apr-2011#Test_Automation ), that the JS team would like to have debug builds tested on the tegra boards, testing js tests/js reftests.
Mfinkle mentioned that you should be able to get to the logcat output , because there is no other way to get the stdout.

Perhaps a new bug be filed for this, Aki?

Comment 19

[Aki Sasaki (not active)]

We have bug 581290 for debug builds.
And sure, we need a bug for debug build tests.

Comment 20

[Martijn Wargers (dead)]

Ok, filed bug 649690.

Comment 21

[Mike Taylor [:bear]]

Joel,

Is the work your currently doing to the tests going to get this bug moving forward, and if not, what is the next step (I'm drawing a blank to be honest.)

Comment 22

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

I have no idea where we assign T-FAIL vs a summary of pass/fail/todo for tinderbox.  None of the work that I have done or am doing is intended to fix this.  The only exception is making tests green, but for browser-chrome that falls on the developers right now.

Comment 23

[cmtalbert]

(In reply to comment #21)
> Joel,
> 
> Is the work your currently doing to the tests going to get this bug moving
> forward, and if not, what is the next step (I'm drawing a blank to be
> honest.)

Bear, this is an issue on the log summary generation in the buildbot steps of the android test steps.  It's not on the harness side.  See comments 11,12,13,14.  I believe the issue you're hitting here is that the code is parsing the android browser chrome test output using the same parser it is using for mochitests, which will not work.  The browser chrome log output is slightly different for browser chrome tests.  

I do want to fix this logging issue, but that's a much bigger issue because I want to unify all the logging.  So, we need to fix this in buildbot land for the time being.

Comment 24

[Mike Taylor [:bear]]

Attachment: override mochitest's log summary routine

Comment 25

[Aki Sasaki (not active)]

Comment on attachment 535452 [details] [diff] [review]
override mochitest's log summary routine

You're close.

* does this work?
* if you look at http://tinderbox.mozilla.org/showbuilds.cgi?tree=Mobile , you'll notice that |Android Tegra 250 mozilla-central opt test mochitest-{1,2,3,4}| don't show T-FAIL on all test runs.  However, |Android Tegra 250 mozilla-central opt test browser-chrome| is always either purple or T-FAIL.

This patch will reverse that.

I think we're probably going to need a RemoteBrowserChromeMixin with its own summarizeLogRemoteBrowserChrome and evaluateRemoteBrowserChrome, or base which summarise/evaluate function call on the test name, which is a little icky.

Comment 26

[Mike Taylor [:bear]]

Attachment: override mochitest's log summary routine

added a browser-chrome mixin and created a custom RemoteMochitest* set of classes to isolate this log parsing change

Comment 27

[Aki Sasaki (not active)]

Comment on attachment 535488 [details] [diff] [review]
override mochitest's log summary routine

>+                if 'browswer-chrome' in name:

'browser-chrome' :)

>+                    stepProc = unittest_steps.RemoteMochitestBrowswerChromeStep

'BrowserChrome'

>+def summarizeLogRemoteMochitest(name, log):

This is possibly a misnomer now, though summarizeLogRemoteBrowserChrome is a bit long-winded.

I believe you also need to override MochitestMixin.evaluateCommand(), so you'll need a

def evaluateRemoteBrowserChrome(name, log, superResult)

as well.

>+class RemoteMochitestBrowserChromeMixin(object):
>+    def createSummary(self, log):
>+        if 'browser-chrome' in self.name:
>+            self.addCompleteLog('summary', summarizeLogRemoteMochitest(self.name, log))
>+        else:
>+            self.addCompleteLog('summary', summarizeLogMochitest(self.name, log))

I don't think you need the if 'browser-chrome' bit, since you're already checking for that in RemoteUnittestFactory.

You do need to override evaluateCommand() though.

Hm, you may not even need the mixin; you could override createSummary() and evaluateCommand() directly in RemoteMochitestBrowserChromeStep.


So: spelling issues in process/factory.py, need to override evaluateCommand().

Comment 28

[Mike Taylor [:bear]]

Attachment: override mochitest's log summary routine

fixed spelling typos, removed mixin and moved new code to override's inside RemoteMochitestBrowserChromeStep

Comment 29

[Aki Sasaki (not active)]

Comment on attachment 537562 [details] [diff] [review]
override mochitest's log summary routine

>--- a/steps/unittest.py
>+++ b/steps/unittest.py
>@@ -123,16 +123,39 @@ def summarizeLogMochitest(name, log):
>     # Support browser-chrome result summary format which differs from MozillaMochitest's.
>     if name == 'mochitest-browser-chrome':
>         infoRe = r"\t(Passed|Failed|Todo): (\d+)"

This 'if' is no longer needed inside of summarizeLogMochitest.

>+    for line in log.readlines():
>+        if found:
>+            s = line.strip()
>+            l = s.split(': ')
>+            if len(l) = 2 and l[0] in keys:

if len(l) == 2 ...
(Doesn't pass test-masters.sh as is)

>+def evaluateRemoteMochitest(name, log, superResult):
>+    # When a unittest fails we mark it orange, indicating with the
>+    # WARNINGS status. Therefore, FAILURE needs to become WARNINGS
>+    # However, we don't want to override EXCEPTION or RETRY, so we still
>+    # need to use worst_status in further status decisions.
>+    if superResult == FAILURE:
>+        superResult = WARNINGS
>+
>+    if superResult != SUCCESS:
>+        return superResult
>+
>+    failIdent = r"^\d+ INFO Failed: 0"
>+    # Support browser-chrome result summary format which differs from MozillaMochitest's.
>+    if 'browser-chrome' in name:
>+        failIdent = r"^\tFailed: 0"

I'm not sure this construct is needed, as I *think* the only thing that's calling evaluateRemoteMochitest is RemoteMochitestBrowserChromeStep.

Comment 30

[Mike Taylor [:bear]]

Comment on attachment 537562 [details] [diff] [review]
override mochitest's log summary routine

committed changeset 1591:8035f4f68725

Comment 31

[Mike Taylor [:bear]]

Attachment: bustage fix because bear doesn't read comments it seems :/

fixed items raised in comment 29

Comment 32

[Mike Taylor [:bear]]

Comment on attachment 537816 [details] [diff] [review]
bustage fix because bear doesn't read comments it seems :/

committed changeset 1592:c43c87a10ee8

Comment 33

[Aki Sasaki (not active)]

Attachment: NameError: name 'MochitestBrowserChromeMixin' is not defined

This seems to be a leftover from when we had a separate mixin.

Comment 34

[Cameron McCormack (:heycam)]

I see many T-FAILs for mochitest-browser-chrome on tbpl, on various platforms.  Have these recent changes caused that?  Attachment 537816 [details] [diff] seems like it might have done it.

For example see all the Moths on http://tbpl.mozilla.org/?tree=Firefox&rev=ac8fceaec76c -- green, but T-FAIL in the status shown when clicking the build.

Comment 35

[Aki Sasaki (not active)]

Attachment: put the browser-chrome checks back into the non-remote steps

I thought this was cruft from your earlier patch(es), but I think this was there originally. =P

Comment 36

[Phil Ringnalda (:philor)]

Tree's closed (and getting backouts) over this, and even if we reopen, someone else is going to rediscover it and close again.

Comment 37

[Mike Taylor [:bear]]

Comment on attachment 537956 [details] [diff] [review]
put the browser-chrome checks back into the non-remote steps

committed changeset 1596:89bda47f09ef

Comment 38

[Aki Sasaki (not active)]

tegra browser-chrome parsing fixed; my oops on desktop browser-chrome backed out.