Closed
Bug 915865
Opened 11 years ago
Closed 10 years ago
Use mozrunner and mozprocess in desktop reftest
Categories
(Testing :: Reftest, defect)
Testing
Reftest
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla35
People
(Reporter: mihneadb, Assigned: ted)
References
(Blocks 1 open bug)
Details
(Whiteboard: [mozbase])
Attachments
(6 files, 5 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
text/plain
|
Details | |
(deleted),
text/plain
|
Details | |
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
ahal
:
review+
|
Details | Diff | Splinter Review |
Reftest should use mozbase instead of automation.py. This is one more step in that direction.
Reporter | ||
Updated•11 years ago
|
Summary: Use mozrunner instead of automation.py in reftest → Use mozrunner instead of automation.py in the reftest harness
Reporter | ||
Comment 1•11 years ago
|
||
Tackling this problem. Not sure how to handle the debug stuff.
https://tbpl.mozilla.org/?tree=Try&rev=d2ba2992882d
Reporter | ||
Updated•11 years ago
|
Assignee: nobody → mihneadb
Status: NEW → ASSIGNED
Reporter | ||
Updated•11 years ago
|
Reporter | ||
Comment 3•11 years ago
|
||
So if we change the runTests method to stop calling runApp, we will have trouble with the Android harness. How should we go about this?
I'm thinking maybe take out the running the app part in a separate function out of runTests and override it in the Android harness.
Reporter | ||
Comment 4•11 years ago
|
||
Something like this (regarding inheriting the runApp method).
Reporter | ||
Updated•11 years ago
|
Attachment #804021 -
Attachment is obsolete: true
Comment 6•11 years ago
|
||
(In reply to Mihnea Dobrescu-Balaur (:mihneadb) from comment #4)
> Created attachment 804177 [details] [diff] [review]
> Use mozrunner instead of automation.py in the reftest harness
>
> Something like this (regarding inheriting the runApp method).
Having hacked on the Android stuff before, I approve of this solution.
Reporter | ||
Comment 7•11 years ago
|
||
Fixed the remote call and a new attempt at capturing the output stream.
https://tbpl.mozilla.org/?tree=Try&rev=09bae34f7649
Reporter | ||
Updated•11 years ago
|
Attachment #804177 -
Attachment is obsolete: true
Comment 8•11 years ago
|
||
I'm a bit confused wrt bug 913209 which I was tackling and this bug; should the former be duped to this? Should I take this now that Mihnea is back to his studies?
Updated•11 years ago
|
Reporter | ||
Comment 9•11 years ago
|
||
(In reply to Jeff Hammel [:jhammel] from comment #8)
> I'm a bit confused wrt bug 913209 which I was tackling and this bug; should
> the former be duped to this? Should I take this now that Mihnea is back to
> his studies?
Sorry, I haven't seen that bug. Feel free to take over, I'm not sure how much time I'll have. :) Maybe the work I did in the patch uploaded here helps, dunno.
Reporter | ||
Comment 11•11 years ago
|
||
Haha, thank you! ;) automation.py is *fun*
Comment 13•11 years ago
|
||
Forward duping bug 913209 to this
Summary: Use mozrunner instead of automation.py in the reftest harness → Use mozrunner and mozprocess in desktop reftest
Comment 14•11 years ago
|
||
This builds on Mihnea's work, removing references to automation from reftest proper, although it is still referenced by the option class(es) and b2g and remote (android) versions.
Next steps:
- push to try
- remove remaining references
As with mochitest
(https://bugzilla.mozilla.org/show_bug.cgi?id=746243#c83), I plan on
using the same logger from automation.py.in :
http://hg.mozilla.org/mozilla-central/file/57d160eda301/build/automation.py.in#l87
http://hg.mozilla.org/mozilla-central/diff/7a733fffc6e9/testing/mochitest/runtests.py#l1.58
While ideally `mozlog` from mozbase would be used instead, our current
evaluation of pass/fail e.g. in mozharness (though not exclusively
there) is sensitive to the form of the log messages. See the
discussion on bug 746243 for one example. I'll file a follow-up to
port both mochitest and reftest to mozlog.
Comment 15•11 years ago
|
||
Comment on attachment 804677 [details] [diff] [review]
Use mozrunner instead of automation.py in the reftest harness
Review of attachment 804677 [details] [diff] [review]:
-----------------------------------------------------------------
::: layout/tools/reftest/runreftest.py
@@ +145,5 @@
> + # TODO debugger stuff
> + status = runner.wait()
> + mozcrash.check_for_crashes(os.path.join(profile.profile, "minidumps"),
> + symbolsPath,
> + test_name=self.automation.lastTestSeen)
I'm fairly sure that we don't need self.automation.lastSeen for the test name. It is used only for reporting, according to https://github.com/mozilla/mozbase/blob/master/mozcrash/mozcrash/mozcrash.py#L45
Assignee | ||
Comment 16•11 years ago
|
||
We do actually want that, it lets us attribute crashes to the test that was running when we crash, which is important for TBPL starring.
Comment 17•11 years ago
|
||
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #16)
> We do actually want that, it lets us attribute crashes to the test that was
> running when we crash, which is important for TBPL starring.
But AIUI from the patch and comment 15, we are just using some random lastSeen which isn't getting updated at all with the refactor? (Probably this one : http://hg.mozilla.org/mozilla-central/file/tip/build/automation.py.in#l166 ?)
Comment 18•11 years ago
|
||
Not that it's a particularly shining example.. but in the B2GRunner we do: https://github.com/mozilla/mozbase/blob/master/mozrunner/mozrunner/remote.py#L184
Assignee | ||
Comment 19•11 years ago
|
||
(In reply to Jeff Hammel [:jhammel] from comment #17)
> (In reply to Ted Mielczarek [:ted.mielczarek] from comment #16)
> > We do actually want that, it lets us attribute crashes to the test that was
> > running when we crash, which is important for TBPL starring.
>
> But AIUI from the patch and comment 15, we are just using some random
> lastSeen which isn't getting updated at all with the refactor? (Probably
> this one :
> http://hg.mozilla.org/mozilla-central/file/tip/build/automation.py.in#l166 ?)
Then in that case we should copy what Mochitest is doing:
http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/runtests.py#1179
(and maybe put that in automationutils?)
Updated•11 years ago
|
Whiteboard: [mozbase]
Comment 20•11 years ago
|
||
wip. I've hit a problem in trying to get this working with TBPL (there's still some minor automation cleanup that needs to be done outside of core reftest). In short, we never print the summary which basically is a failure. This code is never hit:
http://mxr.mozilla.org/mozilla-central/source/layout/tools/reftest/reftest.js#1303
It also becomes evident following the mochitest refactor that there will be duplicate code that should go to moztest (in fact, will chiefly compose moztest). I'll follow up on this when I have the immediate problems figured out.
Attachment #809566 -
Attachment is obsolete: true
Comment 21•11 years ago
|
||
log with current attachment
Comment 22•11 years ago
|
||
log from previous attachment
Comment 23•11 years ago
|
||
log sans patch, for reference
Comment 24•11 years ago
|
||
ABICT, there are no differences in the profiles (using dirdiff and comparing just before browser launch).
Updated•11 years ago
|
Comment 25•11 years ago
|
||
Still working on this bug, but haven't actively been able to look at it as I am debugging bug 920952 which ABICT is a higher priority.
Comment 26•11 years ago
|
||
bug 921676 moves dumpScreen to automationutils.py which is sorely wanted for this bug.
Depends on: 921676
Comment 27•11 years ago
|
||
wip; current form of the patch
Attachment #812751 -
Attachment is obsolete: true
Comment 28•11 years ago
|
||
It looks like the mozprocess+mozrunner form from attachment 815200 [details] [diff] [review] ;
tests + output seem to be cut off from the end.
With the patch:
[1381366067.172621, "make", {"line": "REFTEST TEST-PASS | file:///home/jhammel/m
ozilla/src/mozilla-central/layout/reftests/font-matching/stretchmapping-35.html
| image comparison (==)"}]
[1381366067.172848, "make", {"line": "REFTEST INFO | Loading a blank page"}]
[1381366067.173121, "make", {"line": "REFTEST TEST-END | file:///home/jhammel/mo
zilla/src/mozilla-central/layout/reftests/font-matching/stretchmapping-35.html"}
]
[1381366067.173454, "make", {"line": "REFTEST TEST-START | file:///home/jhammel/
mozilla/src/mozilla-central/layout/reftests/font-matching/stretchmapping-137.htm
l"}]
[1381366067.17374, "make", {"line": "REFTEST TEST-LOAD | http://localhost:47282/
1381365739873/353/font-matching/stretchmapping-137.html | 5047 / 9952 (50%)"}]
[1381366067.174039, "make", {"line": "REFTEST TEST-LOAD | http://localhost:47282
/1381365739873/353/font-matching/stretchmapping-137-ref.html | 5047 / 9952 (50%)
"}]
[1381366067.174351, "make", {"line": "REFTEST TEST-PASS | file:///home/jhammel/m
ozilla/src/mozilla-central/layout/reftests/font-matching/stretchmapping-137.html
| image comparison (==)"}]
[1381366067.174597, "make", {"line": "REFTEST INFO | Loading a blank page"}]
[1381366067.174889, "make", {"line": "REFTEST TEST-END | file:///home/jhammel/mozilla/src/mozilla-central/layout/reftests/font-matching/stretchmapping-137.html"}]
[1381366069.197912, "make", {"line": "REFTEST TEST-START | file:///home/jhammel/mozilla/src/mozilla-central/layout/reftests/font-matching/font-stretch-1.html"}]
[1381366069.198474, "make", {"line": "REFTEST TEST-LOAD | http://localhost:47282/1381365739874/354/font-matching/font-stretch-1.html | 5048 / 9952 (50%)"}]
[1381366069.198868, "make", {"line": "WARNING | leakcheck | refcount logging is off, so leaks can't be detected!"}]
[1381366069.199179, "make", {"line": ""}]
[1381366069.199534, "make", {"line": "REFTEST INFO | runreftest.py | Running tests: end."}]
----
Without the patch:
[1381366879.187479, "make", {"line": "REFTEST TEST-KNOWN-FAIL | file:///home/jha
mmel/mozilla/src/mozilla-central/layout/reftests/invalidation/test-animated-imag
e-layers-background.html | (SKIP)"}]
[1381366879.187791, "make", {"line": "REFTEST TEST-KNOWN-FAIL | file:///home/jha
mmel/mozilla/src/mozilla-central/layout/reftests/invalidation/box-shadow-border-
radius.html | (SKIP)"}]
[1381366879.188114, "make", {"line": "REFTEST FINISHED: Slowest test took 2177ms
(http://localhost:56084/1381366240028/183/text-overflow/selection.html)"}]
[1381366879.188383, "make", {"line": "REFTEST INFO | Result summary:"}]
[1381366879.18865, "make", {"line": "REFTEST INFO | Successful: 9522 (9503 pass,
19 load only)"}]
[1381366879.188987, "make", {"line": "REFTEST INFO | Unexpected: 98 (97 unexpected fail, 1 unexpected pass, 0 unexpected asserts, 0 unexpected fixed asserts, 0 failed load, 0 exception)"}]
[1381366879.189294, "make", {"line": "REFTEST INFO | Known problems: 333 (185 known fail, 0 known asserts, 74 random, 74 skipped, 0 slow)"}]
[1381366879.189578, "make", {"line": "REFTEST INFO | Total canvas count = 8"}]
[1381366879.18983, "make", {"line": "REFTEST TEST-START | Shutdown"}]
[1381366879.282343, "make", {"line": "NOTE: child process received `Goodbye', closing down"}]
[1381366879.359045, "make", {"line": "INFO | automation.py | Application ran for: 0:10:40.627623"}]
[1381366879.359585, "make", {"line": "INFO | zombiecheck | Reading PID log: /tmp/tmptqKc2rpidlog"}]
[1381366879.360032, "make", {"line": "==> process 26385 launched child process 26935"}]
[1381366879.360492, "make", {"line": "INFO | zombiecheck | Checking for orphan process with PID: 26935"}]
[1381366879.361012, "make", {"line": "WARNING | leakcheck | refcount logging is off, so leaks can't be detected!"}]
[1381366879.36142, "make", {"line": ""}]
[1381366879.361732, "make", {"line": "REFTEST INFO | runreftest.py | Running tests: end."}]
My current plan is to (quickly) get the WIP patch to an intended fix
(not a ton left) and see if this behaviour continues.
Assignee | ||
Updated•10 years ago
|
Assignee: k0scist → ted
Assignee | ||
Comment 29•10 years ago
|
||
This is kind of a big patch, but it seems to work. More details forthcoming soon.
Attachment #8495978 -
Flags: review?(ahalberstadt)
Assignee | ||
Comment 30•10 years ago
|
||
This looks good on desktop and Android, but I broke B2G tests:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=21a319200231
Comment 31•10 years ago
|
||
Comment on attachment 8495978 [details] [diff] [review]
imported patch reftest-mozbase
Review of attachment 8495978 [details] [diff] [review]:
-----------------------------------------------------------------
Nice this wasn't as bad as I was expecting, r+ with a few comments.
::: layout/tools/reftest/runreftest.py
@@ +36,3 @@
> import mozprofile
> +import mozrunner
> +from mozrunner.utils import findInPath as which
I recently discovered that there's a which implementation in the stdlib under 'distutils.spawn.find_executable'. Using findInPath is ok too though.
@@ +153,5 @@
> + break
> + dirs.add(path)
> + path = os.path.split(path)[0]
> +
> + print "mozinfo: %s" % mozinfo.find_and_update_from_json(*dirs)
Leftover debug statement?
@@ +395,5 @@
> + if status == 0:
> + return
> + else:
> + try:
> + os.kill(processPID, signal.SIGABRT)
We could alternatively pass in the proc object and call 'proc.kill(sig=signal.SIGABRT)' here.
@@ +538,5 @@
> + process_class=mozprocess.ProcessHandlerMixin,
> + cmdargs=cmdargs,
> + env=env,
> + process_args=kp_kwargs)
> + runner.start(debug_args=debug_args,
Might be good to put a "try: except: runner.stop(); raise" around here.
Attachment #8495978 -
Flags: review?(ahalberstadt) → review+
Assignee | ||
Comment 32•10 years ago
|
||
This almost works, but b2g emulator runs are still broken:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e2112bec0fdd
They're timing out in Marionette somewhere, but I haven't figured out why yet.
Assignee | ||
Comment 33•10 years ago
|
||
Current status: frustrated! This patch is working for everything but the b2g emulator jobs as mentioned above. I've been pushing a number of tests to try but haven't made any real progress.
Assignee | ||
Comment 34•10 years ago
|
||
Just looking for a quick r+ on the minor changes from the previous version.
Attachment #8501906 -
Flags: review?(ahalberstadt)
Assignee | ||
Updated•10 years ago
|
Attachment #8495978 -
Attachment is obsolete: true
Assignee | ||
Comment 35•10 years ago
|
||
Aha! I figured out the emulator bustage, and it looks green there:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=10c549e7ea3d
Previously runreftest.py was calling self.automation.environment from buildBrowserEnv, but I changed that to call automationutils.environment. b2gautomation.py overrides Automation.environment, so that override was getting lost in the shuffle.
Assignee | ||
Comment 36•10 years ago
|
||
Looking green on try:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=2fdf5e40593a
Updated•10 years ago
|
Summary: Use mozrunner and mozprocess in desktop reftest → Use mozrunner and mozprocess in reftest
Assignee | ||
Comment 37•10 years ago
|
||
The summary as written was correct: this patch does not remove the automation.py dependency in the mobile and b2g reftest scripts.
Comment 38•10 years ago
|
||
Comment on attachment 8501906 [details] [diff] [review]
port reftest to mozbase
Review of attachment 8501906 [details] [diff] [review]:
-----------------------------------------------------------------
Lgtm!
::: layout/tools/reftest/runreftest.py
@@ +581,5 @@
> + status = self.runApp(profile, binary=options.app, cmdargs=cmdlineArgs,
> + # give the JS harness 30 seconds to deal with its own timeouts
> + env=browserEnv, timeout=options.timeout + 30.0,
> + symbolsPath=options.symbolsPath,
> + options=options, debuggerInfo=debuggerInfo)
nit: I think this was more readable the way it used to be formatted.
@@ +639,3 @@
> help = "absolute path to directory containing utility "
> "programs (xpcshell, ssltunnel, certutil)")
> + defaults["utilityPath"] = build_obj.bindir if build_obj is not None else None
nit: double negative here is unnecessary
Attachment #8501906 -
Flags: review?(ahalberstadt) → review+
Assignee | ||
Comment 39•10 years ago
|
||
Comment 40•10 years ago
|
||
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #37)
> The summary as written was correct: this patch does not remove the
> automation.py dependency in the mobile and b2g reftest scripts.
Sorry.
Summary: Use mozrunner and mozprocess in reftest → Use mozrunner and mozprocess in desktop reftest
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in
before you can comment on or make changes to this bug.
Description
•