789335 - XPC shell test failures due to bug 755724

Status: RESOLVED FIXED

(Core :: General, defect, P3)

Description

[Jim Mathies [:jimm]]

When we attempt to run XPCShell tests, there's a step where firefox resources are upacked and moved around:

========= Finished unpack tests (results: 0, elapsed: 23 secs) (at 2012-09-06 14:12:01.686386) =========

========= Started 'bash -c ...' warnings (results: 1, elapsed: 0 secs) (at 2012-09-06 14:12:01.690289) =========
'bash' '-c' u'if [ ! -d firefox/plugins ]; then mkdir firefox/plugins; fi && cp bin/xpcshell.exe firefox && cp bin/ssltunnel.exe firefox && cp -R bin/components/* firefox/components/ && cp -R bin/plugins/* firefox/plugins/ && python -u xpcshell/runxpcshelltests.py --symbols-path=http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/elm-win32/1346957854/firefox-18.0a1.en-US.win32.crashreporter-symbols.zip --manifest=xpcshell/tests/all-test-dirs.list firefox/xpcshell.exe'

Prior to landing bug 755724 firefox resources are stored in the root 'firefox' directory of the zip install. After landing some resources will sit in a sub dir 'browser'.

For example:

before:
  inflating: firefox/components/browsercomps.dll  
after:
  inflating: firefox/browser/components/browsercomps.dll  

Doesn't look like there's a way to tweak this through an mc checkin, buildbot config will need an update.

The experimental code that causes this is currently on elm.

Comment 1

[Jim Mathies [:jimm]]

Should add, the test setup failure manifests as such:

