847279 - some makefiles have bad relativesrcdir, making it hard to find tests

Status: RESOLVED FIXED

Description

[David Baron :dbaron: ⌚️UTC-7]

There are some makefiles where relativesrcdir is not the same as the location of the makefile.  Since relativesrcdir is used for exporting tests, this ends up making it hard to find tests from error messages (because the path doesn't match), and hard to run tests after modifying them (because the TEST_PATH argument is different from what is expected).

I tried to mass-fix these at one point in the past, but ran into the problem that some tests depended on the incorrect relativesrcdir.

We should fix these.

Comment 1

[David Baron :dbaron: ⌚️UTC-7]

Let's see what happens:
https://tbpl.mozilla.org/?tree=Try&rev=ecfa9a8ebf09

Those 2 patches don't quite fix all of them.  But I started from:
find * -name Makefile.in -exec sed -i -e 's/^\(relativesrcdir[ \t]*=[ \t]*\).*/\1@relativesrcdir@/' {} \;
and then pared down the changes.

Comment 2

[David Baron :dbaron: ⌚️UTC-7]

The patches I'm testing are:
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/c4b9a5aacf3f/a11y-mochitests
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/c4b9a5aacf3f/fix-relativesrcdir
and the changes I dropped were:
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-rev/c4b9a5aacf3f

Comment 3

[:Ms2ger (⌚ UTC+1/+2)]

You should be able to just remove the relativesrcdir from dom/imptests/Makefile.in.

Comment 4

[David Baron :dbaron: ⌚️UTC-7]

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #1)
> Let's see what happens:
> https://tbpl.mozilla.org/?tree=Try&rev=ecfa9a8ebf09
> 
> Those 2 patches don't quite fix all of them.  But I started from:
> find * -name Makefile.in -exec sed -i -e 's/^\(relativesrcdir[ \t]*=[
> \t]*\).*/\1@relativesrcdir@/' {} \;
> and then pared down the changes.

The problems were:

mochitest-3 and mochitest-ipcplugins:

08:40:35 INFO - 10746 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/plugins/test/mochitest/test_redirect_handling.html | Test for expected stream notification status. - got 1, expected 0

mochitest-5:


00:01:59     INFO -  163651 ERROR TEST-UNEXPECTED-FAIL | /tests/security/manager/ssl/tests/mochitest/mixedcontent/test_bug329869.html | Test timed out.
00:07:29     INFO -  163656 ERROR TEST-UNEXPECTED-FAIL | /tests/security/manager/ssl/tests/mochitest/mixedcontent/test_bug383369.html | Test timed out.
00:12:59     INFO -  163663 ERROR TEST-UNEXPECTED-FAIL | /tests/security/manager/ssl/tests/mochitest/mixedcontent/test_bug455367.html | Test timed out.
00:18:29     INFO -  163668 ERROR TEST-UNEXPECTED-FAIL | /tests/security/manager/ssl/tests/mochitest/mixedcontent/test_bug472986.html | Test timed out.
00:18:29     INFO -  163669 ERROR TEST-UNEXPECTED-FAIL | (SimpleTest/TestRunner.js) | 4 test timeouts, giving up.
00:18:29     INFO -  163670 ERROR TEST-UNEXPECTED-FAIL | (SimpleTest/TestRunner.js) | Skipping 105 remaining tests.
00:18:30     INFO -  163671 ERROR TEST-UNEXPECTED-FAIL | (SimpleTest/TestRunner.js) | /tests/security/manager/ssl/tests/mochitest/mixedcontent/test_bug472986.html finished in a non-clean fashion, probably because it didn't call SimpleTest.finish()


mochitest-chrome:

