Closed Bug 915865 Opened 11 years ago Closed 10 years ago

Use mozrunner and mozprocess in desktop reftest

Categories

(Testing :: Reftest, defect)

defect
Not set
normal

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)

Reftest should use mozbase instead of automation.py. This is one more step in that direction.
Depends on: 915866
Summary: Use mozrunner instead of automation.py in reftest → Use mozrunner instead of automation.py in the reftest harness
Blocks: 775756
Tackling this problem. Not sure how to handle the debug stuff.

https://tbpl.mozilla.org/?tree=Try&rev=d2ba2992882d
Assignee: nobody → mihneadb
Status: NEW → ASSIGNED
Assignee: mihneadb → nobody
No longer blocks: 775756
Status: ASSIGNED → NEW
mid air ..
Assignee: nobody → mihneadb
Blocks: 775756
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.
Something like this (regarding inheriting the runApp method).
Attachment #804021 - Attachment is obsolete: true
(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.
Fixed the remote call and a new attempt at capturing the output stream.

https://tbpl.mozilla.org/?tree=Try&rev=09bae34f7649
Attachment #804177 - Attachment is obsolete: true
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?
See Also: → 913209
(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.
Stealing, thanks a bunch Mihnea!
Assignee: mihneadb → jhammel
Haha, thank you! ;) automation.py is *fun*
Forward duping bug 913209 to this
Summary: Use mozrunner instead of automation.py in the reftest harness → Use mozrunner and mozprocess in desktop reftest
Depends on: 917750
Attached patch bug-915865 (obsolete) — Splinter Review
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 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
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.
(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 ?)
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
(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?)
Depends on: 898379
Whiteboard: [mozbase]
Attached patch bug-915865 (obsolete) — Splinter Review
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
Attached file reftest.new.log
log with current attachment
log from previous attachment
Attached file reftest.clean.out
log sans patch, for reference
ABICT, there are no differences in the profiles (using dirdiff and comparing just before browser launch).
See Also: → 921676
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.
bug 921676 moves dumpScreen to automationutils.py which is sorely wanted for this bug.
Depends on: 921676
Attached patch bug-915865Splinter Review
wip; current form of the patch
Attachment #812751 - Attachment is obsolete: true
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: k0scist → ted
Attached patch imported patch reftest-mozbase (obsolete) — Splinter Review
This is kind of a big patch, but it seems to work. More details forthcoming soon.
Attachment #8495978 - Flags: review?(ahalberstadt)
This looks good on desktop and Android, but I broke B2G tests:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=21a319200231
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+
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.
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.
Just looking for a quick r+ on the minor changes from the previous version.
Attachment #8501906 - Flags: review?(ahalberstadt)
Attachment #8495978 - Attachment is obsolete: true
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.
Summary: Use mozrunner and mozprocess in desktop reftest → Use mozrunner and mozprocess in reftest
The summary as written was correct: this patch does not remove the automation.py dependency in the mobile and b2g reftest scripts.
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+
(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
Blocks: 1080804
Blocks: 1080806
https://hg.mozilla.org/mozilla-central/rev/199775c34d69
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Depends on: 1083279
Depends on: 1125679
Depends on: 1139922
You need to log in before you can comment on or make changes to this bug.