Closed Bug 989583 Opened 10 years ago Closed 10 years ago

Install all tests (even disabled) from manifests

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla33

People

(Reporter: ted, Assigned: armenzg)

References

Details

(Keywords: ateam-unittests-task)

Attachments

(2 files, 10 obsolete files)

15.40 KB, patch
jmaher
: review+
Details | Diff | Splinter Review
3.35 KB, patch
jmaher
: review+
Details | Diff | Splinter Review
changing disabled=True, yields build errors, I will look into this more next week.
This may be something that Armen could help with, if it isn't too high a priority, as he's still finishing up some other tasks.  I'm not sure what the build errors are, but it might be good introduction to some of our tooling.
great idea!
I will give it a shot next week.
Assignee: nobody → armenzg
Blocks: 996183
:armenzg -- Are you finding time for this? My concern is that bug 984523 (important in itself) is waiting on this bug, and is blocking greening up mochitest-gl on Android 2.3 -- one of the last issues for 2.3.
Flags: needinfo?(armenzg)
Hi gbrown,
I have not had time to look at it. The blobbler discoverability was taking preference.
Would you like me to change gears to this? I've two days off this week and another two next week.
Flags: needinfo?(armenzg)
*I* would prefer that you look at this, but I trust your judgement on relative priorities.
I will at least try to get a Firefox build going so I can get to package tests.

So far my environment is not quite there: https://groups.google.com/forum/#!topic/mozilla.dev.builds/BHG-TBkfkeg
What do we mean by "install all tests"?
Create a test.zip that contains even disabled tests?

I got this error: https://pastebin.mozilla.org/5092859
0:02.57 ValueError: Item already in manifest: testing/mochitest/tests/dom/browser-element/mochitest/test_browserElement_inproc_ErrorSecurity.html

I will look more into it.
yes, we want tests.zip to contain all tests including disabled ones for the given build.

I am not sure why you get a duplication error there, probably some silly thing we are doing in manifest parser.
I think the issue is that we have some tests defined in two different manifest under the same path:
armenzg-thinkpad mozilla-inbound hg:[default!] $ grep -r "test_browserElement_inproc_ErrorSecurity.html" .
./dom/browser-element/mochitest/mochitest-oop.ini:[test_browserElement_inproc_ErrorSecurity.html]
./dom/browser-element/mochitest/mochitest.ini:[test_browserElement_inproc_ErrorSecurity.html]

How do we go about this?
FTR in one of the manifests is disabled.

I can avoid duplicates; works for everyone?
This is expected, and this is something we need to support. If we can't currently do this we'll have to fix whatever code doesn't handle it. It looks like this is build system code just trying to copy tests to their final destination. It should be safe to fix that code (python/mozbuild/mozbuild/backend/recursivemake.py) to deal with duplicate tests.
Attached patch all_tests.diff (obsolete) — Splinter Review
I hope I have done this correctly [1]

Pushing to try:
https://tbpl.mozilla.org/?tree=Try&rev=5fe627a54a7b

gbrown, would you mind having a look at the outcoming tests.zip to see if it meets your needs?

[1]
armenzg-thinkpad objdirs $ find firefox-bug989583-current-status/dist/test-package-stage -type f | wc -l
51995
armenzg-thinkpad objdirs $ find firefox-bug989583/dist/test-package-stage -type f | wc -l
53465
Attachment #8418973 - Flags: feedback?
Comment on attachment 8418973 [details] [diff] [review]
all_tests.diff

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

overall this looks good.  There is still an issue with the build on your try push.
Attachment #8418973 - Flags: feedback? → feedback+
Comment on attachment 8421014 [details] [diff] [review]
All tests get installed plus remove two tests that expect that behaviour

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

great stuff!

::: dom/browser-element/mochitest/mochitest-oop.ini
@@ +6,5 @@
>  skip-if = os == "android" || (toolkit == "cocoa" && debug) || (buildapp == 'b2g' && (toolkit != 'gonk' || debug)) || e10s
>  support-files =
>    browserElement_OpenMixedProcess.js
>    file_browserElement_OpenMixedProcess.html
> +dupe-manifest =

my only questions here:
1) is this a truly identical manifest? (does it even matter)
2) can we document what this is a duplicate of?  i.e. dupe-manifest = mochitest.ini
Attachment #8421014 - Flags: review?(jmaher) → review+
Now that you point it out; would we breaking the efforts to fix the oop work?
There are likely tests on the mochitest-oop.ini that are not specified on mochitest.ini.
It feels that oop efforts should not be using this separate manifest or use a separate tree.
I'm not sure.

I think the only reason that I'm adding "dupe-manifest =" to the defaults is because there are tests that are shared between the two manifests [1].
I could also add the disabling lines to the tests that are shared between the two manifests.