23:52:51     INFO -  4222 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/dom/plugins/test/mochitest/test_privatemode_perwindowpb.xul | uncaught exception - NS_ERROR_FILE_NOT_FOUND: Component returned failure code: 0x80520012 (NS_ERROR_FILE_NOT_FOUND) [nsIWebNavigation.loadURI] at chrome://global/content/bindings/browser.xml:156
23:52:52     INFO -  4224 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/dom/plugins/test/mochitest/test_privatemode_perwindowpb.xul | Exception thrown getting private mode state. - got true, expected false
23:52:52     INFO -  4225 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/dom/plugins/test/mochitest/test_privatemode_perwindowpb.xul | Querying private mode reported incorrectly - got undefined, expected true
23:52:52     INFO -  4226 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/dom/plugins/test/mochitest/test_privatemode_perwindowpb.xul | Querying private mode reported incorrectly - got undefined, expected true
23:52:52     INFO -  4227 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/dom/plugins/test/mochitest/test_privatemode_perwindowpb.xul | uncaught exception - TypeError: pluginElement1 is null at chrome://mochitests/content/chrome/dom/plugins/test/mochitest/test_privatemode_perwindowpb.xul:91
23:52:52     INFO -  4228 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/dom/plugins/test/mochitest/test_privatemode_perwindowpb.xul | [SimpleTest.finish()] this test already called finish!
23:52:52     INFO -  4229 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/dom/plugins/test/mochitest/test_privatemode_perwindowpb.xul | called finish() multiple times
23:58:12     INFO -  4245 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/dom/plugins/test/mochitest/test_xulbrowser_plugin_visibility.xul | Test timed out.
23:58:52     INFO -  9951 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/dom/tests/mochitest/localstorage/test_localStorageBasePrivateBrowsing_perwindowpb.html | private browsing values threw away - got private browsing value, expected null
23:58:52     INFO -  9952 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/dom/tests/mochitest/localstorage/test_localStorageBasePrivateBrowsing_perwindowpb.html | No items - got 1, expected 0
00:00:46     INFO -  12041 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/layout/base/tests/chrome/test_bug551434.html | uncaught exception - TypeError: frames[0].document.getElementById(...) is null at chrome://mochitests/content/chrome/layout/base/tests/chrome/test_bug551434.html:1
00:00:46     INFO -  12045 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/layout/base/tests/chrome/test_bug551434.html | keydown on input 4 - got undefined, expected 0
00:00:46     INFO -  12046 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/layout/base/tests/chrome/test_bug551434.html | keypress on input 4 - got undefined, expected 0
00:00:46     INFO -  12047 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/layout/base/tests/chrome/test_bug551434.html | keyup on input 4 - got undefined, expected 0
00:00:46     INFO -  12049 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/layout/base/tests/chrome/test_bug551434.html | uncaught exception - TypeError: i4 is null at chrome://mochitests/content/chrome/layout/base/tests/chrome/test_bug551434.html:68
00:00:46     INFO -  12050 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/layout/base/tests/chrome/test_bug551434.html | [SimpleTest.finish()] this test already called finish!
00:00:46     INFO -  12051 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/layout/base/tests/chrome/test_bug551434.html | called finish() multiple times
00:04:26     INFO -  14034 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/security/manager/ssl/tests/mochitest/stricttransportsecurity/test_sts_privatebrowsing_perwindowpb.html | in ROUND plain, test samedom - got INSECURE, expected SECURE
00:04:28     INFO -  14048 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/security/manager/ssl/tests/mochitest/stricttransportsecurity/test_sts_privatebrowsing_perwindowpb.html | in ROUND subdom, test samedom - got INSECURE, expected SECURE
00:04:28     INFO -  14050 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/security/manager/ssl/tests/mochitest/stricttransportsecurity/test_sts_privatebrowsing_perwindowpb.html | in ROUND subdom, test subdom - got INSECURE, expected SECURE
00:05:38     INFO -  16118 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/toolkit/components/places/tests/test_bug_461710_perwindowpb.html | uncaught exception - TypeError: gIframe is null at chrome://mochitests/content/chrome/toolkit/components/places/tests/test_bug_461710_perwindowpb.html:106
00:15:55     INFO -  32137 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/toolkit/mozapps/downloads/tests/chrome/test_close_on_last_window.xul | Test timed out.
00:15:59     INFO -  32142 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/toolkit/mozapps/downloads/tests/chrome/test_delete_key_removes.xul | [SimpleTest.finish()] this test already called finish!
00:16:39     INFO -  32213 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/toolkit/mozapps/downloads/tests/chrome/test_space_key_pauses_resumes.xul | file was downloaded - got http://example.com/httpd.js, expected http://mochi.test:8888/chrome/toolkit/mozapps/downloads/tests/chrome/unknownContentType_dialog_layout_data.pif
00:16:40     INFO -  32215 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/toolkit/mozapps/downloads/tests/chrome/test_space_key_pauses_resumes.xul | file destination was set
00:16:40     INFO -  32216 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/toolkit/mozapps/downloads/tests/chrome/test_space_key_pauses_resumes.xul | file name was set
00:16:40     INFO -  32217 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/toolkit/mozapps/downloads/tests/chrome/test_space_key_pauses_resumes.xul | called finish() multiple times
00:19:36     INFO -  36997 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/widget/tests/test_bug462106_perwindow.xul | Data no longer available after leaving the private browsing mode - didn't expect another random number: 0.9816515821892492, but got it

