762837 - Tell xpcshell driver where testing modules are

Status: RESOLVED FIXED

(Testing :: XPCShell Harness, defect)

Description

[Gregory Szorc [:gps]]

The road to getting testing-only JS modules working on the build infra continues.

The build infra now extracts the directory containing the modules just fine. Yay! Unfortunately, the xpcshell test runner (runxpcshelltests.py) and subsequently the xpcshell process itself (driven by head.js) do not know where these modules are! 

As things are currently implemented, the Python driver defines a variable in the xpcshell process which itself defines the directory where the testing modules are. The xpcshell's head.js looks for the presence of this variable (_TESTING_MODULES_DIR). If it's present, it sets up the resource:// mapping. If not, nothing happens.

This works fine in local builds because the make targets in the build system pass the necessary command-line argument (--testing-modules-dir) to runxpcshelltests.py to define the modules directory.

Unfortunately, the build infra doesn't use the build system's make targets and instead invokes runxpcshelltests.py using whatever is configured in the buildbot configs.

So, where does that leave us? I think the easy solution is to modify buildbotcustom to add --testing-modules-dir to the command arguments. Unfortunately, we'll have to uplift patches to {release,esr10,beta} to no-op on this argument, or the Python test runner will bail with an "unrecognized argument" error when they start seeing this argument.

But, this simple solution doesn't feel right to me. Any time we add new functionality to the test runners that requires a new CLI argument, we need to modify the buildbot configs. And, as I've experienced, this is a long and painful process (not any person's fault - just the amount of steps you need to perform). This hurdle effectively discourages improvements to our testing infrastructure. And, given the importance of testing, I find this deeply concerning.

Is there any way we can move some of low-level command invocation logic into the in-tree build system? Maybe we have a makefile somewhere - let's say "buildbot.mk" - where the current tree defines the commands used to perform some action. That action could be "extract files from the testing archive to run the xpcshell tests" (bug 733532 solved without headache) or "run the xpcshell tests" (this bug/feature solved). You could even throw in "produce artifacts from build process that need to be uploaded" (this solves bug 733960). Then, the buildbot configs could just be configured to invoke make. e.g. |make -f buildbot.mk xpcshell-tests|. Of course, the buildbot configs would still need to provide some environment variables like the location of build symbols, cwd, etc. But, we still have an extra level of indirection that buys us a *lot* of flexibility and allows us to *move faster*.

There are a few concerns. Management of the custom makefile would need to be solved. How do test runners obtain it? Perhaps publish the file to the FTP server?

So, rant aside, what do we do about this bug? Given time constraints, I think we should proceed with a hacky, non-optimal approach. If we are serious about being flexible, moving faster, and having a better developer experience, we should file a separate bug to better integrate buildbot actions into the trees they operate on.

Comment 1

[Gregory Szorc [:gps]]

Attachment: Choice 1: Ultra hack

This is ultra hacky and fragile. The Python xpcshell test runner looks for a modules directory if one isn't supplied. This is based off of cwd(), hence the fragility. There could be false positives. Try at https://tbpl.mozilla.org/?tree=Try&rev=fef1e556bcb2. I'm optimistic. I may have to tweak a few times depending on the cwd where the build machines actually execute tests from.

Comment 2

[Gregory Szorc [:gps]]

Attachment: Choice 2: Quick fix to buildbotcustom

This adds some arguments when running the xpcshell tests on the build hosts. It /should/ work against mozilla-central. However, there are no tests in the current tree that will fail if this isn't working. So, to test will require pushing a tree with the following patches:

https://hg.mozilla.org/try/rev/09fd25730ddf
https://hg.mozilla.org/try/rev/754062516ba5

If adopted, this approach will require changes to beta, esr10, and release.

Comment 3

[(not currently active) Ted Mielczarek]

I think the hacky version is probably okay, honestly. There are only two scenarios we need to support:
1) Running from the make target in the tree: you already have it passing the right path.
2) Running from the test package on the build slaves: the paths are fixed to what they're packaged at. If you know where the modules dir is going to be, that seems fine. Maybe you don't want to use getcwd here, but use the directory containing the runxpcshelltests.py script, or the parent directory of that.

