771578 - runxpcshelltests.py should be installed in the objdir virtualenv and make use of mozbase packages

Status: RESOLVED WONTFIX

(Testing :: XPCShell Harness, defect)

Description

[Jeff Hammel]

http://mxr.mozilla.org/mozilla-central/source/testing/xpcshell/runxpcshelltests.py

http://mxr.mozilla.org/mozilla-central/source/testing/xpcshell/runxpcshelltests.py#19
#TODO: replace this with json.loads when Python 2.6 is required.

We can actually do this today because we have simplejson installed in
the virtualenv

We can also replace the modules needed for
runxpcshelltests.py since they are in the virtualenv:

http://mxr.mozilla.org/mozilla-central/source/testing/xpcshell/Makefile.in

mozinfo is also magically imported and probably won't work without
running through `make`

If we make use of the objdir virtualenv we can get all of these things
for practically free:

http://mxr.mozilla.org/mozilla-central/source/build/virtualenv/Makefile.in

Though it might be worth blocking on getting automationutils.py (etc)
working via mozbase

Comment 1

[(not currently active) Ted Mielczarek]

I don't think there's any value in installing the file itself into the virtualenv, really. I do agree that it should start using the modules from the virtualenv, but we first need to start using the virtualenv when running packaged tests. (There's a bug with a mozharness script for this.)

Comment 2

[Gregory Szorc [:gps]]

Bug 804648 will be very difficult without mozprocess. So, adding this as a prerequisite.

Comment 3

[Gregory Szorc [:gps]]

Attachment: Part 1: Reindent Python files, v1

I will be giving the Python files a lot of love before I get to the meat of this bug: making runxpcshelltests.py use mozbase.

For this patch, I simply ran reindent.py on all the .py files in /testing/xpcshell.

Comment 4

[Gregory Szorc [:gps]]

Attachment: Part 2: Move "run a test" code into its own method, v1

This code establishes a standalone function for running a single test. I've wanted to do this for a long, long time.

If you don't think this patch by itself warrants being done, you may want to wait to see subsequent patches which build on top of this.

Comment 5

[(not currently active) Ted Mielczarek]

Comment on attachment 716062 [details] [diff] [review]
Part 1: Reindent Python files, v1

Review of attachment 716062 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not actually reviewing this, so rs=me.

Comment 6

[Gregory Szorc [:gps]]

Might as well get the bit rot out of the way sooner.

Part 1: https://hg.mozilla.org/integration/mozilla-inbound/rev/ce4f4d4d69d9

Comment 7

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/08fb882546da because you put spaces before the "INFO | Passed:" etc. summary at the end of the run, and because we like things fragile and intertwingled, the red/orange/green results of a job are tied to grovelling through the log and matching those with a regex, a regex which it seems did not expect "    INFO".

Comment 8

[Gregory Szorc [:gps]]

Attachment: Part 3: Use mozprocess for running tests, v1

This patch does a lot. It is incomplete, so only asking for f? at this time.

The biggest change in this patch is the switch to mozprocess (instead of raw Popen).

Because mozprocess pushes output via callbacks (as opposed to the pull-based model of Popen where you need to call communicate()), this change warranted some additional refactoring around how process execution is performed.

We now have a ProcessWrapper class that provides a nice, clean API around process execution. It replaces APIs on the main xpcshell runner class. I've only implemented the default/local class so far. We obviously need separate classes for remote execution as well as B2G, I believe. These will come in a future version of the patch.

While I was changing things from pull to push, I also took the opportunity to introduce a class to represent an individual test result. This work is not yet complete. I'm not sure whether I'll leave it in a half-baked state and finish it with a subsequent part or just do it all in this part. I'm leaning towards the former.

Since mozprocess supports timeouts easily, I added a timeout for local test running. This is part of bug 597064. I may leave timeouts partially implemented then we can finish the work in that bug.

A nice change resulting from the code refactoring is the output buffering behavior. Since mozprocess is line buffered, if a local process is aborted, the test result class has all of the output already. No additional process/pipe/buffering mucking is needed. This will make bug 804648 much easier to implement. Another side-effect of this is that we don't buffer test output in certain modes. So, now when you run a single xpcshell test with mach, output is printed as the child process emits it. Previously, all output was buffered until the child process exited and then mach printed it. I like the new model.

This patch paves the way for additional refactoring of the xpcshell runner. Eventually, I want there to be a XPCShellTest class that encapsulates test metadata and provides mechanisms for running tests and interpreting the results. Once this is done, the test runner will simply instantiate multiple instances then proceed to run them, eventually in parallel. This will require a lot of additional work because currently individual tests alter state of the global test runner. We'll get there.

For the feedback review, I'm mostly interested in comments on patch direction. If I'm doing something wrong or doing things that overlap with other efforts, please let me know.

Comment 9

[Gregory Szorc [:gps]]

(In reply to Phil Ringnalda (:philor) from comment #7)
> Backed out in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/08fb882546da because
> you put spaces before the "INFO | Passed:" etc. summary at the end of the
> run, and because we like things fragile and intertwingled, the
> red/orange/green results of a job are tied to grovelling through the log and
> matching those with a regex, a regex which it seems did not expect "   
> INFO".

Well, I guess reindent.py is buggy. Sad panda. Perhaps we should have unit tests that ensure the printed output is sane...

Comment 10

[Gregory Szorc [:gps]]

There was a multiline log string that got indented. I changed this to be a series of individual log messages and verified locally the output looks sane. IMO the trivial change did not warrant explicit review.

https://hg.mozilla.org/integration/mozilla-inbound/rev/43af5bd72395

Comment 11

[Ryan VanderMeulen [Away July 12-26]]

https://hg.mozilla.org/mozilla-central/rev/43af5bd72395

Comment 12

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 716321 [details] [diff] [review]
Part 3: Use mozprocess for running tests, v1

Review of attachment 716321 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/xpcshell/runxpcshelltests.py
@@ +71,5 @@
> +
> +        # Environment variables used everywhere.
> +        self._core_environment = {
> +            
> +        }

Something missing?

Comment 13

[Gregory Szorc [:gps]]

Part 2:

https://hg.mozilla.org/integration/mozilla-inbound/rev/2177de4cfb4f

Comment 14

[Ryan VanderMeulen [Away July 12-26]]

https://hg.mozilla.org/mozilla-central/rev/2177de4cfb4f

Comment 15

[(not currently active) Ted Mielczarek]

Comment on attachment 716321 [details] [diff] [review]
Part 3: Use mozprocess for running tests, v1

Review of attachment 716321 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, I keep forgetting this was in my queue (and you never bug me about it). Overall this looks nice. Despite the fact that I only wrote this harness about 6 years ago it's grown pretty crufty.

::: testing/xpcshell/runxpcshelltests.py
@@ +37,5 @@
> +class ProcessWrapper(object):
> +    """Wrapper for interacting with processes.
> +
> +    The default implementation provides a facility for local processes. Derived
> +    classes may support remote processes, etc.

Is it just me, or are you just working around a deficient mozprocess API? We talked about making the mozprocess API suck less, I wonder if you'd have any input there.

@@ -907,5 @@
> -                          # if e10s test started but never finished (child process crash)
> -                          (stdout and re.search("^child: CHILD-TEST-STARTED",
> -                                                stdout, re.MULTILINE)
> -                                  and not re.search("^child: CHILD-TEST-COMPLETED",
> -                                                    stdout, re.MULTILINE)))

Cleaning this up is really nice!

Comment 16

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 17

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 18

[Geoff Brown [:gbrown]]

Unfortunately, I'm not sure the wip here can be salvaged.