If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

XPC shell test failures due to bug 755724

RESOLVED FIXED

Status

()

Core
General
P3
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jimm, Assigned: jimm)

Tracking

Trunk
x86_64
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [metro-it1])

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
Should add, the test setup failure manifests as such:

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

Comment 2

5 years ago
You need this to be fixed ASAP, right?
Assignee: nobody → armenzg
(Assignee)

Comment 3

5 years ago
(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

5 years ago
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.
(Assignee)

Comment 5

5 years ago
(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.

Updated

5 years ago
Priority: -- → P2

Comment 6

5 years ago
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.
Priority: P2 → P3
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

5 years ago
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.
(Assignee)

Comment 9

5 years ago
Created attachment 679265 [details] [diff] [review]
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.
Attachment #679265 - Flags: review?(armenzg)
(Assignee)

Updated

5 years ago
Attachment #679265 - Attachment is patch: true
(Assignee)

Comment 10

5 years ago
Created attachment 679267 [details] [diff] [review]
buildbotcustom patch v.1

oops, sorry, didn't save my local changes, this has the proper line ending.
Attachment #679265 - Attachment is obsolete: true
Attachment #679265 - Flags: review?(armenzg)
Attachment #679267 - Flags: review?(armenzg)
(Assignee)

Updated

5 years ago
Attachment #679267 - Attachment is patch: true
(Assignee)

Comment 11

5 years ago
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
This whole step is horrible. I keep hoping we can get rid of it, but xpcshell is so finicky.
(Assignee)

Comment 13

5 years ago
(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

5 years ago
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
(Assignee)

Comment 15

5 years ago
(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

5 years ago
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

5 years ago
Created attachment 679657 [details] [diff] [review]
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.
Attachment #679267 - Attachment is obsolete: true
Attachment #679267 - Flags: review?(armenzg)
Attachment #679657 - Flags: review+
(Assignee)

Comment 18

5 years ago
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",
(Assignee)

Comment 19

5 years ago
ah, heh, we both got it. Thanks!

Comment 20

5 years ago
The masters are being reconfigured and this will soon be live :)
(Assignee)

Updated

5 years ago
Whiteboard: leave-open
(Assignee)

Comment 21

5 years ago
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

:/
(Assignee)

Comment 22

5 years ago
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

5 years ago
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.
Assignee: armenzg → jmathies
(Assignee)

Comment 24

5 years ago
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
Summary: XP shell test setup config will need updating due to bug 755724 → XP shell test failures due to bug 755724
(Assignee)

Comment 25

5 years ago
*get
(Assignee)

Updated

5 years ago
Depends on: 810299
(Assignee)

Updated

5 years ago
Depends on: 810307
(Assignee)

Updated

5 years ago
Depends on: 810309
(Assignee)

Updated

5 years ago
Depends on: 810617
(Assignee)

Updated

5 years ago
Depends on: 810704
(Assignee)

Updated

5 years ago
Depends on: 810609
(Assignee)

Comment 26

5 years ago
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.
(Assignee)

Updated

5 years ago
Depends on: 810810
(Assignee)

Updated

5 years ago
Whiteboard: leave-open → leave-open, metro-it1
Whiteboard: leave-open, metro-it1 → [leave-open][metro-it1]
(Assignee)

Updated

5 years ago
Depends on: 815256
(Assignee)

Updated

5 years ago
Component: Release Engineering: Automation (General) → General
OS: Windows 7 → All
Product: mozilla.org → Core
QA Contact: catlee
Version: other → Trunk
(Assignee)

Updated

5 years ago
Depends on: 815320
(Assignee)

Updated

5 years ago
Summary: XP shell test failures due to bug 755724 → XPC shell test failures due to bug 755724
(Assignee)

Updated

5 years ago
Depends on: 816111
(Assignee)

Updated

5 years ago
No longer depends on: 815320
(Assignee)

Updated

5 years ago
Depends on: 817076
(Assignee)

Updated

5 years ago
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [leave-open][metro-it1] → [metro-it1]
You need to log in before you can comment on or make changes to this bug.