cp: target `firefox/components/' is not a directory: No such file or directory

Comment 2

[Armen [:armenzg]]

You need this to be fixed ASAP, right?

Comment 3

[Jim Mathies [:jimm]]

(In reply to Armen Zambrano G. [:armenzg] from comment #2)
> You need this to be fixed ASAP, right?

Hey Armen,

If you can make this change on elm for now that would be great. We should keep this open though so that the same change can be made on mc when all this work lands there.

Comment 4

[Armen [:armenzg]]

I don't expect to be a clean change though.

BTW could you please point me to a log where it fails? That way I compare if I need to. If it's not landed yet, that's ok.

Comment 5

[Jim Mathies [:jimm]]

(In reply to Armen Zambrano G. [:armenzg] from comment #4)
> I don't expect to be a clean change though.
> 
> BTW could you please point me to a log where it fails? That way I compare if
> I need to. If it's not landed yet, that's ok.

https://tbpl.mozilla.org/php/getParsedLog.php?id=15030926&tree=Elm&full=1

Glandium's patch set is on elm so any of the xpcshell 'X' logs will show this.

Comment 6

[Armen [:armenzg]]

This has a slightly lower priority with some work that has come my way.
I should be able to grab it by the end of next week.
Please raise concerns if this derails your schedule and can try to adjust.

Comment 7

[Mike Hommey [:glandium]]

The first obvious reason why xpcshell tests fail is because they don't find application data, and we need to pass --app-path to runxpcshelltests.py, but that raises two questions:
  - are we going to have xpcshell tests for webapprt and/or metro? if we do, then we need a way to run some tests with some value of --app-path and other tests with another.
  - should we fix toolkit xpcshell tests that rely on preferences set in browser/? (I know, for instance, that some places tests rely on the places prefs that are only set in browser/ (which, btw, means these tests fail when run with a build of xulrunner, for instance))

The trouble with xpcshell tests is that there are two different ways to run them, the in-tree one, with rules in testing/testsuite-targets.mk, used by developers, and an out-of-tree one, in tinderbox scripts, used by tinderbox builds.

This means we need extra care not to break running xpcshell tests on unrelated branches when fixing the issue for m-c, and it also means it makes testing on try more difficult.

Comment 8

[Benjamin Smedberg]

I unfortunately don't know the answers to those questions: it seems to me that we don't want toolkit tests to depend on browser prefs: I don't know if the correct answer is to move the prefs, set test-specific default prefs, or move the tests to browser. If the correct answer is "move the tests to browser" then we will probably end up with similar webapprt/metro tests. But in general I don't think that we should really avoid app-specific xpcshell tests.

Comment 9

[Jim Mathies [:jimm]]

Attachment: buildbotcustom patch v.1

Before we can get to looking at test problems, we have to get past buildbot's problems with the new file layout. This turned to be a pretty trivial issue. When xpcshell tests run, we go through steps to set of the environment:

1) download firefox and tests zips
2) upack zips

unzip -oq firefox-19.0a1.en-US.win32.zip
unzip -oq firefox-19.0a1.en-US.win32.tests.zip bin* certs* modules* xpcshell*

3) setup some directories and run the tests

In this step, things break because buildbot expects that the firefox unpack will create a firefox/components directory. This dir is in the firefox zip, but it's now specific to the app dirs so it's down in firefox/browser/. and firefox/metro/..

So we need to create a platform components dir buildbot can unzip into. We ran into a similar problem with the plugins dir at some point -

http://mxr.mozilla.org/build/source/buildbotcustom/steps/unittest.py#450

if [ ! -d firefox/plugins ]; then mkdir firefox/plugins; fi &&
cp bin/xpcshell.exe firefox &&
cp bin/ssltunnel.exe firefox &&
cp -R bin/components/* firefox/components/ &&
cp -R bin/plugins/* firefox/plugins/ &&
python -u xpcshell/runxpcshelltests.py --symbols-path=http://ftp.moz...porter-symbols.zip --manifest=xpcshell/tests/all-test-dirs.list firefox/xpcshell.exe

The fix here is to do the same for components - 

if [ ! -d firefox/plugins ]; then mkdir firefox/plugins; fi &&
if [ ! -d firefox/components ]; then mkdir firefox/components; fi &&
cp bin/xpcshell.exe firefox &&
..

Attached is a patch to (what I think!) is the right python script in the buildbotcustom repo. This fix should be safe for all our builders.

--

Note this fix gets us past the buildbot problem so we can see what test failures we have to deal with. For example in my local run I had some problems with the updater tests. We need to get past this buildbot issue though before we can sort these out.

Comment 10

[Jim Mathies [:jimm]]

Attachment: buildbotcustom patch v.1

oops, sorry, didn't save my local changes, this has the proper line ending.

Comment 11

[Jim Mathies [:jimm]]

Here's a recent elm log - 

https://tbpl.mozilla.org/php/getParsedLog.php?id=16827385&tree=Elm&full=1

the error is

cp: target `firefox/components/' is not a directory: No such file or directory

Comment 12

[(not currently active) Ted Mielczarek]

This whole step is horrible. I keep hoping we can get rid of it, but xpcshell is so finicky.

Comment 13