Comment 4

[Gregory Szorc [:gps]]

(In reply to Ted Mielczarek [:ted] from comment #3)
> I think the hacky version is probably okay, honestly.

In that case...

Comment 5

[Gregory Szorc [:gps]]

I will have a review up once I get a Try build to pass.

I was close at https://tbpl.mozilla.org/?tree=Try&rev=fef1e556bcb2 but Windows didn't work. I suspect it was because I was using os.path.join and something is expecting UNIX-style paths.

I've switched to "/" and that try is at http://tbpl.mozilla.org/?tree=Try&rev=51d49202da9e.

Comment 6

[Gregory Szorc [:gps]]

The simple switch to "/" didn't fix it either. I've added some more debugging code and pushed that to http://tbpl.mozilla.org/?tree=Try&rev=ab986786fa92. I'll crack this nut...

Comment 7

[Gregory Szorc [:gps]]

Ted, et al: I may have to concede defeat here. I've tried every permutation I can think of. The closest I can get is https://tbpl.mozilla.org/?tree=Try&rev=aed5cfc6f5fd. The xpcshell tests work on all platforms. However, toolkit\crashreporter\test\unit_ipc\test_content_annotation.js is timing out for unknown reasons on Windows.

Help?

Comment 8

[Gregory Szorc [:gps]]

Latest try at http://tbpl.mozilla.org/?tree=Try&rev=03786209be09

Comment 9

[Gregory Szorc [:gps]]

OK. I can finally reproduce this locally. I have no friggin clue how I got the crashreporter test to hang. Will dig deeper.

Comment 10

[Gregory Szorc [:gps]]

Fuuuuuu.

The problem has to do with backslashes. Essentially the xpcshell JS runner is constructing a string consisting of JS code to be evaluated in some other process. The Python runner properly encoded backslashes on the initial execution, which it seems every test uses except the test_content_annotation one. It appears it is going through this "eval" mechanism multiple times. And, the second time the backslashes aren't escaped and we get a syntax error. That isn't caught and the test hangs.

So, backslashes. I'm sending backslashes because I'm constructing a URI for the resource handler. And, nsILocalFile and/or nsIIOService insists on having native paths (backslashes on Windows). I may just normalize everything to forward slashes as it is passed into head.js and do the conversion when configuring the resource loader.

What a nightmare.

Comment 11

[Gregory Szorc [:gps]]

Attachment: Discover modules directory; fix bugs

Try at https://tbpl.mozilla.org/?tree=Try&rev=75032f341490

This attempts to discover the modules directory to get around buildbot limitations. Along the way, I discovered issues working with PyMake. That's what the added normpath calls are for.

I have no clue if the Try will work or not. This is such a sensitive bug...

Comment 12

[Gregory Szorc [:gps]]

Green xpcshell runs on Linux, OS X, and Windows. I almost don't believe it \o/

Comment 13

[(not currently active) Ted Mielczarek]

Comment on attachment 635063 [details] [diff] [review]
Discover modules directory; fix bugs

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

You could probably write a selftest unittest for this if you really wanted, but I won't hold you to that.

::: testing/xpcshell/runxpcshelltests.py
@@ +207,3 @@
>          self.xpcsCmd.extend([
>              '-e',
> +            'const _TESTING_MODULES_DIR = "%s";' % sanitized

I'd probably just do the .replace inline here instead of using an intermediate var.

@@ +597,5 @@
> +    if not testingModulesDir:
> +        ourDir = os.path.dirname(__file__)
> +        possible = os.path.join(ourDir, os.path.pardir, 'modules')
> +
> +        if os.path.exists(possible):

os.path.isdir to be even stricter?

Comment 14

[Gregory Szorc [:gps]]

https://hg.mozilla.org/services/services-central/rev/6f4f21863da1

Switched to isdir. Didn't do the .replace() inline because it results in a multi-line statement and Python style conventions say to avoid those.

Comment 15

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/6f4f21863da1