768642 - Add unit test coverage for Error Console filter

Status: RESOLVED WONTFIX

(Toolkit Graveyard :: Error Console, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

Bug 760951 added the filter to the error console and I should have requested that tests be added to ensure the proper functioning of the filter.

Tom, can you add some tests here? https://bugzilla.mozilla.org/show_bug.cgi?id=593540 has a test attachment that shows how this can be done.

Comment 1

[Tom S [:evilpie]]

I started looking into this, but it's all rather strange.

Comment 2

[Tom S [:evilpie]]

Attachment: WIP doesn't work

I copied most of the test code from bug 593540. But I can't really seem to get it working. In my case a newly added row, which should not be displayed, because it doesn't fit the filter. Doesn't get the "filtered-by-string" class and the test fails. Maybe this is some timing issue, or I am looking at the wrong node, I don't know.

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

I haven't had time yet to investigate the cause of the failure. Are you still working on this or are you waiting for some help here?

Comment 4

[Tom S [:evilpie]]

I wouldn't mind if you could take a look at this. This is very weird to me and I don't have so much time to finish this until I am gone for 3 weeks.

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 663056 [details] [diff] [review]
WIP doesn't work

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

Also, this will need to get reviewed by a Toolkit peer.

I'm not sure why this is failing exactly, but I have a suspicion that it is due to the undeclared pref being referenced.

Sorry for taking so long on this feedback pass.

::: toolkit/components/console/tests/browser/browser_errorConsoleContent.js
@@ +5,5 @@
> + *
> + * Contributor(s):
> + *   Rob Campbell <[e-mail]>
> + *
> + * ***** END LICENSE BLOCK ***** */

The license block should be formatted the same as this one and should drop the contributor list, http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/newtab/browser_newtab_bug721442.js#1

@@ +26,5 @@
> +  waitForExplicitFinish();
> +  gBrowser.selectedTab = gBrowser.addTab();
> +  content.location = TEST_URI;
> +  let tab = gBrowser.selectedTab;
> +  browser = gBrowser.getBrowserForTab(tab);

browser = gBrowser.selectedBrowser;

@@ +43,5 @@
> +  browser.addEventListener("load", onLoad, true);
> +}
> +
> +function getErrorConsoleContentPref() {
> +  return Services.prefs.getBoolPref("devtools.errorconsole.showContent");

I don't see a pref named devtools.errorconsole.showContent and get this when running the test, 
> TEST-INFO | chrome://mochitests/content/browser/toolkit/components/console/tests/browser/browser_errorConsoleContent.js | Console message: [JavaScript Error: "NS_ERROR_UNEXPECTED: Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.getBoolPref]" {file: "chrome://mochitests/content/browser/toolkit/components/console/tests/browser/browser_errorConsoleContent.js" line: 47}]

@@ +51,5 @@
> +  Services.prefs.setBoolPref("devtools.errorconsole.showContent", aBoolean);
> +}
> +
> +function onLoad(aEvent) {
> +  browser.removeEventListener(aEvent.type, arguments.callee, true);

browser.removeEventListener("load", onLoad, true);

arguments.callee is deprecated (https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Functions_and_function_scope/arguments/callee).

@@ +97,5 @@
> +      if (!nodes)
> +        return false; // no nodes
> +
> +      for (let idx in nodes) {
> +        if (nodes[idx].toString().indexOf(aMsg) != -1)

The above check for |if (!nodes)| isn't needed since the same result will be returned when nodes is empty, and childNodes can't be null here.

for (let node of nodes) {
  if (node.toString().contains(aMsg))
    return true;
}
return false;

::: toolkit/components/console/tests/browser/test-error.html
@@ +11,5 @@
> +      var button = document.getElementsByTagName("button")[0];
> +
> +      button.addEventListener("click", function clicker () {
> +        button.removeEventListener("click", clicker, false);
> +        fooBazBaz.bar();

fooBazBaz isn't defined here. Can you instead just dereference null so the error isn't found until this code is executed? Also, please include a comment to explicitly state that an error here is intended.

Comment 6

[Tom S [:evilpie]]

Sorry I totally forgot about this. I actually don't even remember how to run the tests :(

Comment 7

[Tom S [:evilpie]]

Attachment: wip v2 still doesn't work

This is an updated version of the previous tests. The chrome test already works, but the browser test doesn't handle tab loads in combination with page loads. Rob you were the original author, can you help me fix this?

Comment 8

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 717345 [details] [diff] [review]
wip v2 still doesn't work

sorry, Tom. I don't have time to help out with this right now. Maybe Jared can help you out?

Comment 9

[Tom S [:evilpie]]

Can we find somebody to help me out with this?

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Tom Schuster [:evilpie] from comment #9)
> Can we find somebody to help me out with this?

Yeah, I'll help you with this. Sorry for the huge delay.

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

Tom, what do you think of this? Both the browser and chrome tests now pass.

Comment 12

[Tom S [:evilpie]]

Comment on attachment 726212 [details] [diff] [review]
Patch

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

Seems to do what I wanted it to do. f+

::: toolkit/components/console/tests/chrome/test_errorConsole.js
@@ +69,5 @@
> +            SimpleTest.finish();
> +          }
> +        }
> +      }
> +    }, 1000); /* We need to wait for the console so it actually starts filtering. */