[Jim Mathies [:jimm]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #12)
> This whole step is horrible. I keep hoping we can get rid of it, but
> xpcshell is so finicky.

We should file a follow up on general xpcshell changes. This bug needs to stay narrowly targeted at getting tests running on elm.

Comment 14

[Armen [:armenzg]]

For future work, how can we get closer what developers run VS what buildbot runs?

Should we replace the shell chain [1] for a python script that would allow buildbot to be agnostic of what happens internally?

WRT to this current bug it seems that the patch is correct and should be harmless but I would like to test it first.

[1] http://mxr.mozilla.org/build/source/buildbotcustom/steps/unittest.py#450

Comment 15

[Jim Mathies [:jimm]]

(In reply to Armen Zambrano G. [:armenzg] from comment #14)
> WRT to this current bug it seems that the patch is correct and should be
> harmless but I would like to test it first.

How much testing do we have to do to get something like this landed in production? Is it a few staging runs or is there more to it?

Comment 16

[Armen [:armenzg]]

I just wanted to see it work once.

It went red unfortunately.It seems an easy fix.:
'bash' '-c' 'if [ ! -d firefox/plugins ]; then mkdir firefox/plugins; fi && && if [ ! -d firefox/components ]; then mkdir firefox/components; fi && cp bin/xpcshell.exe firefox && cp bin/ssltunnel.exe firefox && cp -R bin/components/* firefox/components/ && cp -R bin/plugins/* firefox/plugins/ && python -u xpcshell/runxpcshelltests.py --symbols-path=http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-win32/1351034355/firefox-19.0a1.en-US.win32.crashreporter-symbols.zip --manifest=xpcshell/tests/all-test-dirs.list firefox/xpcshell.exe'
...
bash: -c: line 0: syntax error near unexpected token `&&'
bash: -c: line 0: `if [ ! -d firefox/plugins ]; then mkdir firefox/plugins; fi && && if [ ! -d firefox/components ]; then mkdir firefox/components; fi && cp bin/xpcshell.exe firefox && cp bin/ssltunnel.exe firefox && cp -R bin/components/* firefox/components/ && cp -R bin/plugins/* firefox/plugins/ && python -u xpcshell/runxpcshelltests.py --symbols-path=http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-win32/1351034355/firefox-19.0a1.en-US.win32.crashreporter-symbols.zip --manifest=xpcshell/tests/all-test-dirs.list firefox/xpcshell.exe'

Comment 17

[Armen [:armenzg]]

Attachment: XO shell setup changes due to bug 755724

r=me

It is modified from your patch by removing " && " since we already have a " && ".join() at the beginning :)

I've landed it as 5b2d191120bb on default.

I will be doing a reconfiguration of the masters this morning.

Comment 18

[Jim Mathies [:jimm]]

Comment on attachment 679267 [details] [diff] [review]
buildbotcustom patch v.1

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

::: steps/unittest.py
@@ +447,5 @@
>          bin_extension = ""
>          if platform.startswith('win'):
>              bin_extension = ".exe"
>          script = " && ".join(["if [ ! -d %(exedir)s/plugins ]; then mkdir %(exedir)s/plugins; fi",
> +                  "&& if [ ! -d %(exedir)s/components ]; then mkdir %(exedir)s/components; fi",

I see, so the " && ".join handles prepending '&&', so 

"&& if [ ! -d %(exedir)s/components ]; then mkdir %(exedir)s/components; fi",

should be

"if [ ! -d %(exedir)s/components ]; then mkdir %(exedir)s/components; fi",

Comment 19

[Jim Mathies [:jimm]]

ah, heh, we both got it. Thanks!

Comment 20

[Armen [:armenzg]]

The masters are being reconfigured and this will soon be live :)

Comment 21

[Jim Mathies [:jimm]]

I've disabled one test that was hanging to get a more complete picture. Doesn't look very pretty at this point, for example in extensions I see

INFO | Result summary:
INFO | Passed: 136
INFO | Failed: 133
INFO | Todo: 0

:/

Comment 22

[Jim Mathies [:jimm]]

I think what we need here is some sort of an environment variable that identifies when we are building / running tests with the new packager. This way we can touch up tests so that they work in both cases.

For the browser prefs issues, looks like a common step in toolkit test code is to create the prefs you need. I've only looked at a few failure of this type so far though.

Comment 23

[Armen [:armenzg]]

jimm, I will let you keep this bug around for the discussion part of the bug.

AFAIK, the releng fix got deployed and it is running everywhere.

Once there is an action for me please let me know and re-assign it to me.

Comment 24

[Jim Mathies [:jimm]]

I disabled one test which was hanging to got a full run. All in all not too bad, but will take some time to dig through.

https://tbpl.mozilla.org/php/getParsedLog.php?id=16875103&tree=Elm&full=1

Comment 25

[Jim Mathies [:jimm]]

*get

Comment 26

[Jim Mathies [:jimm]]

Note, bug 810609 isn't an xpc shell failure but I added it here so I can keep track of it. It was introduced in the most recent mc->elm merge.