Use mozrunner and mozprocess in desktop reftest

RESOLVED FIXED in mozilla35

Status

Testing
Reftest
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: mihneadb, Assigned: ted)

Tracking

(Blocks: 3 bugs)

unspecified
mozilla35
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mozbase])

Attachments

(6 attachments, 5 obsolete attachments)

(Reporter)

Description

4 years ago
Reftest should use mozbase instead of automation.py. This is one more step in that direction.
(Reporter)

Updated

4 years ago
Depends on: 915866
(Reporter)

Updated

4 years ago
Summary: Use mozrunner instead of automation.py in reftest → Use mozrunner instead of automation.py in the reftest harness
(Reporter)

Updated

4 years ago
Blocks: 775756
(Reporter)

Comment 1

4 years ago
Created attachment 804021 [details] [diff] [review]
Use mozrunner instead of automation.py in the reftest harness

Tackling this problem. Not sure how to handle the debug stuff.

https://tbpl.mozilla.org/?tree=Try&rev=d2ba2992882d
(Reporter)

Updated

4 years ago
Assignee: nobody → mihneadb
Status: NEW → ASSIGNED
(Reporter)

Updated

4 years ago
Assignee: mihneadb → nobody
No longer blocks: 775756
Status: ASSIGNED → NEW
(Reporter)

Comment 2

4 years ago
mid air ..
Assignee: nobody → mihneadb
Blocks: 775756
(Reporter)

Comment 3

4 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

4 years ago
Created attachment 804177 [details] [diff] [review]
Use mozrunner instead of automation.py in the reftest harness

Something like this (regarding inheriting the runApp method).
(Reporter)

Updated

4 years ago
Attachment #804021 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Duplicate of this bug: 746246
(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

4 years ago
Created attachment 804677 [details] [diff] [review]
Use mozrunner instead of automation.py in the reftest harness

Fixed the remote call and a new attempt at capturing the output stream.

https://tbpl.mozilla.org/?tree=Try&rev=09bae34f7649
(Reporter)

Updated

4 years ago
Attachment #804177 - Attachment is obsolete: true

Comment 8

4 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

4 years ago
See Also: → bug 913209
(Reporter)

Comment 9

4 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.

Comment 10

4 years ago
Stealing, thanks a bunch Mihnea!
Assignee: mihneadb → jhammel
(Reporter)

Comment 11

4 years ago
Haha, thank you! ;) automation.py is *fun*

Updated

4 years ago
Duplicate of this bug: 913209

Comment 13

4 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

Updated

4 years ago
Depends on: 917750

Comment 14

4 years ago
Created attachment 809566 [details] [diff] [review]
bug-915865

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

4 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

4 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

4 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 ?)
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

4 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

4 years ago
Depends on: 898379

Updated

4 years ago
Whiteboard: [mozbase]

Comment 20

4 years ago
Created attachment 812751 [details] [diff] [review]
bug-915865

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

4 years ago
Created attachment 812752 [details]
reftest.new.log

log with current attachment

Comment 22

4 years ago
Created attachment 812753 [details]
bug-915865.20130926134133.log

log from previous attachment

Comment 23

4 years ago
Created attachment 812754 [details]
reftest.clean.out

log sans patch, for reference

Comment 24

4 years ago
ABICT, there are no differences in the profiles (using dirdiff and comparing just before browser launch).

Updated

4 years ago
See Also: → bug 921676

Comment 25

4 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

4 years ago
bug 921676 moves dumpScreen to automationutils.py which is sorely wanted for this bug.
Depends on: 921676

Comment 27

4 years ago
Created attachment 815200 [details] [diff] [review]
bug-915865

wip; current form of the patch
Attachment #812751 - Attachment is obsolete: true

Comment 28

4 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

4 years ago
Assignee: k0scist → ted
(Assignee)

Comment 29

3 years ago
Created attachment 8495978 [details] [diff] [review]
imported patch reftest-mozbase

This is kind of a big patch, but it seems to work. More details forthcoming soon.
Attachment #8495978 - Flags: review?(ahalberstadt)
(Assignee)

Comment 30

3 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 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

3 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

3 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

3 years ago
Created attachment 8501906 [details] [diff] [review]
port reftest to mozbase

Just looking for a quick r+ on the minor changes from the previous version.
Attachment #8501906 - Flags: review?(ahalberstadt)
(Assignee)

Updated

3 years ago
Attachment #8495978 - Attachment is obsolete: true
(Assignee)

Comment 35

3 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.
Summary: Use mozrunner and mozprocess in desktop reftest → Use mozrunner and mozprocess in reftest
(Assignee)

Comment 37

3 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 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
(Assignee)

Updated

3 years ago
Blocks: 1080804
(Assignee)

Updated

3 years ago
Blocks: 1080806
https://hg.mozilla.org/mozilla-central/rev/199775c34d69
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35

Updated

3 years ago
Depends on: 1083279
Depends on: 1125679
Depends on: 1130268
(Assignee)

Updated

3 years ago
Depends on: 1139922
You need to log in before you can comment on or make changes to this bug.