Closed Bug 847279 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: General, defect)

Product:
Core
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla23

People

(Reporter: dbaron, Assigned: bkelly)

References

Details

Attachments

(4 files, 6 obsolete files)

dbaron's other mochitest patch from comment 5.
7.62 KB, patch
mbrubeck
: review+
khuey
: feedback+
mossop
: feedback+
briansmith
: feedback+
RyanVM
: checkin+
Details | Diff | Splinter Review
Updated Fixes for hardcoded path names in mochitests to match relativesrcdir.
52.22 KB, patch
mbrubeck
: review+
khuey
: feedback+
mossop
: feedback+
dbaron
: feedback+
briansmith
: feedback+
RyanVM
: checkin+
Details | Diff | Splinter Review
fix for test_bug620145.html
967 bytes, patch
Dolske
: review+
mbrubeck
: checkin+
Details | Diff | Splinter Review
Fix android test exclusions to match new relativesrcdir paths.
2.28 KB, patch
mbrubeck
: review+
dholbert
: feedback+
RyanVM
: checkin+
Details | Diff | Splinter Review 
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.
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.
You should be able to just remove the relativesrcdir from dom/imptests/Makefile.in.
(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
Blocks: 854868
Attached patch mochitest-chrome fixes (obsolete) — — Splinter Review 
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
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.
Attachment #732191 - Attachment is obsolete: true
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.
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.
Attachment #733717 - Attachment is obsolete: true
Here you go: windows, mac, and linux, all mochitest suites.

https://tbpl.mozilla.org/?tree=Try&rev=587fe00f45b5
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.
Yea, looks like the build failed because I hadn't added dbaron's patches.  Sorry about that.
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.
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
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.
Attachment #734249 - Attachment is obsolete: true
Shout if you'd like another push to try.
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?
No longer blocks: 854868
Depends on: 854868
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.
Attachment #734260 - Attachment is obsolete: true
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!
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!
Attachment #734406 - Attachment is obsolete: true
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?
Yep. I have retriggered it.
Attachment #734970 - Flags: review?(mbrubeck)
Attachment #734261 - Flags: review?(mbrubeck)
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 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.
Attachment #734261 - Flags: review?(mbrubeck)
Attachment #734261 - Flags: review+
Attachment #734261 - Flags: feedback?(khuey)
Attachment #734261 - Flags: feedback?(dtownsend+bugmail)
Attachment #734261 - Flags: feedback?(bsmith)
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.
Attachment #734970 - Flags: review?(mbrubeck)
Attachment #734970 - Flags: review+
Attachment #734970 - Flags: feedback?(khuey)
Attachment #734970 - Flags: feedback?(dtownsend+bugmail)
Attachment #734970 - Flags: feedback?(dbaron)
Attachment #734970 - Flags: feedback?(bsmith)
(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.
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.
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?
Attachment #734970 - Attachment is obsolete: true
Attachment #734970 - Flags: feedback?(khuey)
Attachment #734970 - Flags: feedback?(dtownsend+bugmail)
Attachment #734970 - Flags: feedback?(dbaron)
Attachment #734970 - Flags: feedback?(bsmith)
I can try this locally on Windows, though I might not get to it until sometime next week.
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.
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.
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...
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.
I tried re-running M5 with a updated version of the patch from bug 542580, but still got the same failure. :(
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.
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
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
Attachment #737066 - Flags: review?(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?
Attachment #737066 - Flags: review?(dolske) → review+
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.
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.
Assignee: nobody → ben
Status: NEW → ASSIGNED
Whiteboard: [leave open]
Attachment #734261 - Flags: feedback?(bsmith) → feedback+
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.
Attachment #736121 - Flags: review?(mbrubeck)
Attachment #736121 - Flags: feedback?(khuey)
Attachment #736121 - Flags: feedback?(dtownsend+bugmail)
Attachment #736121 - Flags: feedback?(dbaron)
Attachment #736121 - Flags: feedback?(bsmith)
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. :-)
Attachment #736121 - Flags: feedback?(dbaron) → feedback+
Attachment #736121 - Flags: feedback?(bsmith) → feedback+
Attachment #734261 - Flags: feedback?(dtownsend+bugmail) → feedback+
Attachment #736121 - Flags: feedback?(dtownsend+bugmail) → feedback+
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.
This is the commit I was referring to from 4/12 that touched the geolocation
mochitests:

https://hg.mozilla.org/mozilla-central/rev/37b472c847b1
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.
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.
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.
(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.
(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.
(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
I've opened bug 862666 to track the fixes to the geolocation tests.
Attachment #736121 - Flags: review?(mbrubeck) → review+
Depends on: 862666
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.
Keywords: checkin-needed
Attachment #734261 - Flags: checkin+
Attachment #736121 - Flags: checkin+
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.
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
(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 )
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.
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!
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 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
Attachment #738827 - Flags: review?(mbrubeck)
Attachment #738827 - Flags: feedback?(dholbert)
Attachment #738827 - Flags: review?(mbrubeck) → review+
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.)
Attachment #738827 - Flags: feedback?(dholbert) → feedback+
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.
(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.)
Whiteboard: [leave open]
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
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]
Keywords: checkin-needed
Attachment #738827 - Flags: checkin+
Duplicate of this bug: 790279
You need to log in before you can comment on or make changes to this bug.