I originally had a few browser chrome tests for the 'HUD' console. They always would hang. I consolidated them and they stopped hanging - so we could land it. Now we have over 100 checks, we should start splitting them up by functionality: storage net console output code completion etc...

This is the proposed test code split. The patch tries to be as minimal and straight forward as possible. I only removed the big test code file, and moved the test functions into separate files. Some notes I wrote down while working and testing the code: browser_WebConsoleBasicTests.js: testRegistries() testGetDisplayByURISpec() testHUDGetters() getAllHUDS() getHUDById() testGetDisplayByLoadGroup() testGetContentWindowFromHUDId() 19 tests. All pass. Problems: - When the test starts, it shows: JavaScript Error: "console is not defined" {file: test-console.html} - testUnregister() is unused. - the HUDService.shutdown() method cannot be invoked in this test, because if we do, subsequent tests fail to execute. Suggestions: - In testGetDisplayByLoadGroup() the content.location is set to TEST_HTTP_URI and then a function that expects results from the page load is executed immediately. This test should be executed on page load, with a browser DOMContentLoaded event handler. - In testGetDisplayByLoadGroup() the class attribute of each outputNode child is verified twice if it's equal to "hud-network". That should be cleaned up. - The content.location.href is set to TEST_URI on line 63, before any code is executed. Function test() adds a browser DOMContentLoaded event handler *after* that. I would suggest that content.location is changed only *after* the event listener is setup, to avoid potential execution issues. - Based on some preliminary testing, executeSoon() is not needed in function test(). Shall I change the code to execute normally without executeSoon()? Should I work on fixing any of these problems? browser_WebConsoleStorageTests.js: testCreateDisplay() testRecordEntry() testRecordManyEntries() testIteration() 13 tests. All pass. Problem: I sometimes get: Console message: [JavaScript Error: "Cannot get outputNode by id" {file: "resource://gre/modules/HUDService.jsm" line: 2956}] I believe there might be a bug in the HUDService module, in the FirefoxApplicationHooks.getOutputNodeById() method. Any thoughts? browser_WebConsoleAPITests.js: introspectLogNodes() testConsoleLoggingAPI() testExposedConsoleAPI() 26 tests. All pass. Problems: - testExposedConsoleAPI() is unused, I left it unused. Shall I enable it? browser_WebConsoleJSTermTests.js: testConsoleHistory() testOutputOrder() testNullUndefinedOutput() testExecutionScope() testCompletion() testPropertyProvider() 44 tests. All pass. Problems: - TEST_PROPERTY_PROVIDER_URI is unused. Shall I remove the constant? How about the file it references? browser_WebConsoleNetTests.js: testLogEntry() testNet() testPageReload() 10 tests. All pass. No problems that I could notice.