What do you think?

armenzg-thinkpad mozilla-inbound hg:[default!] $ grep "test_browserElement_inproc_Err" dom/browser-element/mochitest/mochitest.ini
[test_browserElement_inproc_ErrorSecurity.html]
armenzg-thinkpad mozilla-inbound hg:[default!] $ grep "test_browserElement_inproc_Err" dom/browser-element/mochitest/mochitest-oop.ini
[test_browserElement_inproc_ErrorSecurity.html]
The mochitest-oop manifest isn't actually related to the e10s work, it's mostly related to B2G.

I think adding dupe-manifest to the top-level is fine, it seems silly to have to put it for every test. I don't think it'll be harmful even if not every test is duplicated. It literally has only one purpose--to let the build backend know that it's okay that we have a duplicated test.
for this, lets just make sure that what is run on try (or locally) is correct.  We should be fine for having all tests/files copied to tests.zip.  I would still like to document what is the dupe-manifest
OK cool.

Let me push to try before landing on m-i.
I see all xpcshell jobs failing:
https://tbpl.mozilla.org/php/getParsedLog.php?id=39496277&tree=Try&full=1#error0

I will try to reproduce locally.
It seems that head_addons.js (and many more) are not being added:
armenzg-thinkpad tmp $ find bug989583_tests_zip -name head_addons.js
armenzg-thinkpad tmp $ find regular_tests_zip/ -name head_addons.js
regular_tests_zip/xpcshell/tests/toolkit/mozapps/extensions/test/xpcshell/head_addons.js
armenzg-thinkpad tmp $ ls bug989583_tests_zip/xpcshell/tests/toolkit/mozapps/extensions/test/xpcshell | wc -l
2
armenzg-thinkpad tmp $ ls regular_tests_zip/xpcshell/tests/toolkit/mozapps/extensions/test/xpcshell | wc -l
162

I will look into it.
Instead of marking mochitest-oop.ini as a dupe manifest, I've removed test_browserElement_inproc_ErrorSecurity.html.

I will ask for reviews in the morning once I'm sure it works.
https://tbpl.mozilla.org/?tree=Try&rev=e0a39d349162
Attachment #8418973 - Attachment is obsolete: true
Attachment #8421014 - Attachment is obsolete: true
Comment on attachment 8421257 [details] [diff] [review]
Disabled tests get installed + remove two tests that expect that behaviour + remove test that mochitest-oop.ini adds which exists in mochitest.ini

It works as intended.

ted: I'm asking for your review mainly for your authorship of mochitest-oop.ini.
Attachment #8421257 - Flags: review?(ted)
Attachment #8421257 - Flags: review?(jmaher)
Comment on attachment 8421257 [details] [diff] [review]
Disabled tests get installed + remove two tests that expect that behaviour + remove test that mochitest-oop.ini adds which exists in mochitest.ini

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

::: dom/browser-element/mochitest/mochitest-oop.ini
@@ -8,5 @@
>    browserElement_OpenMixedProcess.js
>    file_browserElement_OpenMixedProcess.html
>  
> -[test_browserElement_inproc_ErrorSecurity.html]
> -skip-if = toolkit=='gonk'

