Install all tests (even disabled) from manifests

RESOLVED FIXED in mozilla33

Status

()

Core
Build Config
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: ted, Assigned: Armen - back on June 11th)

Tracking

(Blocks: 1 bug, {ateam-unittests-task})

unspecified
mozilla33
ateam-unittests-task
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 10 obsolete attachments)

15.40 KB, patch
jmaher
: review+
Details | Diff | Splinter Review
3.35 KB, patch
jmaher
: review+
Details | Diff | Splinter Review
(Reporter)

Description

3 years ago
Super-tiny fix, just change the value of disabled here:
http://mxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/frontend/emitter.py#482
changing disabled=True, yields build errors, I will look into this more next week.
Keywords: ateam-unittests-task
Blocks: 994920
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!
(Assignee)

Comment 4

3 years ago
I will give it a shot next week.
Assignee: nobody → armenzg
(Assignee)

Updated

3 years ago
Blocks: 996183
Blocks: 945869
: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)
(Assignee)

Comment 6

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

Comment 8

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

Comment 9

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

Comment 11

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

Comment 12

3 years ago
FTR in one of the manifests is disabled.

I can avoid duplicates; works for everyone?
(Reporter)

Comment 13

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

Comment 14

3 years ago
Created attachment 8418973 [details] [diff] [review]
all_tests.diff

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+
(Assignee)

Comment 16

3 years ago
Created attachment 8421014 [details] [diff] [review]
All tests get installed plus remove two tests that expect that behaviour
Attachment #8421014 - Flags: review?(jmaher)
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+
(Assignee)

Comment 18

3 years ago
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]
(Reporter)

Comment 19

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

Comment 21

3 years ago
OK cool.

Let me push to try before landing on m-i.
(Assignee)

Comment 22

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=9453e7e5a514
(Assignee)

Comment 23

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

Comment 24

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

Comment 25

3 years ago
Created 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

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
(Assignee)

Comment 26

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

Comment 28

3 years ago
Created attachment 8421723 [details] [diff] [review]
Disabled tests get installed + remove two tests that expect that behaviour + comment test which exists in mochitest-oop.ini

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)
(Assignee)

Comment 29

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

Comment 30

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

Comment 31

3 years ago
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.
(Reporter)

Comment 33

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

Comment 34

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

Comment 36

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

Comment 38

3 years ago
:(

I'm very sorry about this. I will be more careful.
(Assignee)

Comment 39

3 years ago
I'm back to this while the mulet work is being reviewed.

I will review those failures and try to reproduce them locally.
(Assignee)

Comment 40

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

Comment 43

3 years ago
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?
here is where we do it for mochitest-plain:
http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/runtests.py#449
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.
(Assignee)

Comment 47

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

Comment 48

3 years ago
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.
Actually you probably just want to follow these instructions:  https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Linux_Prerequisites
(Assignee)

Comment 51

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

Comment 53

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

Comment 54

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

Comment 55

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

Comment 57

3 years ago
Created attachment 8430334 [details]
output.txt

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

Comment 60

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

Updated

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

Comment 61

3 years ago
Created attachment 8433546 [details] [diff] [review]
[pushed to try] Disabled tests get installed + remove two tests that expect that behaviour + comment test which exists in mochitest-oop.ini + switch b2g desktop to read manifests
Attachment #8421723 - Attachment is obsolete: true
Attachment #8430334 - Attachment is obsolete: true
(Assignee)

Comment 62

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

Comment 63

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

Updated

3 years ago
Depends on: 1019962
(Assignee)

Comment 64

3 years ago
Created attachment 8435268 [details] [diff] [review]
[pushed to try] Disabled tests get installed + remove two tests that expect that behaviour + comment test which exists in mochitest-oop.ini + switch b2g desktop to read manifests + fix b2g emulator

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
(Assignee)

Comment 65

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

Comment 68

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

Comment 69

3 years ago
Created attachment 8435958 [details] [diff] [review]
b2g_tests.diff

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
(Assignee)

Comment 70

3 years ago
New build pushed:
https://tbpl.mozilla.org/?tree=Try&rev=f6edd210fcc3
(Assignee)

Comment 71

3 years ago
I filed the parsing issues in bug 1021898.
(Assignee)

Comment 72

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

Comment 73

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

Comment 74

3 years ago
Created attachment 8437791 [details] [diff] [review]
[pushed to try] At build time add all disabled tests + at runtime do not add all disabled tests + switch b2g runners to use manifest

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
(Assignee)

Comment 75

3 years ago
Created 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

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+
(Assignee)

Comment 77

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

Comment 78

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

Comment 80

3 years ago
I'm going to be looking into pushing to Cedar with a CLOBBER request.
(Assignee)

Comment 81

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

Comment 82

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

Comment 84

3 years ago
Created 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://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
(Assignee)

Updated

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

Comment 86

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

Updated

3 years ago
Whiteboard: [leave open]
(Assignee)

Comment 87

3 years ago
Created attachment 8440910 [details] [diff] [review]
[checked-in] self.testRoot and self.testRootAbs are only to be set in one place

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+
https://hg.mozilla.org/mozilla-central/rev/9d709ee57c7c
(Assignee)

Comment 90

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

Comment 91

3 years ago
jgriffin: do we need to uplift anything? or are we done in here?
No longer depends on: 1019962
See Also: → bug 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
Last Resolved: 3 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.
(Assignee)

Comment 95

3 years ago
\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!
You need to log in before you can comment on or make changes to this bug.