(In reply to comment #1) > browser_WebConsoleBasicTests.js: > ... > Problems: > - When the test starts, it shows: > JavaScript Error: "console is not defined" {file: test-console.html} > > - testUnregister() is unused. > > - the HUDService.shutdown() method cannot be invoked in this test, because if > we > do, subsequent tests fail to execute. perhaps we can isolate this test at the end of the test runs, in its own test. > > Suggestions: > > - In testGetDisplayByLoadGroup() the content.location is set to TEST_HTTP_URI > and then a function that expects results from the page load is executed > immediately. This test should be executed on page load, with a browser > DOMContentLoaded event handler. > indeed. good catch > - In testGetDisplayByLoadGroup() the class attribute of each outputNode child > is > verified twice if it's equal to "hud-network". That should be cleaned up. indeed > > - The content.location.href is set to TEST_URI on line 63, before any code is > executed. Function test() adds a browser DOMContentLoaded event handler *after* > that. I would suggest that content.location is changed only *after* the event > listener is setup, to avoid potential execution issues. yep. > > - Based on some preliminary testing, executeSoon() is not needed in function > test(). Shall I change the code to execute normally without executeSoon()? i'm not sure about that. I sometimes can not get anything to work in browser tests without executeSoon, remember, these tests are run in VMs on 3+ platforms > > Should I work on fixing any of these problems? YES, please! > > > browser_WebConsoleStorageTests.js: > ... > I sometimes get: > > Console message: [JavaScript Error: "Cannot get outputNode by id" {file: > "resource://gre/modules/HUDService.jsm" line: 2956}] > > I believe there might be a bug in the HUDService module, in the > FirefoxApplicationHooks.getOutputNodeById() method. Any thoughts? Yes, this has to do with the way that the content-document-global-created event works see bug 549539, which *should* be fixed by bug 580352 > > > browser_WebConsoleAPITests.js: > ... > Problems: > > - testExposedConsoleAPI() is unused, I left it unused. Shall I enable it? > Sounds like something we would like to have, however, I am not sure why it is disabled. > > browser_WebConsoleJSTermTests.js: > ... > Problems: > > - TEST_PROPERTY_PROVIDER_URI is unused. Shall I remove the constant? How about > the file it references? you should ask jviereck about this one

i will try to get to the feedback this afternoon.

(In reply to comment #2) > > browser_WebConsoleAPITests.js: > > ... > > Problems: > > > > - testExposedConsoleAPI() is unused, I left it unused. Shall I enable it? > > > Sounds like something we would like to have, however, I am not sure why it is > disabled. I'ver added that test and don't know why it got deactivated. Might be due to some patch-rebasing-rejects-handling action. Definitely should be activated again. > > > > browser_WebConsoleJSTermTests.js: > > ... > > Problems: > > > > - TEST_PROPERTY_PROVIDER_URI is unused. Shall I remove the constant? How about > > the file it references? > you should ask jviereck about this one Good catch. I wanted to use this for testing the propertyProvider, but there were some issues so that I don't use that page anymore. Can be removed.

Comment on attachment 460062 [details] [diff] [review] proposed split The patch did not properly remove the original file, but maybe that is an hg thing where it is "playing it safe". So there was a single .reg file that indicated the HUDServiceTestsAll.js file was not removed. Other than that it looks great. after this lands the command to run all tests is: TEST_PATH=toolkit/components/console/hudservice/tests/browser/ make mochitest-browser-chrome r+ with fixed file removal. also, you need to manually do this: (in obj dir) cd _tests/testing/mochitest/browser/toolkit/components/console/hudservice/tests/browser && rm browser_HUDServiceTestsAll.js

> The patch did not properly remove the original file, but maybe that is an hg > thing where it is "playing it safe". You need to do two things: 1. make sure you do hg rm locally not deleting the file through the fs. 2. generate a git style patch (this handles cp, mv, and rm'ed files properly). (Mercurial recognizes -git style patches fine).

Thanks David for your feedback+ and comments, also thanks Philip for your suggestion. This is the updated patch in git format, as requested. I hope that now the browser_HUDServiceTestsAll.js file will be removed properly. Other patch changes: - I rebased the patch for mozilla-central. - Added the testInputFocus() function to browser_WebConsoleBasicTests.js file. - Added the testErrorOnPageReload() function to browser_ConsoleNetTests.js file. - Enabled the testExposedConsoleAPI() function call in the browser_WebConsoleAPITests.js file, as per discussion above. It runs fine. - Removed the networking test code that did not work correctly... from the browser_WebConsoleBasicTests.js file, in the testGetDisplayByLoadGroup() function. I moved the code to browser_WebConsoleNetTests() ... reincarnated as testXHR(). Fixing the test code required that I also update the test-observe-http-ajax.html. Now the test works fine. - Removed the use of executeSoon() in browser_WebConsoleBasicTests.js - it's not used in the rest of the test files anyway, and there seem to be no issues. - Removed the TEST_PROPERTY_PROVIDER_URI constant from the browser_WebConsoleJSTermTests.js file. I also removed the file it was pointing to: test-property-provider.html. All 114 tests run fine.

(In reply to comment #6) > > The patch did not properly remove the original file, but maybe that is an hg > > thing where it is "playing it safe". > > You need to do two things: > 1. make sure you do hg rm locally not deleting the file through the fs. > 2. generate a git style patch (this handles cp, mv, and rm'ed files properly). > (Mercurial recognizes -git style patches fine). can we not use hg mv file1 file2 or is that what caused the original problem? I thought that worked in recent versions of mercurial.

(In reply to comment #8) > (In reply to comment #6) > > > The patch did not properly remove the original file, but maybe that is an hg > > > thing where it is "playing it safe". > > > > You need to do two things: > > 1. make sure you do hg rm locally not deleting the file through the fs. > > 2. generate a git style patch (this handles cp, mv, and rm'ed files properly). > > (Mercurial recognizes -git style patches fine). > > can we not use hg mv file1 file2 or is that what caused the original problem? I > thought that worked in recent versions of mercurial. whups. I thought this was for a straight rename, not breaking up into smaller files. I will endeavour to read harder.

Comment on attachment 460840 [details] [diff] [review] updated patch Looks good. Is it preferable to do an hg copy from TestsAll to each new file and then commit, edit each test file and commit again in order to preserve blame? maybe it does not matter as these are 'just tests'? what do you think?

Did the original file have any significant history? If not I wouldn't bother (comm-central policy, On the other hand I believe that mozilla-central prefers to hg copy everything on the grounds that storage is cheap).

(In reply to comment #11) > Did the original file have any significant history? not really > If not I wouldn't bother > (comm-central policy, On the other hand I believe that mozilla-central prefers > to hg copy everything on the grounds that storage is cheap). ah, ok, thx.

Resurrecting this for beta 6, because the current tests are hampering our ability to land console changes.

I've rebased to trunk and split up the tests even more, with the goal of adhering strictly to "one function, one file". This patch doesn't have all of the current tests in it, however: the asynchronous tests after "testNet" are missing.

New patch is rebased to trunk and fully splits out all the tests into 33 different files. I'm baking it on the try server so we can see if this helps with the random oranges. Feedback requested. If this proves to be helpful, I'll see what I can do to make this monster patch (that's mostly just shuffling lines around and adding infrastructure) easier to review.

(In reply to comment #15) > Feedback requested. If this proves to be helpful, I'll see what I can do to > make this monster patch (that's mostly just shuffling lines around and adding > infrastructure) easier to review. Awesome. Looking at this asap.

I've attached a "map of HUDServiceTestsAll.js" to make this monster patch a little easier to review. To the left of each line is a number which corresponds to the file each line now appears in. You can find the legend that tells you the meaning of the numbers at the bottom of the file.

Comment on attachment 471368 [details] [diff] [review] Proposed patch. 2 "little" things: 1. You have to add the license header to each test file, bummer, I know:( 2. It would be great to have a comment at the top of each "non-bug-number" test explaining what it is testing. Just a sentence will do. Looks good. r+ with those changes I am also running a new build to test these on Windows.

It looks like the patch does not delete the main test file correctly - or something. I ended up with a failed hunk being the entire main test file with each line prepended with a minus.

on mac: all run perfectly, no leaks:)

New version adds license text and comments to the top of each subtest.

New version of the patch rebases to trunk. Hopefully this will fix the problem with browser_HUDServiceTestsAll.js not being deleted properly.

Comment on attachment 471698 [details] [diff] [review] Proposed patch, version 2.2. So far so good. Works on Mac, 2 tiny failures on Windoze: TEST-PASS | automationutils.processLeakLog() | no leaks detected! INFO | runtests.py | Running tests: end. mochitest-browser-chrome failed: TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/components/console/hudservice/tests/browser/browser_webconsole_basic_net_lo gging.js | Four children in output - Got 2, expected 5 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/components/console/hudservice/tests/browser/browser_webconsole_basic_net_lo gging.js | html page is logged - Didn't expect -1, but got it TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/components/console/hudservice/tests/browser/browser_webconsole_basic_net_lo gging.js | Test timed out make: *** [mochitest-browser-chrome] Error 1

(In reply to comment #23) > Comment on attachment 471698 [details] [diff] [review] > Proposed patch, version 2.2. > > So far so good. Works on Mac, 2 tiny failures on Windoze: > > TEST-PASS | automationutils.processLeakLog() | no leaks detected! > > INFO | runtests.py | Running tests: end. > mochitest-browser-chrome failed: > TEST-UNEXPECTED-FAIL | > chrome://mochikit/content/browser/toolkit/components/console/hudservice/tests/browser/browser_webconsole_basic_net_lo > gging.js | Four children in output - Got 2, expected 5 > TEST-UNEXPECTED-FAIL | > chrome://mochikit/content/browser/toolkit/components/console/hudservice/tests/browser/browser_webconsole_basic_net_lo > gging.js | html page is logged - Didn't expect -1, but got it > TEST-UNEXPECTED-FAIL | > chrome://mochikit/content/browser/toolkit/components/console/hudservice/tests/browser/browser_webconsole_basic_net_lo > gging.js | Test timed out > make: *** [mochitest-browser-chrome] Error 1 I've been having a lot of grief with that test. It seems that sometimes the HTML page that was just loaded just doesn't show up in the output. Maybe this was the random orange? What should I do? Seems like my options are (a) to just remove that part of the test and make a note; (b) add setTimeout calls and hope for the best, or (c) dig more deeply into the platform side of things to figure out why the network request doesn't always make it to the console in time.

(In reply to comment #24) > I've been having a lot of grief with that test. It seems that sometimes the > HTML page that was just loaded just doesn't show up in the output. Maybe this > was the random orange? > It does seem like a timing issue. an executeSoon might help, but make sure are waiting until DOMContentLoaded is fired before running the test. Sometimes you have to use nested executeSoon calls as well. lame. > What should I do? Seems like my options are (a) to just remove that part of the > test and make a note; (b) add setTimeout calls and hope for the best, or (c) > dig more deeply into the platform side of things to figure out why the network > request doesn't always make it to the console in time. DOn't spend a whole lot of time, we can comment it out and file a followup bug, as well as placing a TODO comment in the test.

(In reply to comment #25) > It does seem like a timing issue. an executeSoon might help, but make sure are > waiting until DOMContentLoaded is fired before running the test. Tried that, didn't work :( > DOn't spend a whole lot of time, we can comment it out and file a followup bug, > as well as placing a TODO comment in the test. Ok, looks like that'll be tomorrow if nobody else gets to it due to waiting on builds, but I'll make it first thing.

Patch version 3 removes the waits in browser_webconsole_basic_net_logging.js, which seems to fix the problem for me... Baking on try server. Unfortunately I don't know what caused it to start working, or whether it's really working at all.

Patrick: I see errors when I try to run the split tests. Here they are: TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/components/console/hudservice/tests/browser/browser_webconsole_network_panel.js | Test timed out This is shown before any output from that script, so I think it doesn't start properly - I expect there are issues with the event handlers. Another error: TEST-INFO | chrome://mochikit/content/browser/toolkit/components/console/hudservice/tests/browser/browser_webconsole_null_and_undefined_output.js | Console message: [JavaScript Error: "null has no properties" {file: "chrome://mochikit/content/browser/toolkit/components/console/hudservice/tests/browser/browser_webconsole_null_and_undefined_output.js" line: 74}] TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/components/console/hudservice/tests/browser/browser_webconsole_null_and_undefined_output.js | Test timed out That looks to me like "welcome to the world of royal GC pains". The object from the weak ref is not good. You have to get the jsterm in some other way... I get the same error in the following tests: - browser_webconsole_null_and_undefined_output.js - browser_webconsole_output_order.js - browser_webconsole_property_panel.js - browser_webconsole_property_provider.js It's a problem with the HUD weak ref. It was only magic that the code of these tests worked in the big test code. We can't leave this code like that, because for me and I believe for others, the tests are not runnable. I can't run, as you can see, most of these tests. Unfortunately, I can't test for memleaks, due to this breakage.

On windows with a clobber build i gte: INFO | runtests.py | Running tests: end. mochitest-browser-chrome failed: TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/components/console/hudservice/tests/browser/browser_webconsole_basic_net_lo gging.js | Four children in output - Got 2, expected 5 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/components/console/hudservice/tests/browser/browser_webconsole_basic_net_lo gging.js | html page is logged - Didn't expect -1, but got it TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/components/console/hudservice/tests/browser/browser_webconsole_basic_net_lo gging.js | Test timed out make: *** [mochitest-browser-chrome] Error 1

(In reply to comment #29) Same random orange that's been plaguing us forever. I'm going to just comment that test out.

I did some further tests and tried to fix the issues affecting this patch. Background info: * In bug 580618 Julian provided us with a patch to fix GC issues. I have updated it in bug 578658, attachment 459449 [details] [diff] [review] with more fixes/improvements for JSTerm - old patch. Julian then reused his work in the patch for bug 583476, see attachment 470758 [details] [diff] [review]. Today I updated his patch to include fixes for JSTerm (and other minor stuff), see attachment 472728 [details] [diff] [review]. * In bug 587658, latest attachment 466279 [details] [diff] [review] (which is old, actually), I gave up using weakrefs altogether, and I switched to strong refs for HUD objects. Today I have rebased that patch on top of the latest patch from bug 583476, see attachment 472773 [details] [diff] [review], submitted in bug 580618. I have used attachment 472728 [details] [diff] [review] from bug 583476, and attachment 472773 [details] [diff] [review] from bug 580618, to test how the split test files run (attachment 472030 [details] [diff] [review] - proposed patch, v3). Results: * normal builds (non-debug): - default branch: all tests pass, no issues. - only split tests patch on top of the default branch: as explained in comment #28 I have tests failing, because the HUD weakrefs are cleared too early. - split tests patch on top of the GC fix attempt from bug 583476: all tests pass. - split tests patch on top of the GC fix attempt from bug 580618 (strong refs): all tests pass. * debug builds: - default branch: network bug fails, as known, it randomly fails for others, but it seems it always fails for me. I get memleaks as well. - only split tests patch on top of the default branch: all tests pass, no memleaks. Weird, no GC issues with the HUD weakrefs. - split tests patch on top of the GC fix attempt from bug 583476: all tests pass, no memleaks. - split tests patch on top of the GC fix attempt from bug 580618: all tests pass, no memleaks. - only the patch from bug 583476: like the default branch - the network test fails, and I get memleaks. - only the patch from bug 580618: like the default branch - the network test fails, and I get memleaks. Weird results. Thoughts: * it looks like having the patch from bug 583476 helps with running the split test files. * it looks like having the patch from bug 580618 doesn't really add any value, since we no longer have the GC issue (it seems, but I somehow doubt that, hehe). * default branch has memleaks? somehow I think they are caused by the network test failure. Anything I should test in more detail?

after running all tests on Windows (all mochitests) I get 0 leaks, but I get some failures - which may be due to the new patches that JUST landed on m-c.

Entire log from a mochitest run on mac, with leaks, but they appear to be the RDF service. Also, just the 2 tests that failed previously (in a local -no leaks- test run) are still failing

Comment on attachment 472030 [details] [diff] [review] Proposed patch, version 3. I think we must have introduced some changes very recently that make a lot of these (split up) tests fail or timeout on windows. The mac runs this patch pretty well, with only 2 failures: == BloatView: ALL (cumulative) LEAK AND BLOAT STATISTICS, default process 91412 |<----------------Class--------------->|<-----Bytes------>|<----------------Objects---------------->|<--------------References-------------->| Per-Inst Leaked Total Rem Mean StdDev Total Rem Mean StdDev 0 TOTAL 19 0 7881789 0 ( 2053.12 +/- 3823.78) 6593644 0 ( 3134.25 +/- 5356.73) nsTraceRefcntImpl::DumpStatistics: 875 entries TEST-PASS | automationutils.processLeakLog() | no leaks detected! INFO | runtests.py | Running tests: end. mochitest-browser-chrome failed: TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/components/console/hudservice/tests/browser/browser_webconsole_js_input_expansion.js | got 3 rows - Got 1, expected 3 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/components/console/hudservice/tests/browser/browser_webconsole_js_input_expansion.js | got 8 rows - Got 1, expected 8 make: *** [mochitest-browser-chrome] Error 1 Looking good, not there yet. We need to make sure to always run this suite on windows.

Just wanted to mention that the fact remains that the existing suite of tests is run dozens of times a day on tinderbox and we get no leaks that I know of. When there is a leak I am pinged almost immediately by philor or the sheriff.

Going to wait on bug 595350 to land this, so that we can make sure that leaks don't cause this patch to bounce.

Code checked into Firefox must not leak, in any configuration or test arrangement. If the code does leak, then we have a bad bug that will harm our users. I am going to work with pwalton and ddahl to get this patch landed. We'll document the steps we took to resolve any leaks that remain.

(In reply to comment #37) > Code checked into Firefox must not leak, in any configuration or test > arrangement. If the code does leak, then we have a bad bug that will harm our > users. > > I am going to work with pwalton and ddahl to get this patch landed. We'll > document the steps we took to resolve any leaks that remain. Now that bug 595350 has landed, the memory leaks in the Console should be gone. The reason that the patches in this test showed leaks on try is that they didn't consistently close the Web Console manually before closing the tab they were using. Because of bug 595350, this resulted in leaks. It would still be good to land this patch, however, to improve the maintainability of the test suite.

(In reply to comment #38) > > Now that bug 595350 has landed, the memory leaks in the Console should be gone. OK, great. Attachment 472030 [details] [diff] almost applies, but it looks like some more stuff has been added to browser_HUDServiceTestsAll.js since that patch. Patrick, can you rebase?

I'm going to take this and add the head.js setUp and register a tearDown function as well as add all generic code to head.js.

Pushing to try server

Pushed a WIP to try earlier, had 2 checks fail. I think I fixed those checks. This previous try run had a false positive leak and a leak that is apparently unrelated to our code, according to philor in developers: ddahl> hey philor: how did you figure out it was bug 600843? <ddahl> this one looks like a real leak: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1286735868.1286738109.11464.gz&fulltext=1 <philor> ddahl: mochitest-chrome + 15 nsXULElements + no domwindows + ~350K on Linux is a 600843, at least until someone finds a way to separate them if that really covers more than one <ddahl> jeez <philor> they're both real leaks, but if that only happens intermittently, then it's bug 601381 <ddahl> hmm, so i am seeing leaks but they are not related to my test suite <ddahl> that's good <philor> not obviously-related <ddahl> philor: so much nuance to my untrained eye <philor> 601381 in particular is likely to actually be "dump every single browser-chrome leak here"

Test-only patch that the split tests patch relies on. KHeuey totally bitrotted us in a "build config" patch that moved HUDService from "gre" to resource:///modules/HUDService.jsm - see bug 601890

more infor on the leaks from philir on #developers: <philor> ddahl: I don't really know enough about analyzing leaks to be sure, but I think that the leak in http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1286735868.1286738109.11464.gz&fulltext=1 is the same as the leaks in bug 601381, which I think are actually the fault of http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/browser_inspector_iframeTest.js

At this point with 2 tests disabled, (followup bugs filed), we have 1 error on *linux debug* only: s: talos-r3-fed-005 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/components/console/hudservice/tests/browser/browser_webconsole_basic_net_logging.js | found test-image TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/components/console/hudservice/tests/browser/browser_webconsole_basic_net_logging.js | found network console I'll tweak it and push to try server again.

also: no leaks on this last try server run

http://hg.mozilla.org/mozilla-central/rev/8a6579d39692 http://hg.mozilla.org/mozilla-central/rev/5264e36dbcab