Closed Bug 771578 Opened 7 years ago Closed 3 months ago

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

Categories

(Testing :: XPCShell Harness, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: k0scist, Unassigned)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [mozbase][leave open])

Attachments

(3 files)

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
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.)
Summary: xpcshell should be installed in the objdir virtualenv and make use of mozbase packages → runxpcshelltests.py should be installed in the objdir virtualenv and make use of mozbase packages
Whiteboard: [mozbase]
Blocks: 775756
Bug 804648 will be very difficult without mozprocess. So, adding this as a prerequisite.
Blocks: 804648
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.
Assignee: nobody → gps
Status: NEW → ASSIGNED
Attachment #716062 - Flags: review?(ted)
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.
Attachment #716063 - Flags: review?(ted)
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.
Attachment #716062 - Flags: review?(ted) → review+
Might as well get the bit rot out of the way sooner.

Part 1: https://hg.mozilla.org/integration/mozilla-inbound/rev/ce4f4d4d69d9
Whiteboard: [mozbase] → [mozbase][leave open]
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".
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.
Attachment #716321 - Flags: feedback?(ted)
(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...
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 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?
Attachment #716063 - Flags: review?(ted) → review+
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!
Attachment #716321 - Flags: feedback?(ted) → feedback+
Blocks: 870532
Assignee: gps → nobody
No assignee, updating the status.
Status: ASSIGNED → NEW
No assignee, updating the status.

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

Status: NEW → RESOLVED
Closed: 3 months ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.