Would be awesome if you could find a better solution than I did. :)

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Tom Schuster [:evilpie] from comment #12)
> Comment on attachment 726212 [details] [diff] [review]
> Patch
> 
> Review of attachment 726212 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Seems to do what I wanted it to do. f+
> 
> ::: toolkit/components/console/tests/chrome/test_errorConsole.js
> @@ +69,5 @@
> > +            SimpleTest.finish();
> > +          }
> > +        }
> > +      }
> > +    }, 1000); /* We need to wait for the console so it actually starts filtering. */
> 
> Would be awesome if you could find a better solution than I did. :)

Thanks for catching that. Switching this to the doCommand, executeSoon approach from the other test should allow us to remove the setTimeout. I'll wait for Neil's review before updating the patch.

Comment 14

[Tom S [:evilpie]]

Review ping. I want to get finally this done, although probably I shouldn't be the assignee anymore.

Comment 15

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 726212 [details] [diff] [review]
Patch

Seeing if we can kick-start a review here :)

Comment 16

[Dave Townsend [:mossop]]

Comment on attachment 726212 [details] [diff] [review]
Patch

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

Doesn't seem to be any reason for this to be split across two different test files let alone two test suites, combine both the tests into the browser-chrome test.

You should also test that changing the filter correctly updates the list.

::: toolkit/components/console/tests/chrome/test_errorConsole.js
@@ +41,5 @@
> +    consoleFilter.focus()
> +    sendString("important", win);
> +
> +    setTimeout(function() {
> +      var observer = new MutationObserver(subtreeModified);

Seems overkill, can't you just report the errors then check the console lists them? What am I missing here?

@@ +69,5 @@
> +            SimpleTest.finish();
> +          }
> +        }
> +      }
> +    }, 1000); /* We need to wait for the console so it actually starts filtering. */

Why is this needed?

Comment 17

[Brian Grinstead [:bgrins]]

The Error Console has been removed in favor of the Browser Console (see Bug 1278368), and the component is going to be removed.  If this bug is also relevant in the Browser Console, please reopen and move this into Firefox -> Developer Tools: Console.

Comment 18

[Zack Weinberg (:zwol)]

I am mass-reopening and re-componenting every single one of the Toolkit:Error Console bugs that appear to have been closed without anyone even *glancing* at whether they were relevant to the Browser Console.

If you want to close a whole bunch of old bugs -- FOR ANY REASON -- it is YOUR RESPONSIBILITY to check EVERY SINGLE ONE OF THEM and make sure they are no longer valid.  Do not push that work onto the bug reporters.

(It's okay to close bugs that haven't been touched in years when they don't have enough information for you to figure out whether the problem is still relevant to the current software - the reporter probably isn't coming back to clarify.  But that is the ONLY situation where it is okay.)

(I'm going to have to do this in two steps because of the way the "change several bugs at once" form works.  Apologies for the extra bugspam.)

Comment 19

[Brian Grinstead [:bgrins]]

Doesn't translate to browser console, so resolving