Closed
Bug 989583
Opened 10 years ago
Closed 10 years ago
Install all tests (even disabled) from manifests
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
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 |
Super-tiny fix, just change the value of disabled here: http://mxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/frontend/emitter.py#482
Comment 1•10 years ago
|
||
changing disabled=True, yields build errors, I will look into this more next week.
Updated•10 years ago
|
Keywords: ateam-unittests-task
Comment 2•10 years ago
|
||
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.
Comment 3•10 years ago
|
||
great idea!
Comment 5•10 years ago
|
||
: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•10 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)
Comment 7•10 years ago
|
||
*I* would prefer that you look at this, but I trust your judgement on relative priorities.
Assignee | ||
Comment 8•10 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•10 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.
Comment 10•10 years ago
|
||
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•10 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•10 years ago
|
||
FTR in one of the manifests is disabled. I can avoid duplicates; works for everyone?
Reporter | ||
Comment 13•10 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•10 years ago
|
||
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 15•10 years ago
|
||
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•10 years ago
|
||
Attachment #8421014 -
Flags: review?(jmaher)
Comment 17•10 years ago
|
||
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•10 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•10 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.
Comment 20•10 years ago
|
||
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•10 years ago
|
||
OK cool. Let me push to try before landing on m-i.
Assignee | ||
Comment 22•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=9453e7e5a514
Assignee | ||
Comment 23•10 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•10 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•10 years ago
|
||
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•10 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 27•10 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 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•10 years ago
|
||
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•10 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•10 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•10 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.
Comment 32•10 years ago
|
||
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•10 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•10 years ago
|
||
Oh! That means that we should be good with my current patch.
Comment 35•10 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 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•10 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
Comment 37•10 years ago
|
||
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•10 years ago
|
||
:( I'm very sorry about this. I will be more careful.
Assignee | ||
Comment 39•10 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•10 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.
Comment 41•10 years ago
|
||
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)
Comment 42•10 years ago
|
||
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•10 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.
Comment 44•10 years ago
|
||
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?
Comment 45•10 years ago
|
||
here is where we do it for mochitest-plain: http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/runtests.py#449
Comment 46•10 years ago
|
||
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•10 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•10 years ago
|
||
It is running on the loaner. I will read on the code from comment 45 and comment 46.
Comment 49•10 years ago
|
||
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.
Comment 50•10 years ago
|
||
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•10 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
Comment 52•10 years ago
|
||
Hey Armen, ping myself or ahal on Monday/Tuesday and we can sort this out.
Assignee | ||
Comment 53•10 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•10 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•10 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
Comment 56•10 years ago
|
||
we read the .ini files and create tests.json which can be easily read by the in browser harness.
Assignee | ||
Comment 57•10 years ago
|
||
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
Comment 58•10 years ago
|
||
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.
Comment 59•10 years ago
|
||
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•10 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•10 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•10 years ago
|
||
Attachment #8421723 -
Attachment is obsolete: true
Attachment #8430334 -
Attachment is obsolete: true
Assignee | ||
Comment 62•10 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•10 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 | ||
Comment 64•10 years ago
|
||
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•10 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
Comment 66•10 years ago
|
||
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)
Comment 67•10 years ago
|
||
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•10 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•10 years ago
|
||
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•10 years ago
|
||
New build pushed: https://tbpl.mozilla.org/?tree=Try&rev=f6edd210fcc3
Assignee | ||
Comment 71•10 years ago
|
||
I filed the parsing issues in bug 1021898.
Assignee | ||
Comment 72•10 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•10 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•10 years ago
|
||
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•10 years ago
|
||
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 76•10 years ago
|
||
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•10 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•10 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)
Comment 79•10 years ago
|
||
Backed out for a row of B2G emulator mochitest failures. https://hg.mozilla.org/integration/mozilla-inbound/rev/ccf7bd357e8b
Assignee | ||
Comment 80•10 years ago
|
||
I'm going to be looking into pushing to Cedar with a CLOBBER request.
Assignee | ||
Comment 81•10 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•10 years ago
|
||
Please disregard comment 81. I will review it tomorrow when I have all results.
Comment 83•10 years ago
|
||
any idea why these are failing? a change that landed between your try push and inbound push?
Flags: needinfo?(jmaher)
Assignee | ||
Comment 84•10 years ago
|
||
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•10 years ago
|
Attachment #8439555 -
Flags: review?(jmaher)
Comment 85•10 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 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•10 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•10 years ago
|
Whiteboard: [leave open]
Assignee | ||
Comment 87•10 years ago
|
||
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 88•10 years ago
|
||
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+
Assignee | ||
Comment 90•10 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•10 years ago
|
||
jgriffin: do we need to uplift anything? or are we done in here?
Comment 92•10 years ago
|
||
I don't think we need to uplift anything, but let's confirm with jmaher.
Flags: needinfo?(jmaher)
Comment 93•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c257e034da91
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Comment 94•10 years ago
|
||
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.
Comment 96•10 years ago
|
||
(In reply to Armen Zambrano [:armenzg] (EDT/UTC-4) from comment #95) > \o/ > > My first official ateam bug :) Congratulations!
Updated•6 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•