now we won't run this test in oop mode?  I am not sure if I like this.  In the test run, I don't see us running inproc_ErrorSecurity.html (https://tbpl.mozilla.org/?tree=Try&rev=e0a39d349162), ted, this mochitest-oop.html should be run in the m-e10s() block, right?
Attachment #8421257 - Flags: review?(jmaher) → review-
Commenting it out in mochitest.ini (where the test is marked as disabled)
Leaving mochitest-opp.ini untouched.
Attachment #8421257 - Attachment is obsolete: true
Attachment #8421257 - Flags: review?(ted)
Attachment #8421723 - Flags: review?(ted)
Attachment #8421723 - Flags: review?(jmaher)
Triggered again: https://tbpl.mozilla.org/?tree=Try&rev=62effa027065

From http://mxr.mozilla.org/mozilla-central/source/dom/browser-element/mochitest/mochitest-oop.ini#11
11 [test_browserElement_inproc_ErrorSecurity.html]
12 skip-if = toolkit=='gonk'

From http://mxr.mozilla.org/mozilla-central/source/dom/browser-element/mochitest/mochitest.ini#196
196 [test_browserElement_inproc_ErrorSecurity.html]
197 skip-if = buildapp=='b2g'
198 disabled =
Comment on attachment 8421723 [details] [diff] [review]
Disabled tests get installed + remove two tests that expect that behaviour + comment test which exists in mochitest-oop.ini

jmaher, the root issue is that two manifest can specify the same test; should we just handle the situation? "do not install this test if it already has"

Working on it.
Attachment #8421723 - Flags: review?(ted)
Attachment #8421723 - Flags: review?(jmaher)
The main reason I prioritized this bug is because it would unblock gbrown (now to be gone for PTO).
I will put this to the side until next week while I clear up other bugs assigned to me.
No worries, and thanks for all your efforts here. I worked around this for Android 2.3 mochitest-gl, so this is still important, but not blocking me.
If you look at the original Makefile that I copied from while creating those manifests, you can see that that test is listed twice for some reason, and I must have blindly copied it:
https://hg.mozilla.org/releases/mozilla-esr24/annotate/ab7f02ae031d/dom/browser-element/mochitest/Makefile.in#l175

It should be fine to just remove test_browserElement_inproc_ErrorSecurity.html from mochitest.ini, it was never being run in the non-oop case in the first place.
Oh!
That means that we should be good with my current patch.
Comment on attachment 8421723 [details] [diff] [review]
Disabled tests get installed + remove two tests that expect that behaviour + comment test which exists in mochitest-oop.ini

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

::: dom/browser-element/mochitest/mochitest.ini
@@ +195,5 @@
> +# Commented out because mochitest-oop.ini already includes this test
> +#    # Disable due to certificate issue (no bug that I'm aware of)
> +#    [test_browserElement_inproc_ErrorSecurity.html]
> +#    skip-if = buildapp=='b2g'
> +#    disabled = 

we could probably remove this then!
Attachment #8421723 - Flags: review+
Comment on attachment 8421723 [details] [diff] [review]
Disabled tests get installed + remove two tests that expect that behaviour + comment test which exists in mochitest-oop.ini

https://hg.mozilla.org/integration/mozilla-inbound/rev/c5307cfba415

Landed by deleting the test instead of commenting it out.
Attachment #8421723 - Attachment description: Disabled tests get installed + remove two tests that expect that behaviour + comment test which exists in mochitest-oop.ini → [checked-in] Disabled tests get installed + remove two tests that expect that behaviour + comment test which exists in mochitest-oop.ini
This caused a large number of B2G mochitest failures (desktop and emulator) that contributed to a large bustage pileup on inbound and led to having to revert to a last-good cset, inconveniencing other innocent developers as well. Please give this a full Try run before attempting to re-land.

https://hg.mozilla.org/integration/mozilla-inbound/rev/eb2a6f7785a2
:(

I'm very sorry about this. I will be more careful.
I'm back to this while the mulet work is being reviewed.

I will review those failures and try to reproduce them locally.
Here's the list of jobs that failed:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=c5307cfba415&jobname=b2g.*mochitest

* B2g desktop:   mochitest-1
* B2g ICS opt:   mochitest-{1,2,3,4,5,7,8,9}
* B2g ICS debug: mochitest-{1,2,3,4,5,7,12,13,15}

Any suggestions on how to tackle this?

I'm thinking of checking the manifests for the failing tests.
What's happening here is that we're running tests that should be skipped; i.e., we're running everything in tests.zip and ignoring the test filtering that happens in mochitest.ini.

I'm not sure whether this is because we're not filtering at all at run-time, in which case we need to add that, or because we are filtering but it isn't working for all the manifest conditions.  Joel, do you know which it is?
Flags: needinfo?(jmaher)
so this is an issue on b2g, not other platforms.  That to me indicates the in harness filtering code is working as we thought.

Maybe the b2g test runner is not using a manifest or passing in something that has us ignoring the manifest?

I saw an issue while testing locally where I needed to use more () in my conditions, we might need to make some changes.

How hard is it to run these locally?
Flags: needinfo?(jmaher)
Right now I'm trying to run it with this:
python scripts/scripts/b2g_desktop_unittest.py --cfg b2g/desktop_automation_config.py --test-suite mochitest --this-chunk 1 --total-chunks 1 --blob-upload-branch mozilla-inbound --download-symbols ondemand --no-read-buildbot-config --installer-url http://ftp.mozilla.org/pub/mozilla.org/b2g/tinderbox-builds/mozilla-inbound-linux32_gecko/1400225075/en-US/b2g-32.0a1.en-US.linux-i686.tar.bz2 --test-url http://ftp.mozilla.org/pub/mozilla.org/b2g/tinderbox-builds/mozilla-inbound-linux32_gecko/1400225075/en-US/b2g-32.0a1.en-US.linux-i686.tests.zip --log-level=debug

Not sure if there is a better approach.

I need to make some config changes on mozharness or book a machine to run it in (I should do the latter one).

The machine also needs to hit tooltool to download something (from looking at the configs). This requires buildVPN access.
I've found that the tooltool downloads work with just MozillaVPN access; they don't seem to require buildVPN.

You can run these locally without the mozharness bits (since I don't think this is a mozharness problem) by extracting the build and tests.zip and running something like the following command from tests/mochitest:

07:57:05     INFO - Calling ['/builds/slave/test/build/venv/bin/python', 'runtestsb2g.py', '--console-level=INFO', '', '--total-chunks=1', '--this-chunk=1', '--profile=/builds/slave/test/build/application/b2g/gaia/profile', '--app=/builds/slave/test/build/application/b2g/b2g', '--desktop', '--utility-path=/builds/slave/test/build/tests/bin', '--certificate-path=/builds/slave/test/build/tests/certs', '--symbols-path=https://ftp-ssl.mozilla.org/pub/mozilla.org/b2g/tinderbox-builds/mozilla-central-linux64_gecko/1400739738/en-US/b2g-32.0a1.en-US.linux-x86_64.crashreporter-symbols.zip', '--quiet'] with output_timeout 1000

...omitting --symbols-path and --quiet.

It's quite possible the B2G runners do not honor the manifests correctly, since they were made before we had manifest support.  Where does the desktop Firefox mochitest runner parse the manifests?
Yep, that's the problem!  See what we do in B2G:

http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/runtestsb2g.py#68

It's possible this would magically work by removing that override completely, or we may have to do something special for B2G.
After using mozharness to set things up, I can run it manually [1]
However, it fails with missing a library:
/home/armenzg/moz/tmp/bug985442/build/tests/bin/xpcshell: error while loading shared libraries: libdbus-glib-1.so.2: cannot open shared object file: No such file or directory

I'm going to loan a machine as much as I wanted to make this work without one.

/home/armenzg/moz/tmp/bug985442/build/tests/bin/xpcshell -g /home/armenzg/moz/tmp/bug985442/build/application/b2g -v 170 -f /home/armenzg/moz/tmp/bug985442/build/tests/bin/components/httpd.js -e "const _PROFILE_PATH = '/tmp/tmppwhnio'; const _SERVER_PORT = '8888'; const _SERVER_ADDR = '127.0.0.1'; const _TEST_PREFIX = undefined; const _DISPLAY_RESULTS = false;" -f /home/armenzg/moz/tmp/bug985442/build/tests/mochitest/server.js

[1]
 source build/venv/bin/activate
(venv)armenzg-thinkpad bug985442 $ python build/tests/mochitest/runtestsb2g.py --console-level=INFO "" --total-chunks=1 --this-chunk=1 --profile=/home/armenzg/moz/tmp/bug985442/build/application/b2g/gaia/profile --app=/home/armenzg/moz/tmp/bug985442/build/application/b2g/b2g --desktop --utility-path=/home/armenzg/moz/tmp/bug985442/build/tests/bin --certificate-path=/home/armenzg/moz/tmp/bug985442/build/tests/certs
MochitestServer : launching [u'/home/armenzg/moz/tmp/bug985442/build/tests/bin/xpcshell', '-g', '/home/armenzg/moz/tmp/bug985442/build/application/b2g', '-v', '170', '-f', '/home/armenzg/moz/tmp/bug985442/build/tests/bin/components/httpd.js', '-e', "const _PROFILE_PATH = '/tmp/tmppwhnio'; const _SERVER_PORT = '8888'; const _SERVER_ADDR = '127.0.0.1'; const _TEST_PREFIX = undefined; const _DISPLAY_RESULTS = false;", '-f', '/home/armenzg/moz/tmp/bug985442/build/tests/mochitest/server.js']
/home/armenzg/moz/tmp/bug985442/build/tests/bin/xpcshell: error while loading shared libraries: libdbus-glib-1.so.2: cannot open shared object file: No such file or directory
runtests.py | Server pid: 5838
runtests.py | Websocket server pid: 5840
INFO | runtests.py | SSL tunnel pid: 5842
TEST-UNEXPECTED-FAIL | runtests.py | Timed out while waiting for server startup.
Stopping web server
Stopping web socket server
Stopping ssltunnel
It is running on the loaner.
I will read on the code from comment 45 and comment 46.
The error in comment #47 just means you're lacking a library on your local machine.  If you're running Ubuntu, you need to install libdbus-glib-1-2.
It might have been that I was trying to run the 32-bit desktop build on my 64-bit machine (since I had already gone through the Linux prerequisites).

IIUC, the b2g runner does not read a manifest that tells it which tests to run (unlike desktop).
How do I know if there is a manifest it should start from?

Where is the test manifest that it should pay attention to?
I see this in our code '%(test_manifest)s' however I see this in the log '',

I see this in the log:
08:17:08  WARNING - Ignoring non-existent manifest 'b2g-desktop.json'.

Should we have a file like this for b2g desktop?
http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/androidx86.json

I will probably need someone on Tuesday/Wednesday to go over how this is supposed to work. I don't have the conceptual model clear. Who would be a good candidate?




Ignore my notes at the end of this comment. I'm just building up steps on how to reproduce.

== Command to run locally ==
python scripts/scripts/b2g_desktop_unittest.py --cfg b2g/desktop_automation_config_dev.py --test-suite mochitest --this-chunk 1 --total-chunks 1 --blob-upload-branch mozilla-inbound --download-symbols ondemand --no-read-buildbot-config --installer-url http://ftp.mozilla.org/pub/mozilla.org/b2g/tinderbox-builds/mozilla-inbound-linux64_gecko/1400225075/b2g-32.0a1.multi.linux-x86_64.tar.bz2 --test-url http://ftp.mozilla.org/pub/mozilla.org/b2g/tinderbox-builds/mozilla-inbound-linux64_gecko/1400225075/b2g-32.0a1.multi.linux-x86_64.tests.zip --log-level=debug

== Command to run on Linux 64-bit loaner ==
cd /builds/slave/test/
export MOZ_UPLOAD_DIR=/builds/slave/test/build/blobber_upload_dir
export DISPLAY=:0
/tools/buildbot/bin/python scripts/scripts/b2g_desktop_unittest.py --cfg b2g/desktop_automation_config.py --test-suite mochitest --this-chunk 1 --total-chunks 1 --blob-upload-branch mozilla-inbound --download-symbols ondemand --installer-url http://ftp.mozilla.org/pub/mozilla.org/b2g/tinderbox-builds/mozilla-inbound-linux64_gecko/1400225075/b2g-32.0a1.multi.linux-x86_64.tar.bz2 --test-url http://ftp.mozilla.org/pub/mozilla.org/b2g/tinderbox-builds/mozilla-inbound-linux64_gecko/1400225075/b2g-32.0a1.multi.linux-x86_64.tests.zip --log-level=debug --no-read-buildbot-config

== Without mozharness
If run without the run-tests action, you can do this (note the use of --test-path):
cd build/tests/mochitest
/builds/slave/test/build/venv/bin/python runtestsb2g.py --console-level=INFO --total-chunks=1 --this-chunk=1 --profile=/builds/slave/test/build/application/b2g/gaia/profile --app=/builds/slave/test/build/application/b2g/b2g --desktop --utility-path=/builds/slave/test/build/tests/bin --certificate-path=/builds/slave/test/build/tests/certs --test-path=/builds/slave/test/build/tests/mochitest/tests/b2g/chrome/content/test/mochitest
Hey Armen, ping myself or ahal on Monday/Tuesday and we can sort this out.
My apologies for the delay in here. I needed to tinker around to understand the flow.
I spoke with ahal yesterday and it helped me understand things a bit better.

I managed to reproduce locally and switched to using buildTestPath from runtestb2g.py [1] to runtests.py [2]. I have some tests failing. I will add more info when I'm sure I run it properly.

I pushed to try to see what changes on the other b2g mochitest builds.
I will review the results in the morning.

[1] http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/runtestsb2g.py#68
[2] http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/runtests.py#449
[3] https://tbpl.mozilla.org/?tree=Try&rev=ab16f0f7366b
I'm starting to see a new issue with more recent code.

It is now raising a "No sections found". I can reproduce locally.
https://tbpl.mozilla.org/php/getParsedLog.php?id=40486164&tree=Try&full=1#error0
Why is it trying to read tests.json?
I thought we only read .ini files.

Output from modified code:
> File "/builds/slave/test/build/tests/mochitest/manifestparser.py", line 359, in read_ini
>   raise Exception('No sections found for %s' % fp.name)
> Exception: No sections found for /builds/slave/test/build/tests/mochitest/tests.json
we read the .ini files and create tests.json which can be easily read  by the in browser harness.
Attached file output.txt (obsolete) —
It seems that we run twice buildTestPath [1][2]
One from setup_common_options() and another one from runTests().

On the first pass, the manifest is set to be tests.json.
On the second pass, it tries to read tests.json as if it was a manifest but it is not.

I attached a log with a bit of debug information.

I also noticed that when we call it from buildbot we don't use --manifest=tests/mochitest.ini

What is the right way?

I run this locally like this:
/builds/slave/test/build/venv/bin/python runtestsb2g.py --console-level=INFO --manifest=tests/mochitest.ini --total-chunks=1 --this-chunk=1 --profile=/builds/slave/test/build/application/b2g/gaia/profile --app=/builds/slave/test/build/application/b2g/b2g --desktop --utility-path=/builds/slave/test/build/tests/bin --certificate-path=/builds/slave/test/build/tests/certs --symbols-path=https://ftp-ssl.mozilla.org/pub/mozilla.org/b2g/tinderbox-builds/mozilla-inbound-linux64_gecko/1400225075/b2g-32.0a1.multi.linux-x86_64.crashreporter-symbols.zip

[1] http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/runtestsb2g.py#62
[2] http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/runtests.py#1366
We should get Joel's input, but eyeballing the code, what we probably want to do is change http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/runtests.py#460 so that if options.manifestFile is already a .json file, skip all the manifest processing and just return self.builtTestURL, ala http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/runtests.py#519.

The other option is to refactor runtestsb2g.py so that we don't end up calling buildTestPath twice.  This is somewhat harder, given that we override different things for b2gdesktop mochitests vs emulator mochitests.
I would like to find a way to not call buildTestPath twice.  Maybe we need to pull the manifest parsing outside of buildTestPath?

It is more refactoring, but I see that as a cleaner long term solution.  Otherwise we could do what :jgriffin suggests and check to see if the manifest is a .json (we could validate it as well i.e. read/parse) and then skip the .ini reading/filtering.
Pulling the manifest parsing outside of buildTestPath and not to call buildTestPath twice wfm.
This will take a bit of time to get it right.
Attachment #8421723 - Attachment description: [checked-in] Disabled tests get installed + remove two tests that expect that behaviour + comment test which exists in mochitest-oop.ini → Disabled tests get installed + remove two tests that expect that behaviour + comment test which exists in mochitest-oop.ini
Instead of removing "buildTestPath" from the child class, I did the following:

class B2GMochitest(MochitestUtilsMixin):
...
     def buildTestPath(self, options):
-        # Skip over the manifest building that happens on desktop.
+        if options.manifestFile != 'tests.json':
+            super(Mochitest, self).buildTestPath(options)
         return self.buildTestURL(options)

The problem was coming that setup_options_common() (from B2GMochitest [1]) and doTests() (from Mochitest [2]) were both calling buildTestPath().

[1] https://hg.mozilla.org/mozilla-central/file/default/testing/mochitest/runtestsb2g.py#l63
[2] https://hg.mozilla.org/integration/mozilla-inbound/file/default/testing/mochitest/runtests.py#l1414
A bit closer.

We now have the b2g emulator jobs failing due to my new change:
https://tbpl.mozilla.org/?tree=Try&rev=d6bafd17ba5d&jobname=b2g_emulator_vm%20try.*test%20mochitest

I will try to reproduce locally.
Depends on: 1019962
I've managed to run this on a releng loaner:
https://tbpl.mozilla.org/?tree=Try&rev=fd6871af9796

I'm not happy with what I do with self.testRoot and self.testRootAbs but I want to see results for tomorrow.

screen -R armenzg
export DISPLAY=:0
cd /builds/slave/test
python scripts/scripts/b2g_emulator_unittest.py --cfg b2g/emulator_automation_config.py --test-suite mochitest --this-chunk 1 --total-chunks 9 --blob-upload-branch try --download-symbols ondemand --no-read-buildbot-config --installer-url http://pvtbuilds.pvt.build.mozilla.org/pub/mozilla.org/b2g/try-builds/armenzg@mozilla.com-d6bafd17ba5d/try-emulator/emulator.tar.gz --test-url http://pvtbuilds.pvt.build.mozilla.org/pub/mozilla.org/b2g/try-builds/armenzg@mozilla.com-d6bafd17ba5d/try-emulator/b2g-32.0a1.en-US.android-arm.tests.zip
cd build/tests/mochitest
/builds/slave/test/build/venv/bin/python runtestsb2g.py --adbpath=/builds/slave/test/build/emulator/b2g-distro/out/host/linux-x86/bin/adb --b2gpath=/builds/slave/test/build/emulator/b2g-distro --console-level=INFO --emulator=arm --logcat-dir=/builds/slave/test/build --remote-webserver=10.0.2.2 --xre-path=/builds/slave/test/build/xre/bin --symbols-path=http://pvtbuilds.pvt.build.mozilla.org/pub/mozilla.org/b2g/try-builds/armenzg@mozilla.com-d6bafd17ba5d/try-emulator/emulator.crashreporter-symbols.zip --busybox=/builds/slave/test/build/busybox --total-chunks=9 --this-chunk=1 --quiet --certificate-path=/builds/slave/test/build/tests/certs
Attachment #8433546 - Attachment is obsolete: true
This is great.

I see the jobs running, however, IIUC, I will have to touch a bunch of manifests to disable tests until we can get time to go and fix them.

jgriffin, ahal or jmaher: do you agree with this statement?
https://tbpl.mozilla.org/?tree=Try&rev=fd6871af9796
if the build system filters out features based on the same manifests and conditions that mochitests is doing we should be at parity.  Although I wonder if there is something we are overlooking in this system.

looking at ics emulator opt M(1), we fail on test_csp_report.html:
http://dxr.mozilla.org/mozilla-central/source/content/base/test/csp/mochitest.ini#158

this is commented out for buildapp == 'b2g'.  Is this the right filter?  If this is, then we have a bug in our manifest parsing.  

:armenzg, are you using mozinfo.json in your patch?  can you try editing the manifest to be:
skip-if = e10s || (buildapp == 'b2g')  <-- notice the parans () around the buildapp == 'b2g'

if you do this, you could reschedule with arbitrary build api and save a build :)

:ahal, can you confirm that buildapp == 'b2g' will not run a test for b2g ics emulator opt?
Flags: needinfo?(armenzg)
Flags: needinfo?(ahalberstadt)
I'm pretty sure buildapp == 'b2g' catches both emulator and b2g_desktop. To distinguish b2g_desktop from emulator you need to use toolkit != 'gonk'
Flags: needinfo?(ahalberstadt)
We should have some results in 30 minutes:
https://tbpl.mozilla.org/?tree=Try&rev=fd6871af9796&jobname=b2g_emulator_vm%20try%20opt%20test%20mochitest-1
Flags: needinfo?(armenzg)
Attached patch b2g_tests.diff (obsolete) — Splinter Review
It seems that it is related.

We now have a new set of tests that should not be running (IIUC):
https://tbpl.mozilla.org/php/getParsedLog.php?id=41223404&tree=Try&full=1#error0
[test_permission_gum_remember.html]
run-if = toolkit == "gonk"

I will create a new tests.zip. I've been able to create this patch with:
for file in `find . -name "*.ini"`; do sed s/[^\(]buildapp\ ==\ \'b2g\'/\ \(buildapp\ ==\ \'b2g\'\)/ < $file > $file.new && mv $file.new $file; done
I filed the parsing issues in bug 1021898.
Switch my working repo to be mozilla-central (and never going back).
I pushed a bad m-i revision.

Updated patch and pushed to test on all builds/platforms. We should see on Monday:
https://tbpl.mozilla.org/?tree=Try&rev=b7aceb4a3432
ted is asking to fix the issue in bug 1021898 instead of patching the manifests.
I will be looking to figure out how to write a unit test for it and be sure that we get it fixed.
This patch has yielded the expected results [1]. I'm now increase the try push. [2]

The manifests were not bad. It was a red-herring.
The wallpapering was not actually even working.

The reason that disabled tests were being run is because we were not passing options.manifestFile.
This would make mochitest to use the chunkification of the directories without any knowledge of disabled tests.
Passing option.manifestFile to the self.urlOpts does the trick.

     def setup_common_options(self, options):
         test_url = self.buildTestPath(options)
+        if options.manifestFile:
+            # buildURLOptions has already been called so we're hacking it here
+            self.urlOpts.append("manifestFile=%s" % options.manifestFile)
         if len(self.urlOpts) > 0:
             test_url += "?" + "&".join(self.urlOpts)
         self.test_script_args.append(test_url)

[1] https://tbpl.mozilla.org/?tree=Try&rev=3bb93c4c7f05&jobname=b2g_emulator_vm%20try%20opt%20test%20mochitest-
[2] https://tbpl.mozilla.org/?tree=Try&rev=6610f4646b3c
Attachment #8435268 - Attachment is obsolete: true
Attachment #8435958 - Attachment is obsolete: true
You can see the results in here:
https://tbpl.mozilla.org/?tree=Try&rev=6610f4646b3c

Unrelated note, I'm sincerely disturbed at the high number of intermittent oranges that a push can get.
Attachment #8437791 - Attachment is obsolete: true
Attachment #8437980 - Flags: review?(jmaher)
Comment on attachment 8437980 [details] [diff] [review]
At build time add all disabled tests + at runtime do not add all disabled tests + switch b2g runners to use manifest

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

this is looking much better!

::: testing/mochitest/runtests.py
@@ +455,3 @@
>      """
> +    self.testRoot = self.getTestRoot(options)
> +    self.testRootAbs = os.path.join(SCRIPT_DIR, self.testRoot)

don't we define these two variables elsewhere in the code?
Attachment #8437980 - Flags: review?(jmaher) → review+
(In reply to Joel Maher (:jmaher) from comment #76)
> 
> ::: testing/mochitest/runtests.py
> @@ +455,3 @@
> >      """
> > +    self.testRoot = self.getTestRoot(options)
> > +    self.testRootAbs = os.path.join(SCRIPT_DIR, self.testRoot)
> 
> don't we define these two variables elsewhere in the code?

It has to do with the order on which the various functions get called.
Would you like me to add a comment pointing this out? or create python properties?
Pushing as during r+.
I will follow up last comment when jmaher is back.
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec353cd772b1

With regards to the python properties I suggested, that is not an easy approach since to determine the value we also have to pass a variable "options" and that is the opposite of what @property works like (IIUC) unless there's a way to pass a value.

Leaving NI on jmaher to follow up change.
Flags: needinfo?(jmaher)
Backed out for a row of B2G emulator mochitest failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/ccf7bd357e8b
I'm going to be looking into pushing to Cedar with a CLOBBER request.
I thought I might have landed the wrong patch but I don't think so.

I tried pushing to Try again and it is all green (worry-some green):
https://tbpl.mozilla.org/?tree=Try&rev=ec47fafd0a98

I'm also going to pushed based on m-i:
https://tbpl.mozilla.org/?tree=Try&rev=576754a17c8a

I'm also pushing to Cedar with CLOBBER:
https://tbpl.mozilla.org/?tree=Cedar

I don't know what is different from the m-i push (as I have run a diff between the two pushes):
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=ec353cd772b1&jobname=b2g.*test
https://tbpl.mozilla.org/?tree=Try&rev=6610f4646b3c&jobname=b2g.*test
Please disregard comment 81.
I will review it tomorrow when I have all results.
any idea why these are failing?  a change that landed between your try push and inbound push?
Flags: needinfo?(jmaher)
https://tbpl.mozilla.org/?tree=Try&rev=e9a7a9baa206

It seems I lost my working patch somewhere.
Barely any changes from the previous patch.
The main difference that makes it work is that I add the manifestFile information to the urlOps within setup_common_options instead of outside that way it makes it into self.test_script_args.append(test_url).

jmaher, what would you like me to do about comment 77?
Attachment #8437980 - Attachment is obsolete: true
Attachment #8439555 - Flags: review?(jmaher)
Comment on attachment 8439555 [details] [diff] [review]
[checked-in] At build time add all disabled tests + at runtime do not add all disabled tests + switch b2g runners to use manifest

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

for comment 77 about self.testrootabs, etc. I want to ensure we are only defining it once in the script.
Attachment #8439555 - Flags: review?(jmaher) → review+
Comment on attachment 8439555 [details] [diff] [review]
[checked-in] At build time add all disabled tests + at runtime do not add all disabled tests + switch b2g runners to use manifest

https://hg.mozilla.org/integration/mozilla-inbound/rev/9d709ee57c7c

Meanwhile, I'm working on comment 77 on try.
Attachment #8439555 - Attachment description: At build time add all disabled tests + at runtime do not add all disabled tests + switch b2g runners to use manifest → [checked-in] At build time add all disabled tests + at runtime do not add all disabled tests + switch b2g runners to use manifest
Whiteboard: [leave open]
This is tested in here:
https://tbpl.mozilla.org/?tree=Try&rev=5c99391b052b

I think that the function might be more proper to be called initTestRoot() than setTestRoot()
Attachment #8440910 - Flags: review?(jmaher)
Comment on attachment 8440910 [details] [diff] [review]
[checked-in] self.testRoot and self.testRootAbs are only to be set in one place

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

thanks for the cleanup!
Attachment #8440910 - Flags: review?(jmaher) → review+
Comment on attachment 8440910 [details] [diff] [review]
[checked-in] self.testRoot and self.testRootAbs are only to be set in one place

https://hg.mozilla.org/integration/mozilla-inbound/rev/c257e034da91
Attachment #8440910 - Attachment description: self.testRoot and self.testRootAbs are only to be set in one place → [checked-in] self.testRoot and self.testRootAbs are only to be set in one place
jgriffin: do we need to uplift anything? or are we done in here?
No longer depends on: 1019962
See Also: → 1019962
Whiteboard: [leave open]
I don't think we need to uplift anything, but let's confirm with jmaher.
Flags: needinfo?(jmaher)
https://hg.mozilla.org/mozilla-central/rev/c257e034da91
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
The primary usecase for this is to get reports of the tests enabled/disabled.  Until we have this understood more, sticking on m-c will be all we need.  Secondary use cases are theoretical now.  I suspect when we are ready to scale out with this change v33 will be on Aurora.  Lets keep it simple and not worry about uplifting to Aurora.
\o/

My first official ateam bug :)
Flags: needinfo?(jmaher)
(In reply to Armen Zambrano [:armenzg] (EDT/UTC-4) from comment #95)
> \o/
> 
> My first official ateam bug :)

Congratulations!
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.