mochitest-a11y:

00:27:44     INFO -  4462 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/hittest/test_zoom_tree.xul | Test timed out.
00:27:46     INFO -  4463 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/hittest/test_zoom_tree.xul | [SimpleTest.finish()] waitForFocus() was called a different number of times from the number of callbacks run.  Maybe the test terminated prematurely -- be sure to use SimpleTest.waitForExplicitFinish(). - got 1, expected 0
00:33:15     INFO -  5471 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/pivot/test_virtualcursor.html | Test timed out.
00:33:16     INFO -  5472 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/pivot/test_virtualcursor.html | [SimpleTest.finish()] waitForFocus() was called a different number of times from the number of callbacks run.  Maybe the test terminated prematurely -- be sure to use SimpleTest.waitForExplicitFinish(). - got 1, expected 0
00:39:29  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/states/test_doc_busy.html | application timed out after 330 seconds with no output
00:39:36     INFO -  PROCESS-CRASH | chrome://mochitests/content/a11y/accessible/tests/mochitest/states/test_doc_busy.html | application crashed [@ libc-2.11.so + 0xd4aa3]
00:39:40    ERROR - Return code: 1

Comment 5

[David Baron :dbaron: ⌚️UTC-7]

Current versions of the patches are:
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/tip/a11y-mochitests
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/tip/fix-relativesrcdir

Comment 6

[Ben Kelly [PTO, back July 24][:bkelly]]

Created attachment 732191 [details] [diff] [review]
mochitest-chrome fixes

Fixes for the mochitest-chrome failures.  Builds off of patches referenced in comment 5.

Note, I could note reproduce errors for the following tests.  They look like
some may be intermittent timing issues.

 - dom/plugins/test/mochitest/test_xulbrowser_plugin_visibility.xul
 - toolkit/mozapps/downloads/tests/chrome/test_close_on_last_window.xul
 - toolkit/mozapps/downloads/tests/chrome/test_delete_key_removes.xul
 - toolkit/mozapps/downloads/tests/chrome/test_space_key_pauses_resumes.xul

Comment 7

[Ben Kelly [PTO, back July 24][:bkelly]]

Created attachment 733717 [details] [diff] [review]
Fix hardcoded path names in mochitests to match relativesrcdir.

Updated patch with fixes for:

 * dom/plugins/test/mochitest/test_redirect_handling.html
 * security/manager/ssl/tests/mochitest/mixedcontent/*

Note, some of the mixedcontent tests were strangely sensitive to end-of-file
characters.  That seemed like a bug to me, but I did not have time to
investigate if it was just a problem with the mochitest or the code under
test.

Just need to fix the ally tests and then this initial batch of errors should
be done.

Comment 8

[Ben Kelly [PTO, back July 24][:bkelly]]

Actually, ignore that bit about sensitivity to end-of-file newlines.  It
appears its just the browser accepting content outside of the <body>
element as if it was in the body for compatibility.  This causes the
end-of-file newline to show up in the body which throws off the innerHTML
equality to the empty string in some tests.

Comment 9

[Ben Kelly [PTO, back July 24][:bkelly]]

Created attachment 734249 [details] [diff] [review]
Fix hardcoded path names in mochitests to match relativesrcdir.

Updated with a11y fixes and a few more errors I found.

My poor 4-yo laptop is taking its time running the full suite and my try-server account is not active yet.  If anyone could run this through try, that would be great.

Comment 10

[Josh Matthews [:jdm]]

Here you go: windows, mac, and linux, all mochitest suites.

https://tbpl.mozilla.org/?tree=Try&rev=587fe00f45b5

Comment 11

[Ben Kelly [PTO, back July 24][:bkelly]]

Thanks!  Did you include dbaron's patches from comment 5?  I wasn't sure if I should roll those in or add them as separate attachments, etc.  I'm new here.

Comment 12

[Ben Kelly [PTO, back July 24][:bkelly]]

Created attachment 734260 [details] [diff] [review]
dbaron's a11y patch from comment 5.

Comment 13

[Ben Kelly [PTO, back July 24][:bkelly]]

Created attachment 734261 [details] [diff] [review]
dbaron's other mochitest patch from comment 5.

Comment 14

[Ben Kelly [PTO, back July 24][:bkelly]]

Yea, looks like the build failed because I hadn't added dbaron's patches.  Sorry about that.

Comment 15

[Josh Matthews [:jdm]]

Thanks for clearing that up!

https://tbpl.mozilla.org/?tree=Try&rev=a2a148a876de

Comment 16

[Ben Kelly [PTO, back July 24][:bkelly]]

Thanks, that build went much better.  Looks like there are still errors in:

 * accessible/jsat/test_utterance_order.html
 * toolkit/components/satchel/test/test_bug_511615.html
 * toolkit/components/satchel/test/test_form_autocomplete.html

It looks like test_utterance_order.html was just added and its relativesrcdir
will need to be fixed as well.

The two satchel tests pass on my macbook.  From the try output, it looks like
they only fail on win7/8.  Seems unlikely to be related to the relativesrcdir
changes, but I haven't looked yet.

Comment 17

[Ben Kelly [PTO, back July 24][:bkelly]]

Also these two appear related to the relativesrcdir changes, but are windows
specific tests:

 * dom/plugins/test/mochitest/test_busy_hang.xul
 * dom/plugins/test/mochitest/test_idle_hang.xul

Comment 18

[Ben Kelly [PTO, back July 24][:bkelly]]

Created attachment 734406 [details] [diff] [review]
Fix hardcoded path names in mochitests to match relativesrcdir.

Updated to fix the errors in:

 * accessible/jsat/test_utterance_order.html
 * dom/plugins/test/mochitest/test_busy_hang.xul
 * dom/plugins/test/mochitest/test_idle_hang.xul

The two dom tests are untested since I don't have a windows dev environment
currently.  The changes looked straightforward, though.

Comment 19

[Josh Matthews [:jdm]]

Shout if you'd like another push to try.

Comment 20

[Ben Kelly [PTO, back July 24][:bkelly]]

So I don't think the errors in toolkit/components/satchel/test are related
to the relativesrcdir changes.  The relativesrcdir was not
modified in this directory.

Josh, would you be so kind as to kick off another try?  Thanks!

If this resolves the errors, are you able to review/commit?  Or should I flag dbaron for when he returns from vacation?

Comment 22

[Matt Brubeck (:mbrubeck)]

Comment on attachment 734260 [details] [diff] [review]
dbaron's a11y patch from comment 5.

I just landed a similar change in bug 847279 -- sorry for the duplicate work.

Comment 23

[Ben Kelly [PTO, back July 24][:bkelly]]

No problem.  I think attachment 734406 [details] [diff] [review] will conflict in a couple places with you patch.  I can fix that later this evening if no one else gets to it before then.

Also, it looks like you have a better solution using getRootDirectory().  I wasn't
aware of that call.  I'll look to see if any of my other changes would be better
fixed using that.

Thanks!

Comment 24

[Ben Kelly [PTO, back July 24][:bkelly]]

Created attachment 734970 [details] [diff] [review]
Fix hardcoded path names in mochitests to match relativesrcdir.

Updated to account for fixes from bug 854868.

Also attempt to use getRootDirectory() where possible to avoid hardcoding
paths.  I only found one place that could do this, though.  (Seems to only
work for chrome:// URLs?)

Still waiting for my level 1 access, so if jdm or someone else could kick off
a try that would be great.  Thanks!

Comment 25

[Josh Matthews [:jdm]]

https://tbpl.mozilla.org/?tree=Try&rev=4ee73fed7103

Comment 26

[Ben Kelly [PTO, back July 24][:bkelly]]

Thanks!  Looks like Linux and Mac OS X passed, but there was a try-server timeout
on the windows platforms.  Is there an easy way to requeue the build request?

Comment 27

[Josh Matthews [:jdm]]

Yep. I have retriggered it.

Comment 28

[Ben Kelly [PTO, back July 24][:bkelly]]

Looks like the try succeeded.  Again, the errors in component/satchel look
unrelated since relativesrcdir was not changed in that directory.

Matt, since you landed the similar patch for bug 854868, I thought you might be
a good person to review.

Comment 29

[Matt Brubeck (:mbrubeck)]

Comment on attachment 734261 [details] [diff] [review]
dbaron's other mochitest patch from comment 5.

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

Looks good to me, though it might be good to get a review from peers of each of the modules these live in -- or at least alert them, since it'll mean a minor workflow change for developers of those modules.  Requesting feedback from some module peers.

Comment 30

[Matt Brubeck (:mbrubeck)]

Comment on attachment 734970 [details] [diff] [review]
Fix hardcoded path names in mochitests to match relativesrcdir.

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

Thanks a *ton* for doing this!  Again, all these changes look correct to me, but I'm flagging some module peers to alert them to the changes and give them a chance to review if they choose.

Comment 31

[Matt Brubeck (:mbrubeck)]

(In reply to Ben Kelly from comment #28)
> Looks like the try succeeded.  Again, the errors in component/satchel look
> unrelated since relativesrcdir was not changed in that directory.

We should make sure we can get a Try push with green on all platforms before landing these patches.  The Windows satchel failures are suspicious because they are not intermittent and they did not show up on the parent changeset:
https://tbpl.mozilla.org/?rev=b1fb34b07c17

I don't see anything suspicious in the failing test (test_bug_511615.html); it's also possible behavior has changed in some earlier test that is leaving things in a bad state.

The test_xulbrowser_plugin_visibility.xul failure on Fedora64 also appears to be new; I retriggered to see if it's intermittent or not.

Comment 32

[Ben Kelly [PTO, back July 24][:bkelly]]

Thanks Matt!

I agree the satchel tests are concerning.  Unfortunately I haven't had any luck
triggering these errors on my Mac and I don't have a windows dev
environment at the moment.  I did look through it at one point and didn't see
any obvious paths that needed to be fixed.  I didn't consider cross-test state
issues, though.

Comment 33

[Ben Kelly [PTO, back July 24][:bkelly]]

Created attachment 736121 [details] [diff] [review]
Updated Fixes for hardcoded path names in mochitests to match relativesrcdir.

Found another path issue with the test case failing on Fedora.  It was a linux
only test case.  (Thanks for prodding me to go back and look at the platform
specific failures!)

I still haven't figured out or reproduced the satchel errors on windows.  Does
anyone have a windows environment that can reproduce this?

Comment 34

[Matt Brubeck (:mbrubeck)]

I can try this locally on Windows, though I might not get to it until sometime next week.

Comment 35

[Matt Brubeck (:mbrubeck)]

toolkit/components/satchel/test/test_bug_511615.html succeeds when I run it alone, but fails on my Windows 8 laptop just like it did on Try when I run:

  _virtualenv/Scripts/python _tests/testing/mochitest/runtests.py --total-chunks=5 --this-chunk=5 --chunk-by-dir=4 --autorun --console-level=INFO

I didn't get any more clue as to why it's failing.  I guess I can try running it with differently-sized chunks of tests to narrow down which prior test(s) might be causing the failure.

Comment 36

[Matt Brubeck (:mbrubeck)]

If I change to "--total-chunks=6 --this-chunk=6" the test succeeds.  The difference is that these tests directories are no longer included:

/layout/forms/test/
/layout/generic/test/
/layout/inspector/tests/
/layout/mathml/tests/

None of these directories were touched directly by either patch here.  Maybe some test in these directories depends on a file from /layout/base/tests or some other directory that was moved by these patches.

Comment 37

[Matt Brubeck (:mbrubeck)]

No file in these directories seems to refer to anything in any of the changed directories, unless I'm missing something.

Maybe some the change in order of the tests has just exposed an existing issue in test_bug_511615.html -- perhaps the same one that previously showed up intermittently as bug 542580.  I guess we can try attachment 505706 [details] [diff] [review] from that bug next...

Comment 38

[Ben Kelly [PTO, back July 24][:bkelly]]

Thanks for continuing the investigation on this!  I really need to setup a windows
environment.

What about simply increasing the timeout between steps from 50ms to something like
250ms?  Obviously not a long term solution, but if that resolves the failures then
it would suggest the issue is due to timing effects.

Also, its probably unrelated, but I've been running into bug 851641 in satchel as
well.

Comment 39

[Matt Brubeck (:mbrubeck)]

I tried re-running M5 with a updated version of the patch from bug 542580, but still got the same failure. :(

Comment 40

[Matt Brubeck (:mbrubeck)]

In the screenshot from the M5 timeouts on Try, the Firefox menu is open.  I saw the same thing when those tests failed locally, and it looked like it had been left open by a previous test before the satchel tests started.  I didn't see which previous test opened the menu.

Comment 41

[Matt Brubeck (:mbrubeck)]

toolkit/components/prompts/test/test_bug620145.html seems to leave the Firefox menu open in some situations -- for example when I run:

mach mochitest-plain --keep-open toolkit/components/prompts/test/test_bug620145.html

Comment 42

[Matt Brubeck (:mbrubeck)]

Created attachment 737066 [details] [diff] [review]
fix for test_bug620145.html

There is an duplicate synthesizeMouse in one of the code paths here.  On Windows this ends up hitting the Firefox Menu button, which is harmless for this test but can cause failures in later tests, as seen above.

This fixes the failure in test_bug_511615.html on my local machine.  Pushed all three latest patches to Try:
https://tbpl.mozilla.org/?tree=Try&rev=122944cf05d1

Comment 43

[Justin Dolske [:Dolske]]

Comment on attachment 737066 [details] [diff] [review]
fix for test_bug620145.html

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

Presumably this is hitting the Firefox button because ui.button0 is no longer in the document? I wonder if synthesizeMouse() should check for that... Probably not common enough to worry about?

Comment 44

[Ben Kelly [PTO, back July 24][:bkelly]]

Wow, nice catch!

I'm still trying to get a windows environment setup, but maybe this will be a
clue for the other failures:

(In reply to Matt Brubeck (:mbrubeck) from comment #36)
> /layout/forms/test/
> /layout/generic/test/
> /layout/inspector/tests/
> /layout/mathml/tests/

I added some debug to the mochitest harness to print out the full test list.  I
took a diff of the test list before and after applying the patches.  It appears
that somehow the path changes result in /layout/forms/test being added to the
mochitest-5 run.  The rest of these layout tests were already in mochitest-5.

The order of the tests did not change.

Comment 45

[Ben Kelly [PTO, back July 24][:bkelly]]

For what its worth, mochitest-5 now includes /layout/forms/test due to the way
directories are chunked.  The mochitest-5 target uses --chunk-by-dir=4 which means
it only looks at the first 4 directories in the test path.  As a result,
renaming:

  security/ssl/bugs
  security/ssl/mixedcontent/
  security/ssl/stricttransportsecurity

To:

  security/manager/ssl/tests/mochitest/bugs
  security/manager/ssl/tests/mochitest/mixedcontent
  security/manager/ssl/tests/mochitest/stricttransportsecurity

Causes the chunking algorithm to see security/manager/ssl/tests as a single
test directory instead of the previous three unique directories.  Having two
fewer total directories then causes the chunking algorithm to move a test dir
from mochitest-4 to mochitest-5 due to rounding differences.

Comment 46

[Matt Brubeck (:mbrubeck)]

Comment on attachment 737066 [details] [diff] [review]
fix for test_bug620145.html

https://hg.mozilla.org/integration/mozilla-inbound/rev/6155cad4bea3

Comment 47

[Phil Ringnalda (:philor)(back in August)]

https://hg.mozilla.org/mozilla-central/rev/6155cad4bea3

Comment 48

[Ben Kelly [PTO, back July 24][:bkelly]]

Comment on attachment 736121 [details] [diff] [review]
Updated Fixes for hardcoded path names in mochitests to match relativesrcdir.

Sorry, just realized attachment 737066 [details] [diff] [review] fixed both issues in the satchel
directory.  I thought there was still another problem to resolve.

Resubmitting the latest patch for review/feedback.

Comment 49

[David Baron :dbaron: ⌚️UTC-7]

Comment on attachment 736121 [details] [diff] [review]
Updated Fixes for hardcoded path names in mochitests to match relativesrcdir.

(In reply to Matt Brubeck (:mbrubeck) from comment #30)
> Comment on attachment 734970 [details] [diff] [review]
> Fix hardcoded path names in mochitests to match relativesrcdir.
> 
> Review of attachment 734970 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks a *ton* for doing this!  Again, all these changes look correct to me,
> but I'm flagging some module peers to alert them to the changes and give
> them a chance to review if they choose.

I'm obviously ok with this, given that I filed the bug. :-)

Comment 50

[Ben Kelly [PTO, back July 24][:bkelly]]

I ran another try:

https://tbpl.mozilla.org/?tree=Try&rev=979663f54c28

Looks like there is now a failure in mochitest-4 in dom/tests/geolocation.  There
was a commit on 4/12, so perhaps another relatvesrcdir dependent bit of code was
added.

Comment 51

[Ben Kelly [PTO, back July 24][:bkelly]]

This is the commit I was referring to from 4/12 that touched the geolocation
mochitests:

https://hg.mozilla.org/mozilla-central/rev/37b472c847b1

Comment 52

[Ben Kelly [PTO, back July 24][:bkelly]]

Same error apparently is a known error on Android:

https://bugzilla.mozilla.org/show_bug.cgi?id=778256

I'm still waiting for my updated tree to build.  Will have to investigate tomorrow
evening.  I don't see anything obviously related to the relativesrcdir.

Comment 53

[Ben Kelly [PTO, back July 24][:bkelly]]

Its also interesting that test_mozsettingsWatch.html succeeds while
test_mozsettings.html fails.  The code up until the failure point is identical.

If this is a timing issue, it could be related to the when the geolocation tests
run in the build.  With these patches the tests move from the end of mochitest-3
to be first thing in mochitest-4.

Comment 54

[Ben Kelly [PTO, back July 24][:bkelly]]

The mozSettings error appears unrelated to the relativesrcdir changes.  I can reproduce it and have submitted a workaround to bug 778256.  I believe its a race condition that was previously being masked by its location in the mochitest build order.

Comment 55

[Reuben Morais [:reuben]]

(In reply to Ben Kelly from comment #53)
> Its also interesting that test_mozsettingsWatch.html succeeds while
> test_mozsettings.html fails.  The code up until the failure point is
> identical.

That's fall off from bug 859554, which introduced a timing dependency on the Contacts and Settings tests. I just landed a fix in bug 862247.

Comment 56

[Reuben Morais [:reuben]]

(In reply to Reuben Morais [:reuben] from comment #55)
…a fix that also needs to be applied to those geolocation tests as well. Is there a way we set prefs on the profile used by mochitests? There's no reason to replicate that dance in every test that uses a preffed off API.

Comment 57

[Josh Matthews [:jdm]]

(In reply to Reuben Morais [:reuben] from comment #56)
> (In reply to Reuben Morais [:reuben] from comment #55)
> …a fix that also needs to be applied to those geolocation tests as well. Is
> there a way we set prefs on the profile used by mochitests? There's no
> reason to replicate that dance in every test that uses a preffed off API.

http://mxr.mozilla.org/mozilla-central/source/build/automation.py.in#460

Comment 58

[Ben Kelly [PTO, back July 24][:bkelly]]

I've opened bug 862666 to track the fixes to the geolocation tests.

Comment 59

[Ben Kelly [PTO, back July 24][:bkelly]]

Is it fair to request checkin at this point?  There is still feedback? request out to khuey and bug 862666 is in inbound, but not merged to m-c.  (Sorry, can't access irc at the moment to ask.)

Would be nice to move this forward as it seems susceptible to bitrot since it touches so many directories.

Thanks.

Comment 60

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8e0af273404e
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1096811620f

Comment 61

[Daniel Holbert [:dholbert]]

Unfortunately, it looks like this might've introduced some Android Mochitest-8 test failures in /tests/security/manager/ssl/tests/ :
https://tbpl.mozilla.org/php/getParsedLog.php?id=21935347&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=21935487&tree=Mozilla-Inbound

Looks like the recent try runs here didn't include Android (which is generally reasonable) -- so even with those green Try runs, it's feasible that this is causing the Android M-8 issues.

So: this probably needs a backout, but first I'm retriggering the orange M-8 runs and watching M-8 on the subsequent builds to see if they hit the same test-failure.

Comment 62

[Daniel Holbert [:dholbert]]

Actually, I just went ahead with the backout, since mbrubeck agreed that it's more likely that this is responsible for the M-8 oranges on its push than that they sporadically cropped up randomly.

(In reply to Ryan VanderMeulen [:RyanVM] from comment #60)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/8e0af273404e
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/ee1ba342612b

> https://hg.mozilla.org/integration/mozilla-inbound/rev/b1096811620f
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/d4ff0407496f

Comment 63

[Daniel Holbert [:dholbert]]

(while I was backing out, another Android M-8 job completed on the original TBPL run, which also suffered from the same problem:
 https://tbpl.mozilla.org/php/getParsedLog.php?id=21936395&tree=Mozilla-Inbound )

Comment 64

[Ben Kelly [PTO, back July 24][:bkelly]]

I think these are known issues, but the exclusion in android.json was not updated for the new path:

http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/android.json#301

I'll look for the other modified paths in config files and make a patch.

Comment 65

[Ben Kelly [PTO, back July 24][:bkelly]]

Created attachment 738827 [details] [diff] [review]
Fix android test exclusions to match new relativesrcdir paths.

Here is a patch updating the android.json test exclusions.  A quick search of json files did not show any other effected paths.

https://tbpl.mozilla.org/?tree=Try&rev=bb2dce856c25

Sorry for the breakage!

Comment 66

[Ben Kelly [PTO, back July 24][:bkelly]]

I also found a reference to the old "security/ssl" path here:

http://mxr.mozilla.org/mozilla-central/source/security/nss/tests/pkcs11/netscape/suites/security/ssl/manifest.mn#24

Its unclear to me if this is related or not.

Comment 67

[Ben Kelly [PTO, back July 24][:bkelly]]

Comment on attachment 738827 [details] [diff] [review]
Fix android test exclusions to match new relativesrcdir paths.

Android M8 is green with this patch:

https://tbpl.mozilla.org/?tree=Try&rev=bb2dce856c25

Running another full try to double check for other breakage:

https://tbpl.mozilla.org/?tree=Try&rev=02dcc645361a

Comment 68

[Daniel Holbert [:dholbert]]

Comment on attachment 738827 [details] [diff] [review]
Fix android test exclusions to match new relativesrcdir paths.

This makes sense - thanks for the quick work on the fixup here!

(In reply to Ben Kelly from comment #66)
> I also found a reference to the old "security/ssl" path here:
> 
> http://mxr.mozilla.org/mozilla-central/source/security/nss/tests/pkcs11/
> netscape/suites/security/ssl/manifest.mn#24
> 
> Its unclear to me if this is related or not.

(Probably not.  Generally, it's good to avoid touching security/nss -- it's an upstream library that we import periodically, so any local changes there will get clobbered on whenever we update from upstream. If you did hypothetically need to change something there, it'd be better to do it upstream.  But in this case it sounds like you don't, which is good.)

Comment 69

[Ben Kelly [PTO, back July 24][:bkelly]]

Thanks for catching the problem!

I'll let the full try run to completion and then throw the checkin-needed keyword back on in the morning (assuming there aren't new problems).

Do we need the [leave open] in the whiteboard still?  With a widespread change like this is it common to leave the bug open for a while until any issues have shaken out?

Thanks again.

Comment 70

[Daniel Holbert [:dholbert]]

(In reply to Ben Kelly from comment #69)
> Do we need the [leave open] in the whiteboard still?  With a widespread
> change like this is it common to leave the bug open for a while until any
> issues have shaken out?

Nope, it can be removed. (/me removes)

Generally, bugs are closed when their patches make it to mozilla-central (whether by merging from inbound or by directly landing there).   [leave open] is just a signal to keep sheriffs from auto-closing a bug when merging from inbound, in cases where a bug has more patches that haven't yet been landed anywhere.

(Looks like mbrubeck added it here when pushing an early patch in comment 46, to keep this bug from getting closed when that patch was merged to central. It was appropriate then, but isn't needed anymore.)

Comment 71

[Ben Kelly [PTO, back July 24][:bkelly]]

Thanks for the explanation.  That makes sense.

Sadly, I forgot to update my tree before kicking off that last try, so it hit the mozsettings failures in M4.  Here's another try with after a fresh hg pull -u:

https://tbpl.mozilla.org/?tree=Try&rev=983c03ae4092

Comment 72

[Ben Kelly [PTO, back July 24][:bkelly]]

The last try looks pretty green.  I don't see any consistent errors.

I'm not sure on the process for handling backed out commits, but these are the patches that need to be (re)applied:

  attachment 734261 [details] [diff] [review]
  attachment 736121 [details] [diff] [review]
  attachment 738827 [details] [diff] [review]

Comment 73

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/22ed2b874b4a
https://hg.mozilla.org/integration/mozilla-inbound/rev/46280589c1dc
https://hg.mozilla.org/integration/mozilla-inbound/rev/896d943683eb

Comment 74

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/22ed2b874b4a
https://hg.mozilla.org/mozilla-central/rev/46280589c1dc
https://hg.mozilla.org/mozilla-central/rev/896d943683eb